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

William Tu u9012063 at gmail.com
Tue Oct 1 20:06:33 UTC 2019


Hi Ilya,

Thanks for your review.

On Mon, Sep 30, 2019 at 10:18:32PM +0300, Ilya Maximets wrote:
> 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.
> 
OK, let me see how to make it more generic in netdev-linux.

> 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?
> 
Sure, thanks. 

> >+    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.
> 
OK.

> >+        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.
>

Thanks I will switch to use fscanf

> >+    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.
> 
Good idea. 

Regards,
William



More information about the dev mailing list