subject:"\[ovs\-dev\] \[PATCHv4\] netdev\-linux\: Detect numa node id."

Re: [ovs-dev] [PATCHv4] netdev-linux: Detect numa node id.

2019-10-23 Thread William Tu

On Mon, Oct 21, 2019 at 03:18:51PM +0200, Ilya Maximets wrote:
> On 17.10.2019 22:08, 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.  Currently only the afxdp netdev type uses it,
> >other linux netdev types are disabled due to no use case.
> >
> >Signed-off-by: William Tu 
> >---
> >v4:
> >   Feedbacks from Eelco
> >   - Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/599308893
> >
> >v3:
> >   Feedbacks from Ilya and Eelco
> >   - update doc, afxdp.rst
> >   - fix coding style
> >   - fix limit of numa node max, by using ovs_numa_numa_id_is_valid
> >   - move the function to netdev-linux
> >   - cache the result of numa_id
> >   - add security check for netdev name
> >   - use fscanf instead of read and convert to int
> >   - revise some error message content
> >
> >v2:
> >   address feedback from Eelco
> > fix memory leak of xaspintf
> > log using INFO instead of WARN
> >---
> >  Documentation/intro/install/afxdp.rst |  1 -
> >  lib/netdev-afxdp.c| 11 ---
> >  lib/netdev-linux-private.h|  2 ++
> >  lib/netdev-linux.c| 58 
> > ++-
> >  4 files changed, 59 insertions(+), 13 deletions(-)
> >
> >diff --git a/Documentation/intro/install/afxdp.rst 
> >b/Documentation/intro/install/afxdp.rst
> >index 820e9d993d8f..6c00c4ad1356 100644
> >--- a/Documentation/intro/install/afxdp.rst
> >+++ b/Documentation/intro/install/afxdp.rst
> >@@ -309,7 +309,6 @@ Below is a script using namespaces and veth peer::
> >  Limitations/Known Issues
> >  
> >-#. Device's numa ID is always 0, need a way to find numa id from a netdev.
> >  #. No QoS support because AF_XDP netdev by-pass the Linux TC layer. A 
> > possible
> > work-around is to use OpenFlow meter action.
> >  #. Most of the tests are done using i40e single port. Multiple ports and
> >diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
> >index 8eb270c150e8..cfd93fab9f45 100644
> >--- a/lib/netdev-afxdp.c
> >+++ b/lib/netdev-afxdp.c
> >@@ -543,17 +543,6 @@ out:
> >  return err;
> >  }
> >-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));
> >-return 0;
> >-}
> >-
> >  static void
> >  xsk_remove_xdp_program(uint32_t ifindex, int xdpmode)
> >  {
> >diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
> >index a350be151147..c8f2be47b10b 100644
> >--- a/lib/netdev-linux-private.h
> >+++ b/lib/netdev-linux-private.h
> >@@ -96,6 +96,8 @@ struct netdev_linux {
> >  /* LAG information. */
> >  bool is_lag_master; /* True if the netdev is a LAG master. */
> >+int numa_id;/* NUMA node id. */
> >+
> >  #ifdef HAVE_AF_XDP
> >  /* AF_XDP information. */
> >  struct xsk_socket_info **xsks;
> >diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> >index f4819237370a..add148d83eb7 100644
> >--- a/lib/netdev-linux.c
> >+++ b/lib/netdev-linux.c
> >@@ -236,6 +236,7 @@ enum {
> >  VALID_VPORT_STAT_ERROR  = 1 << 5,
> >  VALID_DRVINFO   = 1 << 6,
> >  VALID_FEATURES  = 1 << 7,
> >+VALID_NUMA_ID   = 1 << 8,
> >  };
> >  
> >  struct linux_lag_slave {
> >@@ -1391,6 +1392,61 @@ netdev_linux_tap_batch_send(struct netdev *netdev_,
> >  return 0;
> >  }
> >+static int OVS_UNUSED
> >+netdev_linux_get_numa_id(const struct netdev *netdev_)
> >+{
> >+struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> >+char *numa_node_path;
> >+const char *name;
> >+int node_id;
> >+FILE *stream;
> >+
> 
> 'netdev' fields should be protected by netdev->mutex.  So, this
> function should take it before accessing 'numa_id' or 'cache_valid'.
> 

Thanks, I will add lock here.

> >+if (netdev->cache_valid & VALID_NUMA_ID) {
> >+return netdev->numa_id;
> >+}
> >+
> >+netdev->numa_id = 0;
> >+netdev->cache_valid |= VALID_NUMA_ID;
> >+
> >+name = netdev_get_name(netdev_);
> >+if (strpbrk(name, "/\\")) {
> >+VLOG_ERR_RL(, "\"%s\" is not a valid name for a port. "
> >+"A valid name must not include '/' or '\\'."
> >+"Using numa_id 0", name);
> >+return 0;
> >+}
> >+
> >+numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node", name);
> >+
> >+stream = fopen(numa_node_path, "r");
> >+if (!stream) {
> >+/* Virtual device does not have this info. */
> >+VLOG_INFO_RL(, "%s: Can't open '%s': %s, using numa_id 0",
> >+ name, numa_node_path, ovs_strerror(errno));
> >+free(numa_node_path);
> >+return 0;
> >+

Re: [ovs-dev] [PATCHv4] netdev-linux: Detect numa node id.

2019-10-21 Thread Ilya Maximets


On 17.10.2019 22:08, 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.  Currently only the afxdp netdev type uses it,
other linux netdev types are disabled due to no use case.

Signed-off-by: William Tu 
---
v4:
   Feedbacks from Eelco
   - Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/599308893

v3:
   Feedbacks from Ilya and Eelco
   - update doc, afxdp.rst
   - fix coding style
   - fix limit of numa node max, by using ovs_numa_numa_id_is_valid
   - move the function to netdev-linux
   - cache the result of numa_id
   - add security check for netdev name
   - use fscanf instead of read and convert to int
   - revise some error message content

v2:
   address feedback from Eelco
fix memory leak of xaspintf
log using INFO instead of WARN
---
  Documentation/intro/install/afxdp.rst |  1 -
  lib/netdev-afxdp.c| 11 ---
  lib/netdev-linux-private.h|  2 ++
  lib/netdev-linux.c| 58 ++-
  4 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/Documentation/intro/install/afxdp.rst 
b/Documentation/intro/install/afxdp.rst
index 820e9d993d8f..6c00c4ad1356 100644
--- a/Documentation/intro/install/afxdp.rst
+++ b/Documentation/intro/install/afxdp.rst
@@ -309,7 +309,6 @@ Below is a script using namespaces and veth peer::
  
  Limitations/Known Issues

  
-#. Device's numa ID is always 0, need a way to find numa id from a netdev.
  #. No QoS support because AF_XDP netdev by-pass the Linux TC layer. A possible
 work-around is to use OpenFlow meter action.
  #. Most of the tests are done using i40e single port. Multiple ports and
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 8eb270c150e8..cfd93fab9f45 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -543,17 +543,6 @@ out:
  return err;
  }
  
-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));
-return 0;
-}
-
  static void
  xsk_remove_xdp_program(uint32_t ifindex, int xdpmode)
  {
diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
index a350be151147..c8f2be47b10b 100644
--- a/lib/netdev-linux-private.h
+++ b/lib/netdev-linux-private.h
@@ -96,6 +96,8 @@ struct netdev_linux {
  /* LAG information. */
  bool is_lag_master; /* True if the netdev is a LAG master. */
  
+int numa_id;/* NUMA node id. */

+
  #ifdef HAVE_AF_XDP
  /* AF_XDP information. */
  struct xsk_socket_info **xsks;
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index f4819237370a..add148d83eb7 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -236,6 +236,7 @@ enum {
  VALID_VPORT_STAT_ERROR  = 1 << 5,
  VALID_DRVINFO   = 1 << 6,
  VALID_FEATURES  = 1 << 7,
+VALID_NUMA_ID   = 1 << 8,
  };
  
  struct linux_lag_slave {
@@ -1391,6 +1392,61 @@ netdev_linux_tap_batch_send(struct netdev *netdev_,
  return 0;
  }
  
+static int OVS_UNUSED

+netdev_linux_get_numa_id(const struct netdev *netdev_)
+{
+struct netdev_linux *netdev = netdev_linux_cast(netdev_);
+char *numa_node_path;
+const char *name;
+int node_id;
+FILE *stream;
+


'netdev' fields should be protected by netdev->mutex.  So, this
function should take it before accessing 'numa_id' or 'cache_valid'.


+if (netdev->cache_valid & VALID_NUMA_ID) {
+return netdev->numa_id;
+}
+
+netdev->numa_id = 0;
+netdev->cache_valid |= VALID_NUMA_ID;
+
+name = netdev_get_name(netdev_);
+if (strpbrk(name, "/\\")) {
+VLOG_ERR_RL(, "\"%s\" is not a valid name for a port. "
+"A valid name must not include '/' or '\\'."
+"Using numa_id 0", name);
+return 0;
+}
+
+numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node", name);
+
+stream = fopen(numa_node_path, "r");
+if (!stream) {
+/* Virtual device does not have this info. */
+VLOG_INFO_RL(, "%s: Can't open '%s': %s, using numa_id 0",
+ name, numa_node_path, ovs_strerror(errno));
+free(numa_node_path);
+return 0;
+}
+
+if (fscanf(stream, "%d", _id) != 1) {
+goto error;
+};


Redundant ';'.


+
+if (!ovs_numa_numa_id_is_valid(node_id)) {
+goto error;
+}


Maybe it will look better if above 2 'if's will be combined like this:

if (fscanf(stream, "%d", _id) != 1
|| !ovs_numa_numa_id_is_valid(node_id) {
goto error;
}

What do you think?


+
+netdev->numa_id = node_id;
+fclose(stream);
+free(numa_node_path);
+return node_id;
+

Re: [ovs-dev] [PATCHv4] netdev-linux: Detect numa node id.

2019-10-17 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
#108 FILE: lib/netdev-linux.c:1419:
numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node", name);

Lines checked: 153, 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] [PATCHv4] netdev-linux: Detect numa node id.

2019-10-17 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.  Currently only the afxdp netdev type uses it,
other linux netdev types are disabled due to no use case.

Signed-off-by: William Tu 
---
v4:
  Feedbacks from Eelco
  - Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/599308893

v3:
  Feedbacks from Ilya and Eelco
  - update doc, afxdp.rst
  - fix coding style
  - fix limit of numa node max, by using ovs_numa_numa_id_is_valid
  - move the function to netdev-linux
  - cache the result of numa_id
  - add security check for netdev name
  - use fscanf instead of read and convert to int
  - revise some error message content

v2:
  address feedback from Eelco
fix memory leak of xaspintf
log using INFO instead of WARN
---
 Documentation/intro/install/afxdp.rst |  1 -
 lib/netdev-afxdp.c| 11 ---
 lib/netdev-linux-private.h|  2 ++
 lib/netdev-linux.c| 58 ++-
 4 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/Documentation/intro/install/afxdp.rst 
b/Documentation/intro/install/afxdp.rst
index 820e9d993d8f..6c00c4ad1356 100644
--- a/Documentation/intro/install/afxdp.rst
+++ b/Documentation/intro/install/afxdp.rst
@@ -309,7 +309,6 @@ Below is a script using namespaces and veth peer::
 
 Limitations/Known Issues
 
-#. Device's numa ID is always 0, need a way to find numa id from a netdev.
 #. No QoS support because AF_XDP netdev by-pass the Linux TC layer. A possible
work-around is to use OpenFlow meter action.
 #. Most of the tests are done using i40e single port. Multiple ports and
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 8eb270c150e8..cfd93fab9f45 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -543,17 +543,6 @@ out:
 return err;
 }
 
-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));
-return 0;
-}
-
 static void
 xsk_remove_xdp_program(uint32_t ifindex, int xdpmode)
 {
diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
index a350be151147..c8f2be47b10b 100644
--- a/lib/netdev-linux-private.h
+++ b/lib/netdev-linux-private.h
@@ -96,6 +96,8 @@ struct netdev_linux {
 /* LAG information. */
 bool is_lag_master; /* True if the netdev is a LAG master. */
 
+int numa_id;/* NUMA node id. */
+
 #ifdef HAVE_AF_XDP
 /* AF_XDP information. */
 struct xsk_socket_info **xsks;
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index f4819237370a..add148d83eb7 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -236,6 +236,7 @@ enum {
 VALID_VPORT_STAT_ERROR  = 1 << 5,
 VALID_DRVINFO   = 1 << 6,
 VALID_FEATURES  = 1 << 7,
+VALID_NUMA_ID   = 1 << 8,
 };
 
 struct linux_lag_slave {
@@ -1391,6 +1392,61 @@ netdev_linux_tap_batch_send(struct netdev *netdev_,
 return 0;
 }
 
+static int OVS_UNUSED
+netdev_linux_get_numa_id(const struct netdev *netdev_)
+{
+struct netdev_linux *netdev = netdev_linux_cast(netdev_);
+char *numa_node_path;
+const char *name;
+int node_id;
+FILE *stream;
+
+if (netdev->cache_valid & VALID_NUMA_ID) {
+return netdev->numa_id;
+}
+
+netdev->numa_id = 0;
+netdev->cache_valid |= VALID_NUMA_ID;
+
+name = netdev_get_name(netdev_);
+if (strpbrk(name, "/\\")) {
+VLOG_ERR_RL(, "\"%s\" is not a valid name for a port. "
+"A valid name must not include '/' or '\\'."
+"Using numa_id 0", name);
+return 0;
+}
+
+numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node", name);
+
+stream = fopen(numa_node_path, "r");
+if (!stream) {
+/* Virtual device does not have this info. */
+VLOG_INFO_RL(, "%s: Can't open '%s': %s, using numa_id 0",
+ name, numa_node_path, ovs_strerror(errno));
+free(numa_node_path);
+return 0;
+}
+
+if (fscanf(stream, "%d", _id) != 1) {
+goto error;
+};
+
+if (!ovs_numa_numa_id_is_valid(node_id)) {
+goto error;
+}
+
+netdev->numa_id = node_id;
+fclose(stream);
+free(numa_node_path);
+return node_id;
+
+error:
+VLOG_WARN_RL(, "%s: Can't detect NUMA node, using numa_id 0", name);
+free(numa_node_path);
+fclose(stream);
+return 0;
+}
+
 /* Sends 'batch' on 'netdev'.  Returns 0 if successful, otherwise a positive
  * errno value.  Returns EAGAIN without blocking if the packet cannot be queued
  * immediately.  Returns EMSGSIZE if a partial packet was transmitted or if
@@ -3298,7 +3354,7 @@ const struct netdev_class

Re: [ovs-dev] [PATCHv4] netdev-linux: Detect numa node id.

Re: [ovs-dev] [PATCHv4] netdev-linux: Detect numa node id.

Re: [ovs-dev] [PATCHv4] netdev-linux: Detect numa node id.

[ovs-dev] [PATCHv4] netdev-linux: Detect numa node id.

4 matches

Site Navigation

Mail list logo

Footer information