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

William Tu u9012063 at gmail.com
Wed Oct 23 17:33:20 UTC 2019


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/<devname>/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 <u9012063 at gmail.com>
> >---
> >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(&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(&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", &node_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", &node_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;
> >+
> >+error:
> >+    VLOG_WARN_RL(&rl, "%s: Can't detect NUMA node, using numa_id 0", name);
> >+    free(numa_node_path);
> >+    fclose(stream);
> >+    return 0;
> >+}
> >+
> 
> 
> Or even like this:
> 
>     if (fscanf(stream, "%d", &node_id) != 1
>         || !ovs_numa_numa_id_is_valid(node_id) {
>         VLOG_WARN_RL(&rl, "%s: Can't detect NUMA node, using numa_id 0", name);
>         node_id = 0;
>     }
> 
>     netdev->numa_id = node_id;
>     fclose(stream);
>     free(numa_node_path);
>     return node_id;
> }

That's good idea, much simpler code now.
I will send v5.

Regards,
William



More information about the dev mailing list