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

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

2019-10-01 Thread William Tu

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//device/numa_node'.
> >If not available, ex: virtual device, or any error happens,
> >return numa id 0.
> >
> >Signed-off-by: William Tu 
> >---
> >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(, "Open %s failed: %s, use numa_id 0",
> >+ numa_node_path, ovs_strerror(errno));
> 
> How about:
> 
> VLOG_INFO_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

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2019-10-01 Thread William Tu

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//device/numa_node'.
> >If not available, ex: virtual device, or any error happens,
> >return numa id 0.
> >
> >Signed-off-by: William Tu 
> 
> 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(, "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(, "Error detecting numa node of %s, use numa_id 0",
> >+ numa_node_path);
> >+free(numa_node_path);
> >+fclose(stream);
> > return 0;
> > }
> >
> >-- 
> >2.7.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2019-09-30 Thread 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//device/numa_node'.
If not available, ex: virtual device, or any error happens,
return numa id 0.

Signed-off-by: William Tu 
---
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(, "Open %s failed: %s, use numa_id 0",
+ numa_node_path, ovs_strerror(errno));


How about:

VLOG_INFO_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(, "Error detecting numa node of %s, use numa_id 0",
+ numa_node_path);


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


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


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2019-09-30 Thread Eelco Chaudron




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//device/numa_node'.
If not available, ex: virtual device, or any error happens,
return numa id 0.

Signed-off-by: William Tu 


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(, "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(, "Error detecting numa node of %s, use numa_id 
0",

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

--
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2019-09-27 Thread 0-day Robot

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 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2019-09-27 Thread William Tu

The patch detects the numa node id from the name of the netdev,
by reading the '/sys/class/net//device/numa_node'.
If not available, ex: virtual device, or any error happens,
return numa id 0.

Signed-off-by: William Tu 
---
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(, "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(, "Error detecting numa node of %s, use numa_id 0",
+ numa_node_path);
+free(numa_node_path);
+fclose(stream);
 return 0;
 }
 
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

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

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

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

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

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

6 matches

Site Navigation

Mail list logo

Footer information