On 20 Dec 2022, at 14:58, Ilya Maximets wrote:
> On 12/20/22 14:01, Eelco Chaudron wrote:
>>
>>
>> On 19 Dec 2022, at 13:20, Ilya Maximets wrote:
>>
>>> AF_XDP functions was deprecated in libbpf 0.7 and moved to libxdp.
>>> Functions bpf_get/set_link_xdp_id() was deprecated in libbpf 0.8
>>> and replaced with bpf_xdp_query_id() and bpf_xdp_attach/detach().
>>>
>>> Updating configuration and source code to accommodate above changes
>>> and allow building OVS with AF_XDP support on newer systems:
>>>
>>> - Checking availability of the libxdp in a system by looking
>>> for a library providing libxdp_strerror().
>>>
>>> - Checking for xsk.h header provided by libxdp-dev[el] first,
>>> fall back to xsk.h from libbpf if not found.
>>>
>>> - Check for the NEED_WAKEUP feature replaced with direct checking
>>> in the source code if XDP_USE_NEED_WAKEUP is defined.
>>>
>>> - Checking availability of bpf_xdp_query_id and bpf_xdp_detach
>>> and using them instead of deprecated APIs. Fall back to old
>>> functions if not found.
>>
>> So I guess this requires our build environment to match our runtime
>> environment, as these functions are from dynamic libraries, not statically
>> linked?
>
> Not exactly match, but symbols available during the build should
> be present in the runtime. In general it means that libraries
> at build time should be the same or older than runtime ones.
>
> If the build environment is newer that will obviously not work,
> but I don't think that is generally supported anyway.
Guess we will find out once we switch the default ;)
>>
>> I guess this is find, as long as people understand it.
>>
>>>
>>> - Dropped LIBBPF_LDADD variable as it makes library and function
>>> detection much harder without providing any actual benefits.
>>> AC_SEARCH_LIBS is used instead and it allows use of AC_CHECK_FUNCS.
>>>
>>> - Header includes moved around to files where they are actually used.
>>>
>>> - Removed libelf dependency as it is not really used.
>>>
>>> With these changes it should be possible to build OVS with either:
>>>
>>> - libbpf built from the kernel sources (5.19 or older).
>>> - libbpf < 0.7 provided in distributions.
>>> - libxdp and libbpf >= 0.7 provided in newer distributions.
>>>
>>> libxdp added as a build dependency for Fedora build since all
>>> supported versions of Fedora are packaging this library.
>>>
>>> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
>>
>> I have problems building this on my fedora35 system with
>> gcc-11.3.1-3.fc35.x86_64:
>>
>> libtool: link: ( cd "include/openvswitch/.libs" && rm -f "libcxxtest.la" &&
>> ln -s "../libcxxtest.la" "libcxxtest.la" )
>> In file included from lib/netdev-linux-private.h:30,
>> from lib/netdev-afxdp.c:19:
>> In function ‘dp_packet_delete’,
>> inlined from ‘dp_packet_delete’ at lib/dp-packet.h:246:1,
>> inlined from ‘dp_packet_batch_add__’ at lib/dp-packet.h:775:9,
>> inlined from ‘dp_packet_batch_add’ at lib/dp-packet.h:783:5,
>> inlined from ‘netdev_afxdp_rxq_recv’ at lib/netdev-afxdp.c:894:9:
>> lib/dp-packet.h:260:9: error: ‘free’ called on pointer ‘*umem.xpool.array’
>> with nonzero offset [8, 2558044588346441168] [-Werror=free-nonheap-object]
>> 260 | free(b);
>> | ^~~~~~~
>>
>> Guess it does not recognise the (b->source == DPBUF_AFXDP) statement…
>
> This is annoying, I didn't found a way to trick compiler into
> doing the right thing. The code path is fairly obvious and
> b->source is always set on that code path just a few lines above.
>
> So, it definitely looks like a compiler bug.
>
> Do you know of a good portable way disabling warnings in the code?
> Otherwise, we can disable it globally in the configure script if
> building with AF_XDP.
I know there is ‘#pragma clang diagnostic’ and ‘#pragma gcc diagnostic’ not
sure what other compilers we support.
>>
>> This is my build config:
>>
>> ./configure --enable-Werror --enable-usdt-probes --localstatedir=/var
>> --prefix=/usr --sysconfdir=/etc --enable-afxdp
>>
>> Guess this should be fixed before we enable afxdp by default?
>>
>>
>> Also when I build it without the Werror option I’m not able to start a
>> sandbox:
>>
>> make[1]: Leaving directory '/home/echaudron/Documents/review/ovs_ilya_afxdp'
>> ovsdb-tool create conf.db
>> /home/echaudron/Documents/review/ovs_ilya_afxdp/vswitchd/vswitch.ovsschema
>> ovsdb-tool: symbol lookup error: /lib64/libxdp.so.1: undefined symbol:
>> silence_libbpf_logging
>> cat:
>> '/home/echaudron/Documents/review/ovs_ilya_afxdp/tutorial/sandbox/*.pid': No
>> such file or directory
>>
>> But this might be something specific to libxdp on my system, and libbpf :(
>
> Yeah, I guess libxdp and libbpf versions on f35 are not really compatible.
> We're not calling silence_libbpf_logging from OVS, so it's a call from the
> libbpf itself.
>
>>
>>> ---
>>> NEWS | 2 ++
>>> acinclude.m4 | 21 +++++++++---------
>>> lib/automake.mk | 1 -
>>> lib/libopenvswitch.pc.in | 2 +-
>>> lib/netdev-afxdp-pool.c | 2 ++
>>> lib/netdev-afxdp-pool.h | 5 -----
>>> lib/netdev-afxdp.c | 38 ++++++++++++++++++++++++++-------
>>> rhel/openvswitch-fedora.spec.in | 2 +-
>>> 8 files changed, 46 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/NEWS b/NEWS
>>> index 265375e1c..5d39c7d27 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -1,5 +1,7 @@
>>> Post-v3.0.0
>>> --------------------
>>> + - AF_XDP:
>>> + * Added support for building with libxdp and libbpf >= 0.7.
>>> - ovs-appctl:
>>> * "ovs-appctl ofproto/trace" command can now display port names with
>>> the
>>> "--names" option.
>>> diff --git a/acinclude.m4 b/acinclude.m4
>>> index aa9af5506..aed01c967 100644
>>> --- a/acinclude.m4
>>> +++ b/acinclude.m4
>>> @@ -251,7 +251,7 @@ AC_DEFUN([OVS_FIND_DEPENDENCY], [
>>>
>>> dnl OVS_CHECK_LINUX_AF_XDP
>>> dnl
>>> -dnl Check both Linux kernel AF_XDP and libbpf support
>>> +dnl Check both Linux kernel AF_XDP and libbpf/libxdp support
>>> AC_DEFUN([OVS_CHECK_LINUX_AF_XDP], [
>>> AC_ARG_ENABLE([afxdp],
>>> [AS_HELP_STRING([--enable-afxdp], [Enable AF-XDP
>>> support])],
>>> @@ -270,23 +270,22 @@ AC_DEFUN([OVS_CHECK_LINUX_AF_XDP], [
>>> AC_CHECK_HEADER([linux/if_xdp.h], [],
>>> [AC_MSG_ERROR([unable to find linux/if_xdp.h for AF_XDP support])])
>>>
>>> - AC_CHECK_HEADER([bpf/xsk.h], [],
>>> - [AC_MSG_ERROR([unable to find bpf/xsk.h for AF_XDP support])])
>>> + AC_CHECK_HEADER([xdp/xsk.h],
>>> + AC_DEFINE([HAVE_LIBXDP], [1], [xsk.h is supplied with libxdp]),
>>> + AC_CHECK_HEADER([bpf/xsk.h], [],
>>> + [AC_MSG_ERROR([unable to find xsk.h for AF_XDP support])]))
>>>
>>> AC_CHECK_FUNCS([pthread_spin_lock], [],
>>> [AC_MSG_ERROR([unable to find pthread_spin_lock for AF_XDP
>>> support])])
>>>
>>> OVS_FIND_DEPENDENCY([numa_alloc_onnode], [numa], [libnuma])
>>> + OVS_FIND_DEPENDENCY([libbpf_strerror], [bpf], [libbpf])
>>> + AC_SEARCH_LIBS([libxdp_strerror], [xdp])
>>> +
>>> + AC_CHECK_FUNCS([bpf_xdp_query_id bpf_xdp_detach])
>>>
>>> AC_DEFINE([HAVE_AF_XDP], [1],
>>> [Define to 1 if AF_XDP support is available and enabled.])
>>> - LIBBPF_LDADD=" -lbpf -lelf"
>>> - AC_SUBST([LIBBPF_LDADD])
>>> -
>>> - AC_CHECK_DECL([xsk_ring_prod__needs_wakeup], [
>>> - AC_DEFINE([HAVE_XDP_NEED_WAKEUP], [1],
>>> - [XDP need wakeup support detected in xsk.h.])
>>> - ], [], [[#include <bpf/xsk.h>]])
>>> fi
>>> AM_CONDITIONAL([HAVE_AF_XDP], test "$AF_XDP_ENABLE" = true)
>>> ])
>>> @@ -357,7 +356,7 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>>> ], [], [[#include <rte_config.h>]])
>>>
>>> AC_CHECK_DECL([RTE_NET_AF_XDP], [
>>> - LIBBPF_LDADD="-lbpf"
>>> + OVS_FIND_DEPENDENCY([libbpf_strerror], [bpf], [libbpf])
>>> ], [], [[#include <rte_config.h>]])
>>>
>>> AC_CHECK_DECL([RTE_LIBRTE_VHOST_NUMA], [
>>> diff --git a/lib/automake.mk b/lib/automake.mk
>>> index a0fabe38f..61bdc308f 100644
>>> --- a/lib/automake.mk
>>> +++ b/lib/automake.mk
>>> @@ -9,7 +9,6 @@ lib_LTLIBRARIES += lib/libopenvswitch.la
>>>
>>> lib_libopenvswitch_la_LIBADD = $(SSL_LIBS)
>>> lib_libopenvswitch_la_LIBADD += $(CAPNG_LDADD)
>>> -lib_libopenvswitch_la_LIBADD += $(LIBBPF_LDADD)
>>>
>>>
>>> if WIN32
>>> diff --git a/lib/libopenvswitch.pc.in b/lib/libopenvswitch.pc.in
>>> index 44fbb1f9f..a5f4d3947 100644
>>> --- a/lib/libopenvswitch.pc.in
>>> +++ b/lib/libopenvswitch.pc.in
>>> @@ -7,5 +7,5 @@ Name: libopenvswitch
>>> Description: Open vSwitch library
>>> Version: @VERSION@
>>> Libs: -L${libdir} -lopenvswitch
>>> -Libs.private: @LIBS@ @SSL_LIBS@ @CAPNG_LDADD@ @LIBBPF_LDADD@
>>> +Libs.private: @LIBS@ @SSL_LIBS@ @CAPNG_LDADD@
>>> Cflags: -I${includedir}
>>> diff --git a/lib/netdev-afxdp-pool.c b/lib/netdev-afxdp-pool.c
>>> index 3386d2dcf..f56a7b29e 100644
>>> --- a/lib/netdev-afxdp-pool.c
>>> +++ b/lib/netdev-afxdp-pool.c
>>> @@ -15,6 +15,8 @@
>>> */
>>> #include <config.h>
>>>
>>
>> Remove new line here?
>
> I just wanted to separate it from system headers, since
> it's not one of them.
>
>>
>>> +#include <errno.h>
>>> +
>>> #include "dp-packet.h"
>>> #include "netdev-afxdp-pool.h"
>>> #include "openvswitch/util.h"
>>> diff --git a/lib/netdev-afxdp-pool.h b/lib/netdev-afxdp-pool.h
>>> index f929b9489..6681cf539 100644
>>> --- a/lib/netdev-afxdp-pool.h
>>> +++ b/lib/netdev-afxdp-pool.h
>>> @@ -19,12 +19,7 @@
>>>
>>> #ifdef HAVE_AF_XDP
>>>
>>> -#include <bpf/xsk.h>
>>> -#include <errno.h>
>>> -#include <stdbool.h>
>>> -
>>> #include "openvswitch/thread.h"
>>> -#include "ovs-atomic.h"
>>>
>>> /* LIFO ptr_array. */
>>> struct umem_pool {
>>> diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
>>> index ca3f2431e..6ced8a2b6 100644
>>> --- a/lib/netdev-afxdp.c
>>> +++ b/lib/netdev-afxdp.c
>>> @@ -21,6 +21,11 @@
>>> #include "netdev-afxdp.h"
>>> #include "netdev-afxdp-pool.h"
>>>
>>> +#ifdef HAVE_LIBXDP
>>> +#include <xdp/xsk.h>
>>> +#else
>>> +#include <bpf/xsk.h>
>>> +#endif
>>> #include <errno.h>
>>> #include <inttypes.h>
>>> #include <linux/rtnetlink.h>
>>> @@ -29,6 +34,7 @@
>>> #include <numa.h>
>>> #include <numaif.h>
>>> #include <poll.h>
>>> +#include <stdbool.h>
>>> #include <stdlib.h>
>>> #include <sys/resource.h>
>>> #include <sys/socket.h>
>>> @@ -44,6 +50,7 @@
>>> #include "openvswitch/list.h"
>>> #include "openvswitch/thread.h"
>>> #include "openvswitch/vlog.h"
>>> +#include "ovs-atomic.h"
>>> #include "ovs-numa.h"
>>> #include "packets.h"
>>> #include "socket-util.h"
>>> @@ -72,7 +79,7 @@ static struct vlog_rate_limit rl =
>>> VLOG_RATE_LIMIT_INIT(5, 20);
>>> #define PROD_NUM_DESCS XSK_RING_PROD__DEFAULT_NUM_DESCS
>>> #define CONS_NUM_DESCS XSK_RING_CONS__DEFAULT_NUM_DESCS
>>>
>>> -#ifdef HAVE_XDP_NEED_WAKEUP
>>> +#ifdef XDP_USE_NEED_WAKEUP
>>> #define NEED_WAKEUP_DEFAULT true
>>> #else
>>> #define NEED_WAKEUP_DEFAULT false
>>> @@ -169,7 +176,7 @@ struct netdev_afxdp_tx_lock {
>>> );
>>> };
>>>
>>> -#ifdef HAVE_XDP_NEED_WAKEUP
>>> +#ifdef XDP_USE_NEED_WAKEUP
>>> static inline void
>>> xsk_rx_wakeup_if_needed(struct xsk_umem_info *umem,
>>> struct netdev *netdev, int fd)
>>> @@ -201,7 +208,7 @@ xsk_tx_need_wakeup(struct xsk_socket_info *xsk_info)
>>> return xsk_ring_prod__needs_wakeup(&xsk_info->tx);
>>> }
>>>
>>> -#else /* !HAVE_XDP_NEED_WAKEUP */
>>> +#else /* !XDP_USE_NEED_WAKEUP */
>>> static inline void
>>> xsk_rx_wakeup_if_needed(struct xsk_umem_info *umem OVS_UNUSED,
>>> struct netdev *netdev OVS_UNUSED,
>>> @@ -215,7 +222,7 @@ xsk_tx_need_wakeup(struct xsk_socket_info *xsk_info
>>> OVS_UNUSED)
>>> {
>>> return true;
>>> }
>>> -#endif /* HAVE_XDP_NEED_WAKEUP */
>>> +#endif /* XDP_USE_NEED_WAKEUP */
>>>
>>> static void
>>> netdev_afxdp_cleanup_unused_pool(struct unused_pool *pool)
>>> @@ -351,7 +358,7 @@ xsk_configure_socket(struct xsk_umem_info *umem,
>>> uint32_t ifindex,
>>> cfg.bind_flags = xdp_modes[mode].bind_flags;
>>> cfg.xdp_flags = xdp_modes[mode].xdp_flags |
>>> XDP_FLAGS_UPDATE_IF_NOEXIST;
>>>
>>> -#ifdef HAVE_XDP_NEED_WAKEUP
>>> +#ifdef XDP_USE_NEED_WAKEUP
>>> if (use_need_wakeup) {
>>> cfg.bind_flags |= XDP_USE_NEED_WAKEUP;
>>> }
>>> @@ -377,7 +384,11 @@ xsk_configure_socket(struct xsk_umem_info *umem,
>>> uint32_t ifindex,
>>> }
>>>
>>> /* Make sure the built-in AF_XDP program is loaded. */
>>> +#ifdef HAVE_BPF_XDP_QUERY_ID
>>> + ret = bpf_xdp_query_id(ifindex, cfg.xdp_flags, &prog_id);
>>> +#else
>>> ret = bpf_get_link_xdp_id(ifindex, &prog_id, cfg.xdp_flags);
>>> +#endif
>>> if (ret || !prog_id) {
>>> if (ret) {
>>> VLOG_ERR("Get XDP prog ID failed (%s)", ovs_strerror(errno));
>>> @@ -630,9 +641,9 @@ netdev_afxdp_set_config(struct netdev *netdev, const
>>> struct smap *args,
>>> }
>>>
>>> need_wakeup = smap_get_bool(args, "use-need-wakeup",
>>> NEED_WAKEUP_DEFAULT);
>>> -#ifndef HAVE_XDP_NEED_WAKEUP
>>> +#ifndef XDP_USE_NEED_WAKEUP
>>> if (need_wakeup) {
>>> - VLOG_WARN("XDP need_wakeup is not supported in libbpf.");
>>> + VLOG_WARN("XDP need_wakeup is not supported in libbpf/libxdp.");
>>> need_wakeup = false;
>>> }
>>> #endif
>>> @@ -742,7 +753,11 @@ xsk_remove_xdp_program(uint32_t ifindex, enum
>>> afxdp_mode mode)
>>> uint32_t ret, prog_id = 0;
>>>
>>> /* Check whether XDP program is loaded. */
>>> +#ifdef HAVE_BPF_XDP_QUERY_ID
>>> + ret = bpf_xdp_query_id(ifindex, flags, &prog_id);
>>> +#else
>>> ret = bpf_get_link_xdp_id(ifindex, &prog_id, flags);
>>> +#endif
>>> if (ret) {
>>> VLOG_ERR("Failed to get XDP prog id (%s)", ovs_strerror(errno));
>>> return;
>>> @@ -753,7 +768,14 @@ xsk_remove_xdp_program(uint32_t ifindex, enum
>>> afxdp_mode mode)
>>> return;
>>> }
>>>
>>> - bpf_set_link_xdp_fd(ifindex, -1, flags);
>>> +#ifdef HAVE_BPF_XDP_DETACH
>>> + if (bpf_xdp_detach(ifindex, flags, NULL) != 0) {
>>> +#else
>>> + if (bpf_set_link_xdp_fd(ifindex, -1, flags) != 0) {
>>> +#endif
>>> + VLOG_ERR("Failed to detach XDP program (%s) at ifindex %d",
>>> + ovs_strerror(errno), ifindex);
>>> + }
>>> }
>>>
>>> void
>>> diff --git a/rhel/openvswitch-fedora.spec.in
>>> b/rhel/openvswitch-fedora.spec.in
>>> index 4a3e6294b..6564d5252 100644
>>> --- a/rhel/openvswitch-fedora.spec.in
>>> +++ b/rhel/openvswitch-fedora.spec.in
>>> @@ -75,7 +75,7 @@ BuildRequires: dpdk-devel >= 22.11
>>> Provides: %{name}-dpdk = %{version}-%{release}
>>> %endif
>>> %if %{with afxdp}
>>> -BuildRequires: libbpf-devel numactl-devel
>>> +BuildRequires: libxdp-devel libbpf-devel numactl-devel
>>> %endif
>>> BuildRequires: unbound unbound-devel
>>>
>>> --
>>> 2.38.1
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
