On 6/20/23 16:13, Frode Nordahl wrote: > On Tue, Jun 20, 2023 at 3:44 PM Ilya Maximets <[email protected]> wrote: >> >> On 6/20/23 10:08, Frode Nordahl wrote: >>> Allow building the upstream Open vSwitch deb package with AF_XDP >>> datapath enabled by passing in the string 'afxdp' in the >>> DEB_BUILD_OPTIONS environment variable. >> >> Hi, Frode. Thanks for the patch! > > Thank you for taking the time to review, Ilya. Much appreciated! > >> Can we also update the debian-deb make target to pass this argument >> whenever we HAVE_AF_XDP ? And maybe add a CI job for it? >> Documentation/intro/install/debian.rst may also need some updates. > > Enabling AF_XDP whenever we HAVE_AF_XDP would be my preference, > we have enabled the build by default in recent development releases of > Debian/Ubuntu. I was just continuing the caution set out in the commit [0] > that added the default build behavior to autoconf.
We enabled AF_XDP support in Fedora by default [2], so I don't see a reason to not do the same for Ubuntu/Debian whenever required versions on libxdp/libbpf are available. [2] https://github.com/openvswitch/ovs/commit/9736b971b > > We have had no reported issues so far for the builds where this has been > enabled, and from the work done on this patch it appears the detection > works good and produces buildable artifacts on systems without the required > libraries. Good to know. > > If that is what we want we could probably do without the > extra DEB_BUILD_OPTION though, and produce a debian/control with the > appropriate build-/runtime dependencies when we HAVE_AF_XDP and let > autoconf/automake dtrt in the package build. > > What do you think? Sounds good to me. Maybe provide a 'noafxdp' option similar to 'nodpdk' ? But I guess it's fine without it either. > > I'll also take a look at docs and CI for this. > > 0: https://github.com/openvswitch/ovs/commit/e44e8034318 > >> Some more comments below. >> >>> >>> Signed-off-by: Frode Nordahl <[email protected]> >>> --- >>> debian/automake.mk | 30 ++++++++++++++++++++++-------- >>> debian/control.in | 2 ++ >>> debian/rules | 8 +++++++- >>> 3 files changed, 31 insertions(+), 9 deletions(-) >>> >>> diff --git a/debian/automake.mk b/debian/automake.mk >>> index 7b2afafae..3195964c9 100644 >>> --- a/debian/automake.mk >>> +++ b/debian/automake.mk >>> @@ -95,17 +95,29 @@ CLEANFILES += debian/copyright >>> >>> >>> if DPDK_NETDEV >>> -update_deb_control = \ >>> - $(AM_V_GEN) sed -e 's/^\# DPDK_NETDEV //' \ >>> - < $(srcdir)/debian/control.in > debian/control >>> +update_deb_control_dpdk = \ >>> + $(AM_V_GEN) sed -i 's/^\# DPDK_NETDEV //' \ >>> + $(srcdir)/debian/control >>> else >>> -update_deb_control = \ >>> - $(AM_V_GEN) grep -v '^\# DPDK_NETDEV' \ >>> - < $(srcdir)/debian/control.in > debian/control >>> +update_deb_control_dpdk = \ >>> + $(AM_V_GEN) sed -i '/^\# DPDK_NETDEV /d' \ >>> + $(srcdir)/debian/control >>> +endif >>> + >>> +if HAVE_AF_XDP >>> +update_deb_control_afxdp = \ >>> + $(AM_V_GEN) sed -i 's/^\# HAVE_AF_XDP //' \ >>> + $(srcdir)/debian/control >>> +else >>> +update_deb_control_afxdp = \ >>> + $(AM_V_GEN) sed -i '/^\# HAVE_AF_XDP /d' \ >>> + $(srcdir)/debian/control >>> endif >>> >>> debian/control: $(srcdir)/debian/control.in Makefile >>> - $(update_deb_control) >>> + cp $(srcdir)/debian/control.in $(srcdir)/debian/control >>> + $(update_deb_control_afxdp) >>> + $(update_deb_control_dpdk) >>> >>> CLEANFILES += debian/control >>> >>> @@ -120,8 +132,10 @@ debian-deb: debian >>> exit 1; >>> \ >>> fi >>> $(MAKE) distclean >>> + cp $(srcdir)/debian/control.in $(srcdir)/debian/control >>> $(update_deb_copyright) >>> - $(update_deb_control) >>> + $(update_deb_control_afxdp) >>> + $(update_deb_control_dpdk) >>> $(AM_V_GEN) fakeroot debian/rules clean >>> if DPDK_NETDEV >>> $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc`" \ >>> diff --git a/debian/control.in b/debian/control.in >>> index 19f590d06..d2711deaa 100644 >>> --- a/debian/control.in >>> +++ b/debian/control.in >>> @@ -19,6 +19,7 @@ Build-Depends: >>> dh-sequence-sphinxdoc, >>> graphviz, >>> iproute2, >>> +# HAVE_AF_XDP libbpf-dev, >>> libcap-ng-dev, >>> libdbus-1-dev [amd64 i386 ppc64el arm64], >>> # DPDK_NETDEV libdpdk-dev (>= 22.11) [amd64 i386 ppc64el arm64], >>> @@ -27,6 +28,7 @@ Build-Depends: >>> libssl-dev, >>> libtool, >>> libunbound-dev, >>> +# HAVE_AF_XDP libxdp-dev (>= 1.2.9~) [!alpha !arc !hppa !ia64 !m68k !sh4], >>> openssl, >>> pkg-config, >>> procps, >> >> These are build dependencies, but the package will have also a runtime >> dependency on libxdp (and libbpf ?). Should these be added to 'Depends' >> section or will they be added automagically somehow as rpm does? > > There is no magic, so it appears I forgot about adding those, thank you for > pointing it out! Ack. > >> Also, we do not update the dpdk-enabled package here. It should be >> possible to build a package with both dpdk and afxdp at the same time >> if we change the 'sed' calls above to not match on the start of a line. >> This way we can have '# DPDK_NETDEV # HAVE_AF_XDP libbpf-dev,' that >> will be uncommented only if both are enabled. What do you think? > > So in the current proposal, I changed the sed calls to do interactive edits > on the control file as opposed to rewriting the file. > > That way you can currently enable the two features in any of the four axis > already, or am I missing something? Ah, OK. We just need to be sure that HAVE_DPDK goes before HAVE_AF_XDP. But that should work, I agree. The openvswitch-switch-dpdk package doesn't have libxdp/bpf as a dependency with the current version on a patch though. > >>> diff --git a/debian/rules b/debian/rules >>> index 28c249d07..8ed19db70 100755 >>> --- a/debian/rules >>> +++ b/debian/rules >>> @@ -19,13 +19,19 @@ endif >>> PYTHON3S:=$(shell py3versions -vr) >>> DEB_HOST_ARCH?=$(shell dpkg-architecture -qDEB_HOST_ARCH) >>> >>> +ifneq (,$(filter afxdp, $(DEB_BUILD_OPTIONS))) >>> +AFXDP:=--enable-afxdp >>> +else >>> +AFXDP:=--disable-afxdp >>> +endif >>> + >>> override_dh_auto_configure: >>> test -d _debian || mkdir _debian >>> cd _debian && ( \ >>> test -e Makefile || \ >>> ../configure --prefix=/usr --localstatedir=/var \ >>> --enable-ssl \ >>> - --disable-afxdp \ >>> + $(AFXDP) \ >>> --sysconfdir=/etc \ >>> $(DATAPATH_CONFIGURE_OPTS) \ >>> $(EXTRA_CONFIGURE_OPTS) \ >> >> Not sure if that can improve anything, but the --enable-afxdp=no|yes|auto >> is an acceptable syntax too. In case that can save some conditionals. > > Yeah, as discussed above, if it would be welcome to just enable this by > default when autoconf/automake has detected and produced a control file > with the required build-/runtime dependencies, I'm all for it. > Sure. Sounds good to me. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
