| Message ID | 1568394931-82238-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] netdev-afxdp: Detect numa node id. | expand |
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
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
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
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
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
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
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; }
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(-)