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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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

> 
> 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.


> 
>>> Also, the license in asm/barrier.h is GPL-2.0 only. It is not a
>>> userspace header so it is not covered by the userspace exception,
>>> which
>>> means at the very least the af_xdp PMD shared object is also
>>> licensed
>>> under GPL-2.0 only, isn't it?
>>>
>>
>>