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 <[email protected]>
---
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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
