On Tue, Jun 20, 2023 at 6:03 PM Ilya Maximets <i.maxim...@ovn.org> wrote: > > On 6/20/23 16:13, Frode Nordahl wrote: > > On Tue, Jun 20, 2023 at 3:44 PM Ilya Maximets <i.maxim...@ovn.org> 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.
Yes, that sounds reasonable to me. > > I'll also take a look at docs and CI for this. Speaking of CI, the next commit in the series adds tests, so we could in essence just run those. I'll check if I can find a good way to run those on GHA. In light of that, the fact that not adding Depends was not caught was a bit disappointing. It is due to the need to include all the build dependencies to run the test, which incidentally make the packaged binaries run even when dependencies are not declared. I'll see if I either can remove the build deps after producing the testsuite or see if I can make the OVS build system build only the testsuite and not the rest of the tree. > > 0: https://github.com/openvswitch/ovs/commit/e44e8034318 > > > >> Some more comments below. > >> > >>> > >>> Signed-off-by: Frode Nordahl <frode.nord...@canonical.com> > >>> --- > >>> 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. Indeed, I did not think about the use case of having AF_XDP and DPDK in the same binary, but that is of course possible. I guess I'll extend the test to run both `dpdk` and `afxdp` testsuites for the DPDK binary when enabled then. > > > >>> 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. Thanks! -- Frode Nordahl > > Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
