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

Ilya Maximets i.maximets at ovn.org
Mon Oct 21 13:18:51 UTC 2019


On 17.10.2019 22: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>
> ---
> 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                    | 58 ++++++++++++++++++++++++++++++++++-
>   4 files changed, 59 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..add148d83eb7 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,61 @@ netdev_linux_tap_batch_send(struct netdev *netdev_,
>       return 0;
>   }
>   
> +static int OVS_UNUSED
> +netdev_linux_get_numa_id(const struct netdev *netdev_)
> +{
> +    struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> +    char *numa_node_path;
> +    const char *name;
> +    int node_id;
> +    FILE *stream;
> +

'netdev' fields should be protected by netdev->mutex.  So, this
function should take it before accessing 'numa_id' or 'cache_valid'.

> +    if (netdev->cache_valid & VALID_NUMA_ID) {
> +        return netdev->numa_id;
> +    }
> +
> +    netdev->numa_id = 0;
> +    netdev->cache_valid |= VALID_NUMA_ID;
> +
> +    name = netdev_get_name(netdev_);
> +    if (strpbrk(name, "/\\")) {
> +        VLOG_ERR_RL(&rl, "\"%s\" is not a valid name for a port. "
> +                    "A valid name must not include '/' or '\\'."
> +                    "Using numa_id 0", name);
> +        return 0;
> +    }
> +
> +    numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node", name);
> +
> +    stream = fopen(numa_node_path, "r");
> +    if (!stream) {
> +        /* Virtual device does not have this info. */
> +        VLOG_INFO_RL(&rl, "%s: Can't open '%s': %s, using numa_id 0",
> +                     name, numa_node_path, ovs_strerror(errno));
> +        free(numa_node_path);
> +        return 0;
> +    }
> +
> +    if (fscanf(stream, "%d", &node_id) != 1) {
> +        goto error;
> +    };

Redundant ';'.

> +
> +    if (!ovs_numa_numa_id_is_valid(node_id)) {
> +        goto error;
> +    }

Maybe it will look better if above 2 'if's will be combined like this:

if (fscanf(stream, "%d", &node_id) != 1
     || !ovs_numa_numa_id_is_valid(node_id) {
     goto error;
}

What do you think?

> +
> +    netdev->numa_id = node_id;
> +    fclose(stream);
> +    free(numa_node_path);
> +    return node_id;
> +
> +error:
> +    VLOG_WARN_RL(&rl, "%s: Can't detect NUMA node, using numa_id 0", name);
> +    free(numa_node_path);
> +    fclose(stream);
> +    return 0;
> +}
> +


Or even like this:

     if (fscanf(stream, "%d", &node_id) != 1
         || !ovs_numa_numa_id_is_valid(node_id) {
         VLOG_WARN_RL(&rl, "%s: Can't detect NUMA node, using numa_id 0", name);
         node_id = 0;
     }

     netdev->numa_id = node_id;
     fclose(stream);
     free(numa_node_path);
     return node_id;
}

So we'll not need to duplicate cleanup code.

Best regards, Ilya Maximets.


More information about the dev mailing list