[ovs-dev] netdev-afxdp: Detect numa node id.
diff mbox series

Message ID 1568394931-82238-1-git-send-email-u9012063@gmail.com
State Superseded
Headers show
Series
  • [ovs-dev] netdev-afxdp: Detect numa node id.
Related show

Commit Message

William Tu Sept. 13, 2019, 5:15 p.m. UTC
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@gmail.com>
---
 lib/netdev-afxdp.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

Comments

0-day Robot Sept. 13, 2019, 5:59 p.m. UTC | #1
Bleep bloop.  Greetings William Tu, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
#35 FILE: lib/netdev-afxdp.c:561:
    numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node",

Lines checked: 67, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
William Tu Sept. 13, 2019, 6:07 p.m. UTC | #2
Hi Aaron,

I think this might be a false positive?
I didn't find any lack whitespace here....

On Fri, Sep 13, 2019 at 11:01 AM 0-day Robot <robot@bytheb.org> wrote:
>
> Bleep bloop.  Greetings William Tu, I am a robot and I have tried out your patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> WARNING: Line lacks whitespace around operator
> #35 FILE: lib/netdev-afxdp.c:561:
>     numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node",
>
Regards,
William


> Lines checked: 67, Warnings: 1, Errors: 0
>
>
> Please check this out.  If you feel there has been an error, please email aconole@redhat.com
>
> Thanks,
> 0-day Robot
Aaron Conole Sept. 13, 2019, 6:40 p.m. UTC | #3
William Tu <u9012063@gmail.com> writes:

> Hi Aaron,
>
> I think this might be a false positive?

Looks like it.

> I didn't find any lack whitespace here....
>
> On Fri, Sep 13, 2019 at 11:01 AM 0-day Robot <robot@bytheb.org> wrote:
>>
>> Bleep bloop.  Greetings William Tu, I am a robot and I have tried out your patch.
>> Thanks for your contribution.
>>
>> I encountered some error that I wasn't expecting.  See the details below.
>>
>>
>> checkpatch:
>> WARNING: Line lacks whitespace around operator
>> #35 FILE: lib/netdev-afxdp.c:561:
>>     numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node",

Possibly something with the '/' inside the '"'

If I get some time, I'll test it out.

> Regards,
> William
>
>
>> Lines checked: 67, Warnings: 1, Errors: 0
>>
>>
>> Please check this out.  If you feel there has been an error, please email aconole@redhat.com
>>
>> Thanks,
>> 0-day Robot
William Tu Sept. 13, 2019, 9:11 p.m. UTC | #4
On Fri, Sep 13, 2019 at 11:40 AM Aaron Conole <aconole@redhat.com> wrote:
>
> William Tu <u9012063@gmail.com> writes:
>
> > Hi Aaron,
> >
> > I think this might be a false positive?
>
> Looks like it.

Hi Aaron,

I looked at the checkpatch.py.
Adding a space in front of /sys makes it PASSED, but I couldn't figure it out
a solution to fix, ex:
+    numa_node_path = xasprintf(" /sys/class/net/%s/device/numa_node",

Will pass.
William
Eelco Chaudron Sept. 26, 2019, 12:18 p.m. UTC | #5
On 13 Sep 2019, at 19:15, 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.

See comments below, but I think the main problem that should be fixed is 
allocating the memory pool on the correct numa.
So replacing the xmalloc_pagealign() in xsk_configure() with something 
that is numa aware like numa_alloc_onnode().
numa_alloc_onnode() will require libnuma.

Also, you might need to update the “Limitations/Known Issues” 
section in the afxdp.rst file.

> Signed-off-by: William Tu <u9012063@gmail.com>
> ---
>  lib/netdev-afxdp.c | 38 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
> index e5b058d08a09..a94d77468cb7 100644
> --- a/lib/netdev-afxdp.c
> +++ b/lib/netdev-afxdp.c
> @@ -552,11 +552,39 @@ 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));

numa_node_path gets allocated by xasprintf() but it’s never free’ed.

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

Should this be a INFO? Nothing we can do about it for virtual 
interfaces.

> +        return 0;
> +    }
> +
> +    n = fread(buffer, 1, sizeof buffer, stream);
> +    if (!n) {
> +        goto error;
> +    }
> +
> +    node_id = strtol(buffer, NULL, 10);
> +    if (node_id < 0 || node_id > 2) {
> +        goto error;
> +    }
> +
> +    fclose(stream);
> +    return (int)node_id;
> +
> +error:
> +    VLOG_WARN_RL(&rl, "Error detecting numa node of %s, use numa_id 
> 0",
> +                 numa_node_path);
> +    fclose(stream);
>      return 0;
>  }
>
> -- 
> 2.7.4
William Tu Sept. 26, 2019, 6:18 p.m. UTC | #6
On Thu, Sep 26, 2019 at 02:18:15PM +0200, Eelco Chaudron wrote:
> 
> 
> On 13 Sep 2019, at 19:15, 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.
> 
> See comments below, but I think the main problem that should be fixed is
> allocating the memory pool on the correct numa.

good point.

> So replacing the xmalloc_pagealign() in xsk_configure() with something that
> is numa aware like numa_alloc_onnode().
> numa_alloc_onnode() will require libnuma.

OK! I will send v2 for this netdev_afxdp_get_numa_id() patch.
And a separate patch for using libnuma and numa_alloc_onnode()

>
> Also, you might need to update the “Limitations/Known Issues” section in the
> afxdp.rst file.

yes, thanks.

> 
> >Signed-off-by: William Tu <u9012063@gmail.com>
> >---
> > lib/netdev-afxdp.c | 38 +++++++++++++++++++++++++++++++++-----
> > 1 file changed, 33 insertions(+), 5 deletions(-)
> >
> >diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
> >index e5b058d08a09..a94d77468cb7 100644
> >--- a/lib/netdev-afxdp.c
> >+++ b/lib/netdev-afxdp.c
> >@@ -552,11 +552,39 @@ 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));
> 
> numa_node_path gets allocated by xasprintf() but it’s never free’ed.

yes, thanks!

> 
> >+    stream = fopen(numa_node_path, "r");
> >+    if (!stream) {
> >+        /* Virtual device does not have this info. */
> >+        VLOG_WARN_RL(&rl, "Open %s failed: %s, use numa_id 0",
> >+                     numa_node_path, ovs_strerror(errno));
> 
> Should this be a INFO? Nothing we can do about it for virtual interfaces.

agree, INFO is more appropriate.

William

> 
> >+        return 0;
> >+    }
> >+
> >+    n = fread(buffer, 1, sizeof buffer, stream);
> >+    if (!n) {
> >+        goto error;
> >+    }
> >+
> >+    node_id = strtol(buffer, NULL, 10);
> >+    if (node_id < 0 || node_id > 2) {
> >+        goto error;
> >+    }
> >+
> >+    fclose(stream);
> >+    return (int)node_id;
> >+
> >+error:
> >+    VLOG_WARN_RL(&rl, "Error detecting numa node of %s, use numa_id 0",
> >+                 numa_node_path);
> >+    fclose(stream);
> >     return 0;
> > }
> >
> >-- 
> >2.7.4

Patch
diff mbox series

diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index e5b058d08a09..a94d77468cb7 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -552,11 +552,39 @@  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));
+    stream = fopen(numa_node_path, "r");
+    if (!stream) {
+        /* Virtual device does not have this info. */
+        VLOG_WARN_RL(&rl, "Open %s failed: %s, use numa_id 0",
+                     numa_node_path, ovs_strerror(errno));
+        return 0;
+    }
+
+    n = fread(buffer, 1, sizeof buffer, stream);
+    if (!n) {
+        goto error;
+    }
+
+    node_id = strtol(buffer, NULL, 10);
+    if (node_id < 0 || node_id > 2) {
+        goto error;
+    }
+
+    fclose(stream);
+    return (int)node_id;
+
+error:
+    VLOG_WARN_RL(&rl, "Error detecting numa node of %s, use numa_id 0",
+                 numa_node_path);
+    fclose(stream);
     return 0;
 }