On Wed, Dec 18, 2019 at 8:41 AM Ilya Maximets <[email protected]> wrote: > > On 17.12.2019 23:27, Yi-Hung Wei wrote: > > Currently, the AF_XDP socket (XSK) related memory are allocated by main > > thread in the main thread's NUMA domain. With the patch that detects > > netdev-linux's NUMA node id, the PMD thread of AF_XDP port will be run on > > the AF_XDP netdev's NUMA domain. If the net device's NUMA domain > > is different from the main thread's NUMA domain, we will have two > > cross-NUMA memory accesses (netdev <-> memory, memory <-> CPU). > > > > This patch addresses the aforementioned issue by allocating > > the memory in the net device's NUMA domain. > > > > Signed-off-by: Yi-Hung Wei <[email protected]> > > --- > > diff --git a/acinclude.m4 b/acinclude.m4 > > index 542637ac8cb8..73ed11d701aa 100644 > > --- a/acinclude.m4 > > +++ b/acinclude.m4 > > @@ -286,9 +286,12 @@ AC_DEFUN([OVS_CHECK_LINUX_AF_XDP], [ > > AC_CHECK_FUNCS([pthread_spin_lock], [], > > [AC_MSG_ERROR([unable to find pthread_spin_lock for AF_XDP > > support])]) > > > > + AC_CHECK_LIB(numa, numa_alloc_onnode, [], > > Please, embrace all arguments with brackets. > And you may actually use OVS_FIND_DEPENDENCY() instead.
Thanks Ilya for review. Yes, it is much cleaner to use OVS_FIND_DEPENDENCY(). > > diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c > > index 4c1f9c68270a..2d6f739b4b67 100644 > > --- a/lib/netdev-afxdp.c > > +++ b/lib/netdev-afxdp.c > > @@ -477,6 +478,14 @@ xsk_configure_all(struct netdev *netdev) > > ovs_assert(dev->tx_locks == NULL); > > > > n_rxq = netdev_n_rxq(netdev); > > + > > + /* Allocate all the xsk related memory in the netdev's NUMA domain. */ > > + struct bitmask *old_bm = numa_get_membind(); > > + struct bitmask *new_bm = numa_allocate_nodemask(); > > + netdev_get_numa_id(netdev); > > Does it work? > We're holding dev->mutex here and netdev_get_numa_id() will try to > take it too and should fail. Am I missing something? You're right. I moved the numa related logic from xsk_configure_all() to netdev_afxdp_reconfigure() to avoid the locking issue in the next revision. Thanks! > > + numa_bitmask_setbit(new_bm, dev->numa_id); > > As Eelco said, it's better to use the actual result of get_numa_id() > instead of relying on cache population. > BTW, it might be good to handle NETDEV_NUMA_UNSPEC for that case even > if we can't have it right now. Sounds good. > > + numa_set_membind(new_bm); > > It might be better to use 'preferred' policy instead to avoid > assertion if we're out of memory on a certain node. Sure, I will use numa_set_preferred() in the next version. Thanks, -Yi-Hung _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
