RE: [PATCH] net/af_xdp: make compatible with libbpf v0.8.0

["Loftus, Ciara" <ciara.loftus@intel.com>]

> 
> > >>>>
> > >>>> On 6/24/22 13:23, Ciara Loftus wrote:
> > >>>>> libbpf v0.8.0 deprecates the bpf_get_link_xdp_id and
> > >> bpf_set_link_xdp_fd
> > >>>>> functions. Use meson to detect if libbpf >= v0.7.0 is linked and if so,
> > use
> > >>>>> the recommended replacement functions bpf_xdp_query_id,
> > >>>> bpf_xdp_attach
> > >>>>> and bpf_xdp_detach which are available to use since libbpf v0.7.0.
> > >>>>>
> > >>>>> Also prevent linking with libbpf versions > v0.8.0.
> > >>>>>
> > >>>>> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > >>>>> ---
> > >>>>>     doc/guides/nics/af_xdp.rst          |  3 ++-
> > >>>>>     drivers/net/af_xdp/compat.h         | 36
> > >>>> ++++++++++++++++++++++++++++-
> > >>>>>     drivers/net/af_xdp/meson.build      |  7 ++----
> > >>>>>     drivers/net/af_xdp/rte_eth_af_xdp.c | 19 +++------------
> > >>>>>     4 files changed, 42 insertions(+), 23 deletions(-)
> > >>>>
> > >>>> Don't we need to mention these changes in release notes?
> > >>>>
> > >>>>>
> > >>>>> diff --git a/doc/guides/nics/af_xdp.rst b/doc/guides/nics/af_xdp.rst
> > >>>>> index 56681c8365..9edb48df67 100644
> > >>>>> --- a/doc/guides/nics/af_xdp.rst
> > >>>>> +++ b/doc/guides/nics/af_xdp.rst
> > >>>>> @@ -43,7 +43,8 @@ Prerequisites
> > >>>>>     This is a Linux-specific PMD, thus the following prerequisites
> apply:
> > >>>>>
> > >>>>>     *  A Linux Kernel (version > v4.18) with XDP sockets configuration
> > >> enabled;
> > >>>>> -*  Both libxdp >=v1.2.2 and libbpf libraries installed, or, libbpf
> > <=v0.6.0
> > >>>>> +*  Both libxdp >=v1.2.2 and libbpf <=v0.8.0 libraries installed, or,
> > libbpf
> > >>>>> +   <=v0.6.0.
> > >>>>>     *  If using libxdp, it requires an environment variable called
> > >>>>>        LIBXDP_OBJECT_PATH to be set to the location of where libxdp
> > >> placed its
> > >>>> bpf
> > >>>>>        object files. This is usually in /usr/local/lib/bpf or
> > /usr/local/lib64/bpf.
> > >>>>> diff --git a/drivers/net/af_xdp/compat.h
> > >> b/drivers/net/af_xdp/compat.h
> > >>>>> index 28ea64aeaa..8f4ac8b5ea 100644
> > >>>>> --- a/drivers/net/af_xdp/compat.h
> > >>>>> +++ b/drivers/net/af_xdp/compat.h
> > >>>>> @@ -60,7 +60,7 @@ tx_syscall_needed(struct xsk_ring_prod *q
> > >>>> __rte_unused)
> > >>>>>     }
> > >>>>>     #endif
> > >>>>>
> > >>>>> -#ifdef RTE_NET_AF_XDP_LIBBPF_OBJ_OPEN
> > >>>>> +#ifdef RTE_NET_AF_XDP_LIBBPF_V070
> > >>>>
> > >>>> Typically version-based checks are considered as bad. Isn't it
> > >>>> better use feature-based checks/defines?
> > >>>
> > >>> Hi Andrew,
> > >>>
> > >>> Thank you for the feedback. Is the feature-based checking something
> > that
> > >> we can push to the next release?
> > >>>
> > >>> We are already using the pkg-config version-check method for other
> > >> libraries/features in the meson.build file:
> > >>> * libxdp >= v1.2.2 # earliest compatible libxdp release
> > >>> * libbpf >= v0.7.0 # bpf_object__* functions
> > >>> * libbpf >= v0.2.0 # shared umem feature
> > >>>
> > >>> If we change to your suggested method I think we should change
> them
> > all
> > >> in one patch. IMO it's probably too close to the release to change them
> all
> > >> right now. What do you think?
> > >>>
> > >>> Thanks,
> > >>> Ciara
> > >>
> > >> Hi Ciara,
> > >>
> > >> yes, ideally we should avoid usage of version-based check everywhere,
> > >> but I don't think that it is critical to switch at once. We can use it
> > >> for new checks right now and rewrite old/existing checks a bit later in
> > >> the next release.
> > >>
> > >> Please, note that my notes are related to review notes from Thomas
> who
> > >> asked by file_library() method is removed. Yes, it is confusing and it
> > >> is better to avoid it. Usage of feature-based checks would allow to
> > >> preserve find_library() as well.
> > >
> > > Thank you for the explanation.
> > > In this case we want to check that the libbpf library is <=v0.8.0. At this
> > moment in time v0.8.0 is the latest version of libbpf so we cannot check for
> a
> > symbol that tells us the library is > v0.8.0. Can you think of a way to
> approach
> > this without using the pkg-config version check method?
> > >
> > > I've introduced this check to future-proof the PMD and ensure we only
> > ever link with versions of libbpf that we've validated to be compatible with
> > the PMD. When say v0.9.0 is released we can patch the PMD allowing for
> > libbpf <= v0.9.0 and make any necessary API changes as part of that patch.
> > This should hopefully help avoid the scenario Thomas encountered.
> >
> > Personally I'd consider such checks which limit version as a drawback.
> > I think checks on build should not be used to reject future versions.
> > Otherwise, introduction of any further even minor version would require
> > a patch to allow it. Documentation is the place for information about
> > validated versions. Build should not enforce it.
> 
> Got it. I'll submit a v2 which removes the version-limiting and reinstates the
> cc.find_library() method. I'll update the documentation to indicate only
> versions up to v0.8.0 are supported and add a note to the release notes.
> Although if it's too late in the release cycle we can postpone this patch until
> after, and simply patch the docs stating that only libbpf <=v0.7.0 is supported
> for now?
> 
> Next release we can move away from the pkg-config version-checking
> method which already exists for other features, and replace with the symbol
> checking method.

I've submitted an RFC for this feature: http://patches.dpdk.org/project/dpdk/list/?series=24043
I'm starting maternity leave next week so am not in a position to rework it in the near future, but if it is functionality that a community member finds useful perhaps they can pick it up in my absence.

Thanks,
Ciara

> 
> Thanks,
> Ciara