On Thu, Jun 22, 2023 at 11:58 AM Frode Nordahl <[email protected]> wrote: > > On Tue, Jun 20, 2023 at 6:03 PM Ilya Maximets <[email protected]> wrote: > > > > 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. > > 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 <[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.
Circling back to this one, there is actually magic here, specifically this line [1][2] will make the tooling automatically determine runtime dependencies. The updated v3, which is imminent, ensures removal of build-deps before running the test to ensure no runtime deps are missed. A job is also added to Cirrus CI, as it has access to newer images, running the tests added in the next patch in the series. 1: https://github.com/openvswitch/ovs/blob/903294cde6e1/debian/control.in#L162 2: https://www.debian.org/doc/debian-policy/ch-sharedlibs.html#generating-dependencies-on-shared-libraries -- Frode Nordahl > > > > > > > >> 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
