Re: [ovs-dev] [PATCHv5] netdev-linux: Detect numa node id.
On Tue, Oct 29, 2019 at 9:15 AM Ilya Maximets wrote:
>
> On 23.10.2019 23: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
> > ---
> > v5:
> >Feedbacks from Ilya
> >- reafactor the error handling
> >- add mutex lock
> >- Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/601947245
> >
> > 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| 63
> > ++-
> > 4 files changed, 64 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..4dd05493b238 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,66 @@ netdev_linux_tap_batch_send(struct netdev *netdev_,
> > return 0;
> > }
> >
> > +static int
> > +netdev_linux_get_numa_id__(const struct netdev *netdev_)
>
> Since you've created a separate function, I think we need to add
> a thread safety annotation here: OVS_REQUIRES(netdev->mutex)
>
OK thanks!
> For this purpose, you'll better pass the instance of 'netdev_linux'
> in this function. Original netdev only used to get the netdev name,
> so it'll be not hard to use &netdev->up instead for that purpose.
>
OK
> Second thing is that we should preserve numa cache on netlink updates.
> Something like this:
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 4dd05493b..0495a035f 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -821,9 +821,9 @@ netdev_linux_update__(struct netdev_linux *dev,
> {
> if (rtnetlink_type_is_rtnlgrp_link(change->nlmsg_type)) {
> if (change->nlmsg_type == RTM_NEWLINK) {
> -/* Keep drv-info, and ip addresses. */
> +/* Keep drv-info, ip addresses and NUMA id. */
> netdev_linux_changed(dev, change->ifi_flags,
>
Re: [ovs-dev] [PATCHv5] netdev-linux: Detect numa node id.
On 23.10.2019 23: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
---
v5:
Feedbacks from Ilya
- reafactor the error handling
- add mutex lock
- Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/601947245
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| 63 ++-
4 files changed, 64 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..4dd05493b238 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,66 @@ netdev_linux_tap_batch_send(struct netdev *netdev_,
return 0;
}
+static int
+netdev_linux_get_numa_id__(const struct netdev *netdev_)
Since you've created a separate function, I think we need to add
a thread safety annotation here: OVS_REQUIRES(netdev->mutex)
For this purpose, you'll better pass the instance of 'netdev_linux'
in this function. Original netdev only used to get the netdev name,
so it'll be not hard to use &netdev->up instead for that purpose.
Second thing is that we should preserve numa cache on netlink updates.
Something like this:
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 4dd05493b..0495a035f 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -821,9 +821,9 @@ netdev_linux_update__(struct netdev_linux *dev,
{
if (rtnetlink_type_is_rtnlgrp_link(change->nlmsg_type)) {
if (change->nlmsg_type == RTM_NEWLINK) {
-/* Keep drv-info, and ip addresses. */
+/* Keep drv-info, ip addresses and NUMA id. */
netdev_linux_changed(dev, change->ifi_flags,
- VALID_DRVINFO | VALID_IN);
+ VALID_DRVINFO | VALID_IN | VALID_NUMA_ID);
/* Update netdev from rtnl-change msg. */
if (change->mtu) {
---
Without this change we're re-checking the numa id after each time device
flags/mtu/whatever updated.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCHv5] netdev-linux: Detect numa node id.
On 23 Oct 2019, at 23: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 LGTM, Eelco Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCHv5] netdev-linux: Detect numa node id.
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: 158, 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] [PATCHv5] netdev-linux: Detect numa node id.
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
---
v5:
Feedbacks from Ilya
- reafactor the error handling
- add mutex lock
- Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/601947245
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| 63 ++-
4 files changed, 64 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..4dd05493b238 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,66 @@ netdev_linux_tap_batch_send(struct netdev *netdev_,
return 0;
}
+static int
+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
+|| !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;
+}
+
+static int OVS_UNUSED
+netdev_linux_get_numa_id(const struct netdev *netdev_)
+{
+struct netdev_linux *netdev = netdev_linux_cast(netdev_);
+int numa_id;
+
+ovs_mutex_lock(&netdev->mutex);
+numa_id = netdev_linux
