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 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. 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? 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! > 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? > > 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. -- Frode Nordahl > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
