Hi Ilya, Thanks for your review.
On Mon, Sep 30, 2019 at 10:18:32PM +0300, Ilya Maximets wrote: > 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. > OK, let me see how to make it more generic in netdev-linux. > 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 <[email protected]> > >--- > >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? > Sure, thanks. > >+ 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. > OK. > >+ 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. > Thanks I will switch to use fscanf > >+ 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. > Good idea. Regards, William _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
