Hi, William.
Thanks for the patch.
Few general comments on the topic:
1. This function is not afxdp specific. Maybe it's worth to move
it to more generic netdev-linux?
2. netdev-linux caches most of things like mtu and ifindex.
Maybe we could cache numa_id too and not read it all the time
from the filesystem?
3. More comments inline.
Best regards, Ilya Maximets.
On 27.09.2019 20:26, 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.
Signed-off-by: William Tu <u9012...@gmail.com>
---
v2:
Address feedback from Eelco
fix memory leak of xaspintf
log using INFO instead of WARN
---
lib/netdev-afxdp.c | 41 ++++++++++++++++++++++++++++++++++++-----
1 file changed, 36 insertions(+), 5 deletions(-)
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 6e01803272aa..6ff1473461a6 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -552,11 +552,42 @@ out:
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));
+ const char *numa_node_path;
+ long int node_id;
+ char buffer[4];
+ FILE *stream;
+ int n;
+
+ numa_node_path = xasprintf("/sys/class/net/%s/device/numa_node",
+ netdev_get_name(netdev));
Do we need some escaping here like we have for vhost:
strchr(name, '/') || strchr(name, '\\')
For security reasons?
+ stream = fopen(numa_node_path, "r");
+ if (!stream) {
+ /* Virtual device does not have this info. */
+ VLOG_INFO_RL(&rl, "Open %s failed: %s, use numa_id 0",
+ numa_node_path, ovs_strerror(errno));
How about:
VLOG_INFO_RL(&rl, "%s: Can't open '%s': %s, using numa_id 0",
netdev_get_name(netdev), numa_node_path,
ovs_strerror(errno));
In general, I'd like if we could print only 'fopen' related error here
and report about 'numa_id 0' in the end of function in the last message.
+ free(numa_node_path);
+ return 0;
+ }
+
+ n = fread(buffer, 1, sizeof buffer, stream);
Why fread? It looks much easier to use fscanf instead.
And you'll not need to parse the resulted string in this case.
+ if (!n) {
+ goto error;
+ }
+
+ node_id = strtol(buffer, NULL, 10);
Anyway, please, use str_to_int() from lib/util.h instead.
+ if (node_id < 0 || node_id > 2) {
This check looks strange. Even on Intel platforms, systems with
more than 2 NUMA nodes are widely available.
You may use ovs_numa_numa_id_is_valid() instead.
+ goto error;
+ }
+
+ fclose(stream);
+ free(numa_node_path);
+ return (int)node_id;
+
+error:
+ VLOG_WARN_RL(&rl, "Error detecting numa node of %s, use numa_id 0",
+ numa_node_path);
VLOG_WARN_RL(&rl, "%s: Can't detect NUMA node, using numa_id 0",
netdev_get_name(netdev));
+ free(numa_node_path);
+ fclose(stream);
return 0;
}
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
