[ovs-dev] [PATCHv2] netdev-afxdp: Detect numa node id.

[Ilya Maximets]

Hi, William.

Thanks for the patch.
Few general comments on the topic:
1. This function is not afxdp specific. Maybe it's worth to move
    it to more generic netdev-linux?
2. netdev-linux caches most of things like mtu and ifindex.
    Maybe we could cache numa_id too and not read it all the time
    from the filesystem?
3. More comments inline.

Best regards, Ilya Maximets.

On 27.09.2019 20:26, 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.
> 
> Signed-off-by: William Tu <u9012063 at gmail.com>
> ---
> v2:
>    Address feedback from Eelco
>      fix memory leak of xaspintf
>      log using INFO instead of WARN
> ---
>   lib/netdev-afxdp.c | 41 ++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
> index 6e01803272aa..6ff1473461a6 100644
> --- a/lib/netdev-afxdp.c
> +++ b/lib/netdev-afxdp.c
> @@ -552,11 +552,42 @@ out:
>   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));
> +    const char *numa_node_path;
> +    long int node_id;
> +    char buffer[4];
> +    FILE *stream;
> +    int n;
> +
> +    numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node",
> +                               netdev_get_name(netdev));

Do we need some escaping here like we have for vhost:
     
     strchr(name, '/') || strchr(name, '\\')

For security reasons?

> +    stream = fopen(numa_node_path, "r");
> +    if (!stream) {
> +        /* Virtual device does not have this info. */
> +        VLOG_INFO_RL(&rl, "Open %s failed: %s, use numa_id 0",
> +                     numa_node_path, ovs_strerror(errno));

How about:

         VLOG_INFO_RL(&rl, "%s: Can't open '%s': %s, using numa_id 0",
                      netdev_get_name(netdev), numa_node_path,
                      ovs_strerror(errno));

In general, I'd like if we could print only 'fopen' related error here
and report about 'numa_id 0' in the end of function in the last message.

> +        free(numa_node_path);
> +        return 0;
> +    }
> +
> +    n = fread(buffer, 1, sizeof buffer, stream);

Why fread?  It looks much easier to use fscanf instead.
And you'll not need to parse the resulted string in this case.

> +    if (!n) {
> +        goto error;
> +    }
> +
> +    node_id = strtol(buffer, NULL, 10);

Anyway, please, use str_to_int() from lib/util.h instead.

> +    if (node_id < 0 || node_id > 2) {

This check looks strange. Even on Intel platforms, systems with
more than 2 NUMA nodes are widely available.
You may use ovs_numa_numa_id_is_valid() instead.

> +        goto error;
> +    }
> +
> +    fclose(stream);
> +    free(numa_node_path);
> +    return (int)node_id;
> +
> +error:
> +    VLOG_WARN_RL(&rl, "Error detecting numa node of %s, use numa_id 0",
> +                 numa_node_path);

     VLOG_WARN_RL(&rl, "%s: Can't detect NUMA node, using numa_id 0",
                  netdev_get_name(netdev));

> +    free(numa_node_path);
> +    fclose(stream);
>       return 0;
>   }
>   
>