[ovs-dev] [PATCHv5] netdev-linux: Detect numa node id.

[William Tu]

On Tue, Oct 29, 2019 at 9:15 AM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> On 23.10.2019 23:08, William Tu wrote:
> > The patch detects the numa node id from the name of the netdev,
> > by reading the '/sys/class/net/<devname>/device/numa_node'.
> > If not available, ex: virtual device, or any error happens,
> > return numa id 0.  Currently only the afxdp netdev type uses it,
> > other linux netdev types are disabled due to no use case.
> >
> > Signed-off-by: William Tu <u9012063 at gmail.com>
> > ---
> > v5:
> >    Feedbacks from Ilya
> >    - reafactor the error handling
> >    - add mutex lock
> >    - Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/601947245
> >
> > v4:
> >    Feedbacks from Eelco
> >    - Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/599308893
> >
> > v3:
> >    Feedbacks from Ilya and Eelco
> >    - update doc, afxdp.rst
> >    - fix coding style
> >    - fix limit of numa node max, by using ovs_numa_numa_id_is_valid
> >    - move the function to netdev-linux
> >    - cache the result of numa_id
> >    - add security check for netdev name
> >    - use fscanf instead of read and convert to int
> >    - revise some error message content
> >
> > v2:
> >    address feedback from Eelco
> >       fix memory leak of xaspintf
> >       log using INFO instead of WARN
> > ---
> >   Documentation/intro/install/afxdp.rst |  1 -
> >   lib/netdev-afxdp.c                    | 11 ------
> >   lib/netdev-linux-private.h            |  2 ++
> >   lib/netdev-linux.c                    | 63 ++++++++++++++++++++++++++++++++++-
> >   4 files changed, 64 insertions(+), 13 deletions(-)
> >
> > diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst
> > index 820e9d993d8f..6c00c4ad1356 100644
> > --- a/Documentation/intro/install/afxdp.rst
> > +++ b/Documentation/intro/install/afxdp.rst
> > @@ -309,7 +309,6 @@ Below is a script using namespaces and veth peer::
> >
> >   Limitations/Known Issues
> >   ------------------------
> > -#. Device's numa ID is always 0, need a way to find numa id from a netdev.
> >   #. No QoS support because AF_XDP netdev by-pass the Linux TC layer. A possible
> >      work-around is to use OpenFlow meter action.
> >   #. Most of the tests are done using i40e single port. Multiple ports and
> > diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
> > index 8eb270c150e8..cfd93fab9f45 100644
> > --- a/lib/netdev-afxdp.c
> > +++ b/lib/netdev-afxdp.c
> > @@ -543,17 +543,6 @@ out:
> >       return err;
> >   }
> >
> > -int
> > -netdev_afxdp_get_numa_id(const struct netdev *netdev)
> > -{
> > -    /* FIXME: Get netdev's PCIe device ID, then find
> > -     * its NUMA node id.
> > -     */
> > -    VLOG_INFO("FIXME: Device %s always use numa id 0.",
> > -              netdev_get_name(netdev));
> > -    return 0;
> > -}
> > -
> >   static void
> >   xsk_remove_xdp_program(uint32_t ifindex, int xdpmode)
> >   {
> > diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
> > index a350be151147..c8f2be47b10b 100644
> > --- a/lib/netdev-linux-private.h
> > +++ b/lib/netdev-linux-private.h
> > @@ -96,6 +96,8 @@ struct netdev_linux {
> >       /* LAG information. */
> >       bool is_lag_master;         /* True if the netdev is a LAG master. */
> >
> > +    int numa_id;                /* NUMA node id. */
> > +
> >   #ifdef HAVE_AF_XDP
> >       /* AF_XDP information. */
> >       struct xsk_socket_info **xsks;
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index f4819237370a..4dd05493b238 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -236,6 +236,7 @@ enum {
> >       VALID_VPORT_STAT_ERROR  = 1 << 5,
> >       VALID_DRVINFO           = 1 << 6,
> >       VALID_FEATURES          = 1 << 7,
> > +    VALID_NUMA_ID           = 1 << 8,
> >   };
> >
> >   struct linux_lag_slave {
> > @@ -1391,6 +1392,66 @@ netdev_linux_tap_batch_send(struct netdev *netdev_,
> >       return 0;
> >   }
> >
> > +static int
> > +netdev_linux_get_numa_id__(const struct netdev *netdev_)
>
> Since you've created a separate function, I think we need to add
> a thread safety annotation here: OVS_REQUIRES(netdev->mutex)
>
OK thanks!

> For this purpose, you'll better pass the instance of 'netdev_linux'
> in this function.  Original netdev only used to get the netdev name,
> so it'll be not hard to use &netdev->up instead for that purpose.
>
OK

> Second thing is that we should preserve numa cache on netlink updates.
> Something like this:
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 4dd05493b..0495a035f 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -821,9 +821,9 @@ netdev_linux_update__(struct netdev_linux *dev,
>   {
>       if (rtnetlink_type_is_rtnlgrp_link(change->nlmsg_type)) {
>           if (change->nlmsg_type == RTM_NEWLINK) {
> -            /* Keep drv-info, and ip addresses. */
> +            /* Keep drv-info, ip addresses and NUMA id. */
>               netdev_linux_changed(dev, change->ifi_flags,
> -                                 VALID_DRVINFO | VALID_IN);
> +                                 VALID_DRVINFO | VALID_IN | VALID_NUMA_ID);
>
>               /* Update netdev from rtnl-change msg. */
>               if (change->mtu) {
> ---
>
> Without this change we're re-checking the numa id after each time device
> flags/mtu/whatever updated.

good catch.
I will work on the next version.
Thanks
William