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. > > 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 <[email protected]> > > 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. > > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
