| Message ID | 1571342891-55993-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,PATCHv4] netdev-linux: 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
#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 aconole@redhat.com
Thanks,
0-day Robot
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@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'. > + 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; } So we'll not need to duplicate cleanup code. Best regards, Ilya Maximets.
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@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
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(&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; + }; + + 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(&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 netdev_afxdp_class = { .set_config = netdev_afxdp_set_config, .get_config = netdev_afxdp_get_config, .reconfigure = netdev_afxdp_reconfigure, - .get_numa_id = netdev_afxdp_get_numa_id, + .get_numa_id = netdev_linux_get_numa_id, .send = netdev_afxdp_batch_send, .rxq_construct = netdev_afxdp_rxq_construct, .rxq_destruct = netdev_afxdp_rxq_destruct,
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@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(-)