Message ID | 1569605185-10548-1-git-send-email-u9012063@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,PATCHv2] 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: 70, 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 27 Sep 2019, at 19: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@gmail.com> Will you update the “Limitations/Known Issues” section as part of the libnuma and numa_alloc_onnode() patch? > --- > 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)); > + 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)); > + free(numa_node_path); > + 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; > + } Any reason why you limit this to 2? Guess it depends on the kernel’s CONFIG_NODES_SHIFT value. You might be able to use the libnuma API numa_max_possible_node(). > + > + fclose(stream); > + free(numa_node_path); > + return (int)node_id; Guess you need a space after (int) node_id. > + > +error: > + VLOG_WARN_RL(&rl, "Error detecting numa node of %s, use numa_id > 0", > + numa_node_path); > + free(numa_node_path); > + fclose(stream); > return 0; > } > > -- > 2.7.4
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@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; > } > >
On Mon, Sep 30, 2019 at 12:40:59PM +0200, Eelco Chaudron wrote: > > > On 27 Sep 2019, at 19: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@gmail.com> > > Will you update the “Limitations/Known Issues” section as part of the > libnuma and numa_alloc_onnode() patch? Thanks! I will fix it in next version. > > >--- > >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)); > >+ 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)); > >+ free(numa_node_path); > >+ 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; > >+ } > > Any reason why you limit this to 2? Guess it depends on the kernel’s > CONFIG_NODES_SHIFT value. You might be able to use the libnuma API > numa_max_possible_node(). > I thought most of the systems have max 2 numa... I will fix it, thank you. Regards, William > >+ > >+ fclose(stream); > >+ free(numa_node_path); > >+ return (int)node_id; > > Guess you need a space after (int) node_id. > > >+ > >+error: > >+ VLOG_WARN_RL(&rl, "Error detecting numa node of %s, use numa_id 0", > >+ numa_node_path); > >+ free(numa_node_path); > >+ fclose(stream); > > return 0; > > } > > > >-- > >2.7.4
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@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
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)); + 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)); + free(numa_node_path); + 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); + 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); + free(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> --- 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(-)