Re: [dpdk-dev] [PATCH v9 1/1] net/af_xdp: introduce AF XDP PMD driver

[Luca Boccassi <bluca@debian.org>]

On Wed, 2019-04-03 at 14:09 +0100, Ferruh Yigit wrote:
> On 4/3/2019 12:35 PM, Luca Boccassi wrote:
> > On Wed, 2019-04-03 at 12:18 +0100, Ferruh Yigit wrote:
> > > On 4/3/2019 11:42 AM, Luca Boccassi wrote:
> > > > On Wed, 2019-04-03 at 11:36 +0100, Luca Boccassi wrote:
> > > > > On Wed, 2019-04-03 at 17:59 +0800, Ye Xiaolong wrote:
> > > > > > Hi, Luca
> > > > > > 
> > > > > > On 04/02, Luca Boccassi wrote:
> > > > > > > On Tue, 2019-04-02 at 23:46 +0800, Xiaolong Ye wrote:
> > > > > > > > diff --git a/drivers/net/af_xdp/Makefile
> > > > > > > > b/drivers/net/af_xdp/Makefile
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000..8343e3016
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/net/af_xdp/Makefile
> > > > > > > > @@ -0,0 +1,32 @@
> > > > > > > > +# SPDX-License-Identifier: BSD-3-Clause
> > > > > > > > +# Copyright(c) 2019 Intel Corporation
> > > > > > > > +
> > > > > > > > +include $(RTE_SDK)/mk/rte.vars.mk
> > > > > > > > +
> > > > > > > > +#
> > > > > > > > +# library name
> > > > > > > > +#
> > > > > > > > +LIB = librte_pmd_af_xdp.a
> > > > > > > > +
> > > > > > > > +EXPORT_MAP := rte_pmd_af_xdp_version.map
> > > > > > > > +
> > > > > > > > +LIBABIVER := 1
> > > > > > > > +
> > > > > > > > +CFLAGS += -O3
> > > > > > > > +
> > > > > > > > +# require kernel version >= v5.1-rc1
> > > > > > > > +CFLAGS += -I$(RTE_KERNELDIR)/tools/include
> > > > > > > > +CFLAGS += -I$(RTE_KERNELDIR)/tools/lib/bpf
> > > > > > > 
> > > > > > > Sorry for not noticing this before, but doesn't this
> > > > > > > require
> > > > > > > the
> > > > > > > full
> > > > > > > kernel tree rather than just the typical headers package?
> > > > > > > Requiring
> > > > > > > the
> > > > > > > full kernel tree to be available at build time will make
> > > > > > > this
> > > > > > > unbuildable on distros that still use makefiles, like
> > > > > > > RHEL
> > > > > > > and
> > > > > > > SUSE. At
> > > > > > > least on Debian and Ubuntu, the kernel headers packages
> > > > > > > distributed
> > > > > > > do
> > > > > > > not include the full kernel tree, only the headers, so
> > > > > > > there's no
> > > > > > > tools/lib or tools/include.
> > > > > > 
> > > > > > Currently we do have dependencies on the kernel src tree,
> > > > > > as
> > > > > > xsk.h
> > > > > > and
> > > > > > asm/barrier wouldn't be installed by libbpf, so before
> > > > > > libbpf
> > > > > > handles
> > > > > > these
> > > > > > properly, can we keep the current RTE_KERNELDIR in Makefile
> > > > > > for
> > > > > > now,
> > > > > > and mention
> > > > > > the dependencies in document, also suggest users to config
> > > > > > RTE_KERNELDIR to correct
> > > > > > kernel src tree if they want to use af_xdp pmd?
> > > > > > 
> > > > > > Something like:
> > > > > > 
> > > > > > dependencies:
> > > > > > - kernel source code (>= v5.1-rc1)
> > > > > > - build libbfp and install
> > > > > > 
> > > > > > Thanks,
> > > > > > Xiaolong
> > > > > 
> > > > > asm/barrier.h is installed by the kernel headers packages so
> > > > > it
> > > > > would
> > > > > be fine (although not ideal) and not need the full source
> > > > > tree.
> > > > > xsk.h is a bit more worrying, as it looks like an internal
> > > > > header
> > > > > from
> > > > > here.
> > > > > 
> > > > > Is it really necessary for external applications to use an
> > > > > internal-
> > > > > only header and a kernel header to be able to use libbpf?
> > > > 
> > > > Actually, xsk.h is now installed by the library makefile:
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=379e2014c95b
> > > > 
> > > > 
> > > 
> > > Good to have this one. But again it is in BPF tree and it won't
> > > be in
> > > 5.1.
> > 
> > It looks like a small and required bug fix to me, and 5.1 is still
> > in
> > RC state, so perhaps there's still time.
> > 
> > Bjorn and Magnus, any chance the above makefile install fix could
> > be
> > sent for inclusion in 5.1-rc4?
> > 
> > > I suggested changing code as following for now, it would help to
> > > keep
> > > changes
> > > small when above patch merged into kernel:
> > >  CFLAGS += -I$(RTE_KERNELDIR)/tools/lib [in makefile]
> > >  #include <bpf/xsk.h>                   [in .c file]
> > > 
> > > > So the full kernel source tree is no longer required.
> > > > 
> > > > Is asm/barrier.h really required? Isn't there an userspace
> > > > alternative?
> > > 
> > > The 'asm/barrier.h' in the kernel headers and the
> > > 'tools/include/asm/barrier.h'
> > > looks different, the one in the kernel source has dependency to
> > > other
> > > kernel
> > > headers.
> > > 
> > > I wonder same thing, what is used from
> > > 'tools/include/asm/barrier.h'
> > > and if it
> > > can be avoided.
> 
> It seems, 'tools/include/asm/barrier.h' is required for 'smp_wmb()' &
> 'smp_rmb()' in 'xsk.h'.
> We have equivalents of these in DPDK [1], and perhaps it can be
> possible to use
> them and not include this header at all.
> 
> in 'rte_eth_af_xdp.c', before including 'xsk.h', we can include an
> local
> compatibility header which does following should work:
> #define smp_rmb() rte_rmb()
> #define smp_wmb() rte_wmb()
> 
> @Xiaolong, what do you think?
> 
> [1]
> https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/include/arch/x86/rte_atomic.h?h=v19.02#n30

Perfect, that looks like a great solution for the PMD.

For the broader picture, now that xsk.h is a public userspace header,
it should at some point in the future be fixed to avoid depending on an
internal kernel definition for the barriers, and either ship their own
or depend on another public header that provides them. Otherwise every
application that wants to use bpf with xdp needs to provide their own
implementation - but this is not relanted to this patchset and we can
live without for the moment in DPDK.

> > The one in tools/include also is GPL-2.0 only so it cannot be
> > included
> > from the PMD, which is BSD-3-clause only (and it recursively
> > includes
> > the other arch-specific kernel headers)
> > 
> > > Anyway, as Xiaolong mentioned, following is working, can it work
> > > from
> > > a distro
> > > point of view:
> > > - get kernel source code (>= v5.1-rc1)
> > > - build libbfp and install
> > > - set 'RTE_KERNELDIR' to point kernel source path
> > > - build dpdk with af_xdp enabled
> > 
> > As long as the full kernel tree is required, we cannot enable it in
> > Debian and Ubuntu - we can't have it at build time on the build
> > workers, and also there's the licensing problem.
> 
> Got it.
> 
> In above steps, 'libbpf' also build from kernel source tree, will it
> be problem
> in you builds to not have it build from source?
> 
> If not, taking into account that xsk.h also will be fixed, only
> 'tools/include/asm/barrier.h' remains the problem, and it looks like
> it can be
> solved, please check above.

libbpf is already packaged separately in Debian and I think other
distros will follow soon, so it's all good for me once the barrier
issue is solved.

https://packages.debian.org/buster/libbpf-dev

From the makefile's perspective it should not matter where it comes
from - the headers should be expected to be in /usr/include and the
library in /usr/lib* - and pkg-config can help with that if available.
And if a user wants to use a custom path, then it's no different than
any of the other dependencies on other external libraries

-- 
Kind regards,
Luca Boccassi