| Message ID | 20221219122020.3778161-3-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | AF_XDP build fixes and enhancements. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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? 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.maximets@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 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 :( > --- > 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? > +#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
On Tue, Dec 20, 2022 at 2:01 PM Eelco Chaudron <echaudro@redhat.com> wrote: > 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 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? Same for me. I have been scratching my head over this report... I wonder if this is a compiler bug.
On 20 Dec 2022, at 14:06, David Marchand wrote: > On Tue, Dec 20, 2022 at 2:01 PM Eelco Chaudron <echaudro@redhat.com> wrote: >> 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 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? > > Same for me. > I have been scratching my head over this report... I wonder if this is > a compiler bug. I guess the compiler does not understand that we will always call dp_packet_delete() with the source being DPBUF_AFXDP, and don’t hit the free(). Guess we should probably disable the warning in this specific code path. //Eelco
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 <i.maximets@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. > > 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 >
On 12/20/22 14:14, Eelco Chaudron wrote: > > > On 20 Dec 2022, at 14:06, David Marchand wrote: > >> On Tue, Dec 20, 2022 at 2:01 PM Eelco Chaudron <echaudro@redhat.com> wrote: >>> 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 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? >> >> Same for me. >> I have been scratching my head over this report... I wonder if this is >> a compiler bug. > > I guess the compiler does not understand that we will always call dp_packet_delete() with the source being DPBUF_AFXDP, and don’t hit the free(). > Guess we should probably disable the warning in this specific code path. > > //Eelco > Meanwhile I opened a GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108187 There are few similar issues in the tracker, so it might make sense disabling the warning. Best regards, Ilya Maximets.
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.maximets@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 >>
On 12/20/22 15:38, Ilya Maximets wrote: > On 12/20/22 14:14, Eelco Chaudron wrote: >> >> >> On 20 Dec 2022, at 14:06, David Marchand wrote: >> >>> On Tue, Dec 20, 2022 at 2:01 PM Eelco Chaudron <echaudro@redhat.com> wrote: >>>> 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 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? >>> >>> Same for me. >>> I have been scratching my head over this report... I wonder if this is >>> a compiler bug. >> >> I guess the compiler does not understand that we will always call dp_packet_delete() with the source being DPBUF_AFXDP, and don’t hit the free(). >> Guess we should probably disable the warning in this specific code path. >> >> //Eelco >> > > Meanwhile I opened a GCC bug: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108187 So, AFAICT, the issue is that implementation of the dp_packet_use_afxdp() is part of a different translation unit (dp-packet.c) and GCC has no access to it while processing netdev-afxdp.c, so it doesn't know that packet source is set to DPBUF_AFXDP. Hence the warning. There are still a few misteries around this, e.g. why the issue is not seen with -O0, but they will likely not very important. I don't see a good way to fix that. We could inline a bunch of functions from dp-packet.c, or explicitly set the source to DPBUF_AFXDP somewhere in the realm of netdev-afxdp.c module, but I don't think that any of that is a good solution. I'll add a patch to disable the warning with #pragma GCC here around the dp_packet_batch_add() call in netdev-afxdp.c for now, unless there are better suggestions. Clang doesn't emit this warning, so we don't need to have a workaround for it. Best regards, Ilya Maximets.
On 21 Dec 2022, at 13:42, Ilya Maximets wrote: > On 12/20/22 15:38, Ilya Maximets wrote: >> On 12/20/22 14:14, Eelco Chaudron wrote: >>> >>> >>> On 20 Dec 2022, at 14:06, David Marchand wrote: >>> >>>> On Tue, Dec 20, 2022 at 2:01 PM Eelco Chaudron <echaudro@redhat.com> wrote: >>>>> 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 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? >>>> >>>> Same for me. >>>> I have been scratching my head over this report... I wonder if this is >>>> a compiler bug. >>> >>> I guess the compiler does not understand that we will always call dp_packet_delete() with the source being DPBUF_AFXDP, and don’t hit the free(). >>> Guess we should probably disable the warning in this specific code path. >>> >>> //Eelco >>> >> >> Meanwhile I opened a GCC bug: >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108187 > > So, AFAICT, the issue is that implementation of the dp_packet_use_afxdp() > is part of a different translation unit (dp-packet.c) and GCC has no access > to it while processing netdev-afxdp.c, so it doesn't know that packet > source is set to DPBUF_AFXDP. Hence the warning. There are still a few > misteries around this, e.g. why the issue is not seen with -O0, but they > will likely not very important. > > I don't see a good way to fix that. We could inline a bunch of functions > from dp-packet.c, or explicitly set the source to DPBUF_AFXDP somewhere in > the realm of netdev-afxdp.c module, but I don't think that any of that is > a good solution. > > I'll add a patch to disable the warning with #pragma GCC here around the > dp_packet_batch_add() call in netdev-afxdp.c for now, unless there are > better suggestions. Clang doesn't emit this warning, so we don't need > to have a workaround for it. Sound like a good plan to me! > Best regards, Ilya Maximets.
On Wed, Dec 21, 2022 at 1:42 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 12/20/22 15:38, Ilya Maximets wrote: > > On 12/20/22 14:14, Eelco Chaudron wrote: > >> On 20 Dec 2022, at 14:06, David Marchand wrote: > >>> On Tue, Dec 20, 2022 at 2:01 PM Eelco Chaudron <echaudro@redhat.com> wrote: > >>>> 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 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? > >>> > >>> Same for me. > >>> I have been scratching my head over this report... I wonder if this is > >>> a compiler bug. > >> > >> I guess the compiler does not understand that we will always call dp_packet_delete() with the source being DPBUF_AFXDP, and don’t hit the free(). > >> Guess we should probably disable the warning in this specific code path. > >> > >> //Eelco > >> > > > > Meanwhile I opened a GCC bug: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108187 > > So, AFAICT, the issue is that implementation of the dp_packet_use_afxdp() > is part of a different translation unit (dp-packet.c) and GCC has no access > to it while processing netdev-afxdp.c, so it doesn't know that packet > source is set to DPBUF_AFXDP. Hence the warning. There are still a few > misteries around this, e.g. why the issue is not seen with -O0, but they > will likely not very important. > > I don't see a good way to fix that. We could inline a bunch of functions > from dp-packet.c, or explicitly set the source to DPBUF_AFXDP somewhere in > the realm of netdev-afxdp.c module, but I don't think that any of that is > a good solution. > > I'll add a patch to disable the warning with #pragma GCC here around the > dp_packet_batch_add() call in netdev-afxdp.c for now, unless there are > better suggestions. Clang doesn't emit this warning, so we don't need > to have a workaround for it. +1. Thanks.
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> +#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
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. - 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.maximets@ovn.org> --- 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(-)