diff mbox series

[ovs-dev,v2,6/7] acinclude.m4: Build with AF_XDP support by default if possible.

Message ID 20221219122020.3778161-7-i.maximets@ovn.org
State Superseded
Headers show
Series AF_XDP build fixes and enhancements. | expand

Checks

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

Commit Message

Ilya Maximets Dec. 19, 2022, 12:20 p.m. UTC 
With this change we will try to detect all the netdev-afxdp
dependencies and enable AF_XDP support by default if they are
present at the build time.

Configuration script behaves in a following way:

 - ./configure --enable-afxdp

   Will check for AF_XDP dependencies and fail if they are
   not available.

 - ./configure --disable-afxdp

   Disables checking for AF_XDP.  Build will not support
   AF_XDP even if all dependencies are installed.

 - Just ./configure or ./configure --enable-afxdp=auto

   Will check for AF_XDP dependencies.  Will print a warning
   if they are not available, but will continue without AF_XDP
   support.  If dependencies are available in a system, this
   option is equal to --enable-afxdp, except that AF_XDP will
   not be enabled for libbpf >= 0.7 if libxdp is not available,
   to avoid deprecation warnings during the build.

'--disable-afxdp' added to the debian and fedora package builds
to keep predictable behavior.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 Documentation/intro/install/afxdp.rst |  6 +-
 NEWS                                  |  3 +
 acinclude.m4                          | 89 ++++++++++++++++++---------
 debian/rules                          | 25 +++++---
 rhel/openvswitch-fedora.spec.in       |  2 +
 5 files changed, 85 insertions(+), 40 deletions(-)

Comments

Eelco Chaudron Dec. 20, 2022, 1:19 p.m. UTC | #1 
On 19 Dec 2022, at 13:20, Ilya Maximets wrote:

> With this change we will try to detect all the netdev-afxdp
> dependencies and enable AF_XDP support by default if they are
> present at the build time.
>
> Configuration script behaves in a following way:
>
>  - ./configure --enable-afxdp
>
>    Will check for AF_XDP dependencies and fail if they are
>    not available.
>
>  - ./configure --disable-afxdp
>
>    Disables checking for AF_XDP.  Build will not support
>    AF_XDP even if all dependencies are installed.
>
>  - Just ./configure or ./configure --enable-afxdp=auto
>
>    Will check for AF_XDP dependencies.  Will print a warning
>    if they are not available, but will continue without AF_XDP
>    support.  If dependencies are available in a system, this
>    option is equal to --enable-afxdp, except that AF_XDP will
>    not be enabled for libbpf >= 0.7 if libxdp is not available,
>    to avoid deprecation warnings during the build.
>
> '--disable-afxdp' added to the debian and fedora package builds
> to keep predictable behavior.
>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

I still don’t like building AF_XDP automatically, but looks like I’m the only one ;)

> ---
>  Documentation/intro/install/afxdp.rst |  6 +-
>  NEWS                                  |  3 +
>  acinclude.m4                          | 89 ++++++++++++++++++---------
>  debian/rules                          | 25 +++++---
>  rhel/openvswitch-fedora.spec.in       |  2 +
>  5 files changed, 85 insertions(+), 40 deletions(-)
>
> diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst
> index a4f0b87fe..51c24bf5b 100644
> --- a/Documentation/intro/install/afxdp.rst
> +++ b/Documentation/intro/install/afxdp.rst
> @@ -30,8 +30,7 @@ This document describes how to build and install Open vSwitch using
>  AF_XDP netdev.
>
>  .. warning::
> -  The AF_XDP support of Open vSwitch is considered 'experimental',
> -  and it is not compiled in by default.
> +  The AF_XDP support of Open vSwitch is considered 'experimental'.
>
>
>  Introduction
> @@ -137,6 +136,9 @@ bootstrap/configure the package::
>
>    ./boot.sh && ./configure --enable-afxdp
>
> +``--enable-afxdp`` here is optional, but it will ensure that all dependencies
> +are available at the build time.
> +
>  Finally, build and install OVS::
>
>    make && make install
> diff --git a/NEWS b/NEWS
> index 5d39c7d27..d2bbae591 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -2,6 +2,9 @@ Post-v3.0.0
>  --------------------
>     - AF_XDP:
>       * Added support for building with libxdp and libbpf >= 0.7.
> +     * Support for AF_XDP is now enabled by default if all dependencies are
> +       available at the build time.  Use --disable-afxdp to disable.
> +       Use --enable-afxdp to fail the build if dependencies are not present.
>     - 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 aed01c967..8411c0e6c 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -253,39 +253,72 @@ dnl OVS_CHECK_LINUX_AF_XDP
>  dnl
>  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])],
> -                [], [enable_afxdp=no])
> +  AC_ARG_ENABLE(
> +    [afxdp],
> +    [AS_HELP_STRING([--disable-afxdp], [Disable AF-XDP support])],
> +    [case "${enableval}" in
> +       (yes | no | auto) ;;
> +       (*) AC_MSG_ERROR([bad value ${enableval} for --enable-afxdp]) ;;
> +     esac],
> +    [enable_afxdp=auto])
> +
>    AC_MSG_CHECKING([whether AF_XDP is enabled])
> -  if test "$enable_afxdp" != yes; then
> +  if test "$enable_afxdp" == no; then
>      AC_MSG_RESULT([no])
>      AF_XDP_ENABLE=false
>    else
> -    AC_MSG_RESULT([yes])
> +    AC_MSG_RESULT([$enable_afxdp])
>      AF_XDP_ENABLE=true
> -
> -    AC_CHECK_HEADER([bpf/libbpf.h], [],
> -      [AC_MSG_ERROR([unable to find bpf/libbpf.h for AF_XDP support])])
> -
> -    AC_CHECK_HEADER([linux/if_xdp.h], [],
> -      [AC_MSG_ERROR([unable to find linux/if_xdp.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.])
> +    failed_dep=none
> +    dnl Saving libs to restore in case we will end up not building with AF_XDP.
> +    save_LIBS=$LIBS
> +
> +    AC_CHECK_HEADER([bpf/libbpf.h], [], [failed_dep="bpf/libbpf.h"])
> +
> +    if test "$failed_dep" = none; then
> +      AC_CHECK_HEADER([linux/if_xdp.h], [], [failed_dep="linux/if_xdp.h"])
> +    fi
> +
> +    if test "$failed_dep" = none; then
> +      AC_CHECK_HEADER([xdp/xsk.h],
> +        AC_DEFINE([HAVE_LIBXDP], [1], [xsk.h is supplied with libxdp]),
> +        AC_CHECK_HEADER([bpf/xsk.h], [], [failed_dep="xsk.h"]))
> +    fi
> +
> +    if test "$failed_dep" = none; then
> +      AC_CHECK_FUNCS([pthread_spin_lock], [], [failed_dep="pthread_spin_lock"])
> +    fi
> +
> +    if test "$failed_dep" = none; then
> +      AC_SEARCH_LIBS([numa_alloc_onnode], [numa], [], [failed_dep="libnuma"])
> +    fi
> +    if test "$failed_dep" = none; then
> +      AC_SEARCH_LIBS([libbpf_strerror], [bpf], [], [failed_dep="libbpf"])
> +    fi
> +
> +    if test "$failed_dep" = none; then
> +      AC_CHECK_FUNCS([bpf_xdp_query_id bpf_xdp_detach])
> +      AC_SEARCH_LIBS([libxdp_strerror], [xdp], [],
> +        [dnl Fail the auto discovery if we have libbpf >= 0.7 but not libxdp
> +         dnl to avoid deprecation warnings during the build.
> +         if test "$enable_afxdp" = auto -a "x$ac_cv_func_bpf_xdp_detach" = xyes; then
> +            failed_dep="libxdp"
> +         fi
> +        ])
> +    fi
> +
> +    if test "$failed_dep" = none; then
> +      AC_DEFINE([HAVE_AF_XDP], [1],
> +                [Define to 1 if AF_XDP support is available and enabled.])
> +    elif test "$enable_afxdp" = yes; then
> +      AC_MSG_ERROR([Missing $failed_dep dependency for AF_XDP support])
> +    else
> +      AC_MSG_WARN(m4_normalize(
> +          [Cannot find $failed_dep, netdev-afxdp will not be supported
> +           (you may use --disable-afxdp to suppress this warning).]))
> +      AF_XDP_ENABLE=false
> +      LIBS=$save_LIBS
> +    fi
>    fi
>    AM_CONDITIONAL([HAVE_AF_XDP], test "$AF_XDP_ENABLE" = true)
>  ])
> diff --git a/debian/rules b/debian/rules
> index 971bc1775..ddbd4dc5c 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -23,21 +23,26 @@ override_dh_auto_configure:
>  	test -d _debian || mkdir _debian
>  	cd _debian && ( \
>  		test -e Makefile || \
> -		../configure --prefix=/usr --localstatedir=/var --enable-ssl \
> -					 --sysconfdir=/etc \
> -					 $(DATAPATH_CONFIGURE_OPTS) \
> -					 $(EXTRA_CONFIGURE_OPTS) \
> -					 )
> +		../configure --prefix=/usr --localstatedir=/var \
> +					--enable-ssl \
> +					--disable-afxdp \
> +					--sysconfdir=/etc \
> +					$(DATAPATH_CONFIGURE_OPTS) \
> +					$(EXTRA_CONFIGURE_OPTS) \
> +					)
>  ifneq (,$(filter i386 amd64 ppc64el arm64, $(DEB_HOST_ARCH)))
>  ifeq (,$(filter nodpdk, $(DEB_BUILD_OPTIONS)))
>  	test -d _dpdk || mkdir _dpdk
>  	cd _dpdk && ( \
>  		test -e Makefile || \
> -        ../configure --prefix=/usr --localstatedir=/var --enable-ssl \
> -                     --with-dpdk=shared --sysconfdir=/etc \
> -					 $(DATAPATH_CONFIGURE_OPTS) \
> -					 $(EXTRA_CONFIGURE_OPTS) \
> -					 )
> +		../configure --prefix=/usr --localstatedir=/var \
> +					--enable-ssl \
> +					--disable-afxdp \
> +					--with-dpdk=shared \
> +					--sysconfdir=/etc \
> +					$(DATAPATH_CONFIGURE_OPTS) \
> +					$(EXTRA_CONFIGURE_OPTS) \
> +					)
>  endif
>  endif
>
> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
> index 6564d5252..fbfcdcf63 100644
> --- a/rhel/openvswitch-fedora.spec.in
> +++ b/rhel/openvswitch-fedora.spec.in
> @@ -171,6 +171,8 @@ This package provides IPsec tunneling support for OVS tunnels.
>  %endif
>  %if %{with afxdp}
>          --enable-afxdp \
> +%else
> +        --disable-afxdp \
>  %endif

Now that we build with libxdp, should we also not add some requirements for packages to be installed?

I do not think we link statically to libxdp/libbpf?

>          --enable-ssl \
>          --disable-static \
> -- 
> 2.38.1
Ilya Maximets Dec. 20, 2022, 1:24 p.m. UTC | #2 
On 12/20/22 14:19, Eelco Chaudron wrote:
> 
> 
> On 19 Dec 2022, at 13:20, Ilya Maximets wrote:
> 
>> With this change we will try to detect all the netdev-afxdp
>> dependencies and enable AF_XDP support by default if they are
>> present at the build time.
>>
>> Configuration script behaves in a following way:
>>
>>  - ./configure --enable-afxdp
>>
>>    Will check for AF_XDP dependencies and fail if they are
>>    not available.
>>
>>  - ./configure --disable-afxdp
>>
>>    Disables checking for AF_XDP.  Build will not support
>>    AF_XDP even if all dependencies are installed.
>>
>>  - Just ./configure or ./configure --enable-afxdp=auto
>>
>>    Will check for AF_XDP dependencies.  Will print a warning
>>    if they are not available, but will continue without AF_XDP
>>    support.  If dependencies are available in a system, this
>>    option is equal to --enable-afxdp, except that AF_XDP will
>>    not be enabled for libbpf >= 0.7 if libxdp is not available,
>>    to avoid deprecation warnings during the build.
>>
>> '--disable-afxdp' added to the debian and fedora package builds
>> to keep predictable behavior.
>>
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> 
> I still don’t like building AF_XDP automatically, but looks like I’m the only one ;)
> 
>> ---
>>  Documentation/intro/install/afxdp.rst |  6 +-
>>  NEWS                                  |  3 +
>>  acinclude.m4                          | 89 ++++++++++++++++++---------
>>  debian/rules                          | 25 +++++---
>>  rhel/openvswitch-fedora.spec.in       |  2 +
>>  5 files changed, 85 insertions(+), 40 deletions(-)
>>
>> diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst
>> index a4f0b87fe..51c24bf5b 100644
>> --- a/Documentation/intro/install/afxdp.rst
>> +++ b/Documentation/intro/install/afxdp.rst
>> @@ -30,8 +30,7 @@ This document describes how to build and install Open vSwitch using
>>  AF_XDP netdev.
>>
>>  .. warning::
>> -  The AF_XDP support of Open vSwitch is considered 'experimental',
>> -  and it is not compiled in by default.
>> +  The AF_XDP support of Open vSwitch is considered 'experimental'.
>>
>>
>>  Introduction
>> @@ -137,6 +136,9 @@ bootstrap/configure the package::
>>
>>    ./boot.sh && ./configure --enable-afxdp
>>
>> +``--enable-afxdp`` here is optional, but it will ensure that all dependencies
>> +are available at the build time.
>> +
>>  Finally, build and install OVS::
>>
>>    make && make install
>> diff --git a/NEWS b/NEWS
>> index 5d39c7d27..d2bbae591 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -2,6 +2,9 @@ Post-v3.0.0
>>  --------------------
>>     - AF_XDP:
>>       * Added support for building with libxdp and libbpf >= 0.7.
>> +     * Support for AF_XDP is now enabled by default if all dependencies are
>> +       available at the build time.  Use --disable-afxdp to disable.
>> +       Use --enable-afxdp to fail the build if dependencies are not present.
>>     - 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 aed01c967..8411c0e6c 100644
>> --- a/acinclude.m4
>> +++ b/acinclude.m4
>> @@ -253,39 +253,72 @@ dnl OVS_CHECK_LINUX_AF_XDP
>>  dnl
>>  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])],
>> -                [], [enable_afxdp=no])
>> +  AC_ARG_ENABLE(
>> +    [afxdp],
>> +    [AS_HELP_STRING([--disable-afxdp], [Disable AF-XDP support])],
>> +    [case "${enableval}" in
>> +       (yes | no | auto) ;;
>> +       (*) AC_MSG_ERROR([bad value ${enableval} for --enable-afxdp]) ;;
>> +     esac],
>> +    [enable_afxdp=auto])
>> +
>>    AC_MSG_CHECKING([whether AF_XDP is enabled])
>> -  if test "$enable_afxdp" != yes; then
>> +  if test "$enable_afxdp" == no; then
>>      AC_MSG_RESULT([no])
>>      AF_XDP_ENABLE=false
>>    else
>> -    AC_MSG_RESULT([yes])
>> +    AC_MSG_RESULT([$enable_afxdp])
>>      AF_XDP_ENABLE=true
>> -
>> -    AC_CHECK_HEADER([bpf/libbpf.h], [],
>> -      [AC_MSG_ERROR([unable to find bpf/libbpf.h for AF_XDP support])])
>> -
>> -    AC_CHECK_HEADER([linux/if_xdp.h], [],
>> -      [AC_MSG_ERROR([unable to find linux/if_xdp.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.])
>> +    failed_dep=none
>> +    dnl Saving libs to restore in case we will end up not building with AF_XDP.
>> +    save_LIBS=$LIBS
>> +
>> +    AC_CHECK_HEADER([bpf/libbpf.h], [], [failed_dep="bpf/libbpf.h"])
>> +
>> +    if test "$failed_dep" = none; then
>> +      AC_CHECK_HEADER([linux/if_xdp.h], [], [failed_dep="linux/if_xdp.h"])
>> +    fi
>> +
>> +    if test "$failed_dep" = none; then
>> +      AC_CHECK_HEADER([xdp/xsk.h],
>> +        AC_DEFINE([HAVE_LIBXDP], [1], [xsk.h is supplied with libxdp]),
>> +        AC_CHECK_HEADER([bpf/xsk.h], [], [failed_dep="xsk.h"]))
>> +    fi
>> +
>> +    if test "$failed_dep" = none; then
>> +      AC_CHECK_FUNCS([pthread_spin_lock], [], [failed_dep="pthread_spin_lock"])
>> +    fi
>> +
>> +    if test "$failed_dep" = none; then
>> +      AC_SEARCH_LIBS([numa_alloc_onnode], [numa], [], [failed_dep="libnuma"])
>> +    fi
>> +    if test "$failed_dep" = none; then
>> +      AC_SEARCH_LIBS([libbpf_strerror], [bpf], [], [failed_dep="libbpf"])
>> +    fi
>> +
>> +    if test "$failed_dep" = none; then
>> +      AC_CHECK_FUNCS([bpf_xdp_query_id bpf_xdp_detach])
>> +      AC_SEARCH_LIBS([libxdp_strerror], [xdp], [],
>> +        [dnl Fail the auto discovery if we have libbpf >= 0.7 but not libxdp
>> +         dnl to avoid deprecation warnings during the build.
>> +         if test "$enable_afxdp" = auto -a "x$ac_cv_func_bpf_xdp_detach" = xyes; then
>> +            failed_dep="libxdp"
>> +         fi
>> +        ])
>> +    fi
>> +
>> +    if test "$failed_dep" = none; then
>> +      AC_DEFINE([HAVE_AF_XDP], [1],
>> +                [Define to 1 if AF_XDP support is available and enabled.])
>> +    elif test "$enable_afxdp" = yes; then
>> +      AC_MSG_ERROR([Missing $failed_dep dependency for AF_XDP support])
>> +    else
>> +      AC_MSG_WARN(m4_normalize(
>> +          [Cannot find $failed_dep, netdev-afxdp will not be supported
>> +           (you may use --disable-afxdp to suppress this warning).]))
>> +      AF_XDP_ENABLE=false
>> +      LIBS=$save_LIBS
>> +    fi
>>    fi
>>    AM_CONDITIONAL([HAVE_AF_XDP], test "$AF_XDP_ENABLE" = true)
>>  ])
>> diff --git a/debian/rules b/debian/rules
>> index 971bc1775..ddbd4dc5c 100755
>> --- a/debian/rules
>> +++ b/debian/rules
>> @@ -23,21 +23,26 @@ override_dh_auto_configure:
>>  	test -d _debian || mkdir _debian
>>  	cd _debian && ( \
>>  		test -e Makefile || \
>> -		../configure --prefix=/usr --localstatedir=/var --enable-ssl \
>> -					 --sysconfdir=/etc \
>> -					 $(DATAPATH_CONFIGURE_OPTS) \
>> -					 $(EXTRA_CONFIGURE_OPTS) \
>> -					 )
>> +		../configure --prefix=/usr --localstatedir=/var \
>> +					--enable-ssl \
>> +					--disable-afxdp \
>> +					--sysconfdir=/etc \
>> +					$(DATAPATH_CONFIGURE_OPTS) \
>> +					$(EXTRA_CONFIGURE_OPTS) \
>> +					)
>>  ifneq (,$(filter i386 amd64 ppc64el arm64, $(DEB_HOST_ARCH)))
>>  ifeq (,$(filter nodpdk, $(DEB_BUILD_OPTIONS)))
>>  	test -d _dpdk || mkdir _dpdk
>>  	cd _dpdk && ( \
>>  		test -e Makefile || \
>> -        ../configure --prefix=/usr --localstatedir=/var --enable-ssl \
>> -                     --with-dpdk=shared --sysconfdir=/etc \
>> -					 $(DATAPATH_CONFIGURE_OPTS) \
>> -					 $(EXTRA_CONFIGURE_OPTS) \
>> -					 )
>> +		../configure --prefix=/usr --localstatedir=/var \
>> +					--enable-ssl \
>> +					--disable-afxdp \
>> +					--with-dpdk=shared \
>> +					--sysconfdir=/etc \
>> +					$(DATAPATH_CONFIGURE_OPTS) \
>> +					$(EXTRA_CONFIGURE_OPTS) \
>> +					)
>>  endif
>>  endif
>>
>> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
>> index 6564d5252..fbfcdcf63 100644
>> --- a/rhel/openvswitch-fedora.spec.in
>> +++ b/rhel/openvswitch-fedora.spec.in
>> @@ -171,6 +171,8 @@ This package provides IPsec tunneling support for OVS tunnels.
>>  %endif
>>  %if %{with afxdp}
>>          --enable-afxdp \
>> +%else
>> +        --disable-afxdp \
>>  %endif
> 
> Now that we build with libxdp, should we also not add some requirements for packages to be installed?
> 
> I do not think we link statically to libxdp/libbpf?

I thought so as well, but it looks like rpm is smart to figure
out on its own which libraries the binary is linked with.  So,
it adds all required runtime dependencies automatically.
Kind of magic.

Otherwise, we already have a lot of missing runtime dependencies
in this spec.

> 
>>          --enable-ssl \
>>          --disable-static \
>> -- 
>> 2.38.1
>
Eelco Chaudron Dec. 20, 2022, 1:25 p.m. UTC | #3 
On 20 Dec 2022, at 14:24, Ilya Maximets wrote:

> On 12/20/22 14:19, Eelco Chaudron wrote:
>>
>>
>> On 19 Dec 2022, at 13:20, Ilya Maximets wrote:
>>
>>> With this change we will try to detect all the netdev-afxdp
>>> dependencies and enable AF_XDP support by default if they are
>>> present at the build time.
>>>
>>> Configuration script behaves in a following way:
>>>
>>>  - ./configure --enable-afxdp
>>>
>>>    Will check for AF_XDP dependencies and fail if they are
>>>    not available.
>>>
>>>  - ./configure --disable-afxdp
>>>
>>>    Disables checking for AF_XDP.  Build will not support
>>>    AF_XDP even if all dependencies are installed.
>>>
>>>  - Just ./configure or ./configure --enable-afxdp=auto
>>>
>>>    Will check for AF_XDP dependencies.  Will print a warning
>>>    if they are not available, but will continue without AF_XDP
>>>    support.  If dependencies are available in a system, this
>>>    option is equal to --enable-afxdp, except that AF_XDP will
>>>    not be enabled for libbpf >= 0.7 if libxdp is not available,
>>>    to avoid deprecation warnings during the build.
>>>
>>> '--disable-afxdp' added to the debian and fedora package builds
>>> to keep predictable behavior.
>>>
>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>>
>> I still don’t like building AF_XDP automatically, but looks like I’m the only one ;)
>>
>>> ---
>>>  Documentation/intro/install/afxdp.rst |  6 +-
>>>  NEWS                                  |  3 +
>>>  acinclude.m4                          | 89 ++++++++++++++++++---------
>>>  debian/rules                          | 25 +++++---
>>>  rhel/openvswitch-fedora.spec.in       |  2 +
>>>  5 files changed, 85 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst
>>> index a4f0b87fe..51c24bf5b 100644
>>> --- a/Documentation/intro/install/afxdp.rst
>>> +++ b/Documentation/intro/install/afxdp.rst
>>> @@ -30,8 +30,7 @@ This document describes how to build and install Open vSwitch using
>>>  AF_XDP netdev.
>>>
>>>  .. warning::
>>> -  The AF_XDP support of Open vSwitch is considered 'experimental',
>>> -  and it is not compiled in by default.
>>> +  The AF_XDP support of Open vSwitch is considered 'experimental'.
>>>
>>>
>>>  Introduction
>>> @@ -137,6 +136,9 @@ bootstrap/configure the package::
>>>
>>>    ./boot.sh && ./configure --enable-afxdp
>>>
>>> +``--enable-afxdp`` here is optional, but it will ensure that all dependencies
>>> +are available at the build time.
>>> +
>>>  Finally, build and install OVS::
>>>
>>>    make && make install
>>> diff --git a/NEWS b/NEWS
>>> index 5d39c7d27..d2bbae591 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -2,6 +2,9 @@ Post-v3.0.0
>>>  --------------------
>>>     - AF_XDP:
>>>       * Added support for building with libxdp and libbpf >= 0.7.
>>> +     * Support for AF_XDP is now enabled by default if all dependencies are
>>> +       available at the build time.  Use --disable-afxdp to disable.
>>> +       Use --enable-afxdp to fail the build if dependencies are not present.
>>>     - 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 aed01c967..8411c0e6c 100644
>>> --- a/acinclude.m4
>>> +++ b/acinclude.m4
>>> @@ -253,39 +253,72 @@ dnl OVS_CHECK_LINUX_AF_XDP
>>>  dnl
>>>  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])],
>>> -                [], [enable_afxdp=no])
>>> +  AC_ARG_ENABLE(
>>> +    [afxdp],
>>> +    [AS_HELP_STRING([--disable-afxdp], [Disable AF-XDP support])],
>>> +    [case "${enableval}" in
>>> +       (yes | no | auto) ;;
>>> +       (*) AC_MSG_ERROR([bad value ${enableval} for --enable-afxdp]) ;;
>>> +     esac],
>>> +    [enable_afxdp=auto])
>>> +
>>>    AC_MSG_CHECKING([whether AF_XDP is enabled])
>>> -  if test "$enable_afxdp" != yes; then
>>> +  if test "$enable_afxdp" == no; then
>>>      AC_MSG_RESULT([no])
>>>      AF_XDP_ENABLE=false
>>>    else
>>> -    AC_MSG_RESULT([yes])
>>> +    AC_MSG_RESULT([$enable_afxdp])
>>>      AF_XDP_ENABLE=true
>>> -
>>> -    AC_CHECK_HEADER([bpf/libbpf.h], [],
>>> -      [AC_MSG_ERROR([unable to find bpf/libbpf.h for AF_XDP support])])
>>> -
>>> -    AC_CHECK_HEADER([linux/if_xdp.h], [],
>>> -      [AC_MSG_ERROR([unable to find linux/if_xdp.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.])
>>> +    failed_dep=none
>>> +    dnl Saving libs to restore in case we will end up not building with AF_XDP.
>>> +    save_LIBS=$LIBS
>>> +
>>> +    AC_CHECK_HEADER([bpf/libbpf.h], [], [failed_dep="bpf/libbpf.h"])
>>> +
>>> +    if test "$failed_dep" = none; then
>>> +      AC_CHECK_HEADER([linux/if_xdp.h], [], [failed_dep="linux/if_xdp.h"])
>>> +    fi
>>> +
>>> +    if test "$failed_dep" = none; then
>>> +      AC_CHECK_HEADER([xdp/xsk.h],
>>> +        AC_DEFINE([HAVE_LIBXDP], [1], [xsk.h is supplied with libxdp]),
>>> +        AC_CHECK_HEADER([bpf/xsk.h], [], [failed_dep="xsk.h"]))
>>> +    fi
>>> +
>>> +    if test "$failed_dep" = none; then
>>> +      AC_CHECK_FUNCS([pthread_spin_lock], [], [failed_dep="pthread_spin_lock"])
>>> +    fi
>>> +
>>> +    if test "$failed_dep" = none; then
>>> +      AC_SEARCH_LIBS([numa_alloc_onnode], [numa], [], [failed_dep="libnuma"])
>>> +    fi
>>> +    if test "$failed_dep" = none; then
>>> +      AC_SEARCH_LIBS([libbpf_strerror], [bpf], [], [failed_dep="libbpf"])
>>> +    fi
>>> +
>>> +    if test "$failed_dep" = none; then
>>> +      AC_CHECK_FUNCS([bpf_xdp_query_id bpf_xdp_detach])
>>> +      AC_SEARCH_LIBS([libxdp_strerror], [xdp], [],
>>> +        [dnl Fail the auto discovery if we have libbpf >= 0.7 but not libxdp
>>> +         dnl to avoid deprecation warnings during the build.
>>> +         if test "$enable_afxdp" = auto -a "x$ac_cv_func_bpf_xdp_detach" = xyes; then
>>> +            failed_dep="libxdp"
>>> +         fi
>>> +        ])
>>> +    fi
>>> +
>>> +    if test "$failed_dep" = none; then
>>> +      AC_DEFINE([HAVE_AF_XDP], [1],
>>> +                [Define to 1 if AF_XDP support is available and enabled.])
>>> +    elif test "$enable_afxdp" = yes; then
>>> +      AC_MSG_ERROR([Missing $failed_dep dependency for AF_XDP support])
>>> +    else
>>> +      AC_MSG_WARN(m4_normalize(
>>> +          [Cannot find $failed_dep, netdev-afxdp will not be supported
>>> +           (you may use --disable-afxdp to suppress this warning).]))
>>> +      AF_XDP_ENABLE=false
>>> +      LIBS=$save_LIBS
>>> +    fi
>>>    fi
>>>    AM_CONDITIONAL([HAVE_AF_XDP], test "$AF_XDP_ENABLE" = true)
>>>  ])
>>> diff --git a/debian/rules b/debian/rules
>>> index 971bc1775..ddbd4dc5c 100755
>>> --- a/debian/rules
>>> +++ b/debian/rules
>>> @@ -23,21 +23,26 @@ override_dh_auto_configure:
>>>  	test -d _debian || mkdir _debian
>>>  	cd _debian && ( \
>>>  		test -e Makefile || \
>>> -		../configure --prefix=/usr --localstatedir=/var --enable-ssl \
>>> -					 --sysconfdir=/etc \
>>> -					 $(DATAPATH_CONFIGURE_OPTS) \
>>> -					 $(EXTRA_CONFIGURE_OPTS) \
>>> -					 )
>>> +		../configure --prefix=/usr --localstatedir=/var \
>>> +					--enable-ssl \
>>> +					--disable-afxdp \
>>> +					--sysconfdir=/etc \
>>> +					$(DATAPATH_CONFIGURE_OPTS) \
>>> +					$(EXTRA_CONFIGURE_OPTS) \
>>> +					)
>>>  ifneq (,$(filter i386 amd64 ppc64el arm64, $(DEB_HOST_ARCH)))
>>>  ifeq (,$(filter nodpdk, $(DEB_BUILD_OPTIONS)))
>>>  	test -d _dpdk || mkdir _dpdk
>>>  	cd _dpdk && ( \
>>>  		test -e Makefile || \
>>> -        ../configure --prefix=/usr --localstatedir=/var --enable-ssl \
>>> -                     --with-dpdk=shared --sysconfdir=/etc \
>>> -					 $(DATAPATH_CONFIGURE_OPTS) \
>>> -					 $(EXTRA_CONFIGURE_OPTS) \
>>> -					 )
>>> +		../configure --prefix=/usr --localstatedir=/var \
>>> +					--enable-ssl \
>>> +					--disable-afxdp \
>>> +					--with-dpdk=shared \
>>> +					--sysconfdir=/etc \
>>> +					$(DATAPATH_CONFIGURE_OPTS) \
>>> +					$(EXTRA_CONFIGURE_OPTS) \
>>> +					)
>>>  endif
>>>  endif
>>>
>>> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
>>> index 6564d5252..fbfcdcf63 100644
>>> --- a/rhel/openvswitch-fedora.spec.in
>>> +++ b/rhel/openvswitch-fedora.spec.in
>>> @@ -171,6 +171,8 @@ This package provides IPsec tunneling support for OVS tunnels.
>>>  %endif
>>>  %if %{with afxdp}
>>>          --enable-afxdp \
>>> +%else
>>> +        --disable-afxdp \
>>>  %endif
>>
>> Now that we build with libxdp, should we also not add some requirements for packages to be installed?
>>
>> I do not think we link statically to libxdp/libbpf?
>
> I thought so as well, but it looks like rpm is smart to figure
> out on its own which libraries the binary is linked with.  So,
> it adds all required runtime dependencies automatically.
> Kind of magic.
>
> Otherwise, we already have a lot of missing runtime dependencies
> in this spec.

Nice :) Then the series looks good to me…

Acked-by: Eelco Chaudron <echaudro@redhat.com>

>>
>>>          --enable-ssl \
>>>          --disable-static \
>>> -- 
>>> 2.38.1
>>
diff mbox series

Patch

diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst
index a4f0b87fe..51c24bf5b 100644
--- a/Documentation/intro/install/afxdp.rst
+++ b/Documentation/intro/install/afxdp.rst
@@ -30,8 +30,7 @@  This document describes how to build and install Open vSwitch using
 AF_XDP netdev.
 
 .. warning::
-  The AF_XDP support of Open vSwitch is considered 'experimental',
-  and it is not compiled in by default.
+  The AF_XDP support of Open vSwitch is considered 'experimental'.
 
 
 Introduction
@@ -137,6 +136,9 @@  bootstrap/configure the package::
 
   ./boot.sh && ./configure --enable-afxdp
 
+``--enable-afxdp`` here is optional, but it will ensure that all dependencies
+are available at the build time.
+
 Finally, build and install OVS::
 
   make && make install
diff --git a/NEWS b/NEWS
index 5d39c7d27..d2bbae591 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,9 @@  Post-v3.0.0
 --------------------
    - AF_XDP:
      * Added support for building with libxdp and libbpf >= 0.7.
+     * Support for AF_XDP is now enabled by default if all dependencies are
+       available at the build time.  Use --disable-afxdp to disable.
+       Use --enable-afxdp to fail the build if dependencies are not present.
    - 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 aed01c967..8411c0e6c 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -253,39 +253,72 @@  dnl OVS_CHECK_LINUX_AF_XDP
 dnl
 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])],
-                [], [enable_afxdp=no])
+  AC_ARG_ENABLE(
+    [afxdp],
+    [AS_HELP_STRING([--disable-afxdp], [Disable AF-XDP support])],
+    [case "${enableval}" in
+       (yes | no | auto) ;;
+       (*) AC_MSG_ERROR([bad value ${enableval} for --enable-afxdp]) ;;
+     esac],
+    [enable_afxdp=auto])
+
   AC_MSG_CHECKING([whether AF_XDP is enabled])
-  if test "$enable_afxdp" != yes; then
+  if test "$enable_afxdp" == no; then
     AC_MSG_RESULT([no])
     AF_XDP_ENABLE=false
   else
-    AC_MSG_RESULT([yes])
+    AC_MSG_RESULT([$enable_afxdp])
     AF_XDP_ENABLE=true
-
-    AC_CHECK_HEADER([bpf/libbpf.h], [],
-      [AC_MSG_ERROR([unable to find bpf/libbpf.h for AF_XDP support])])
-
-    AC_CHECK_HEADER([linux/if_xdp.h], [],
-      [AC_MSG_ERROR([unable to find linux/if_xdp.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.])
+    failed_dep=none
+    dnl Saving libs to restore in case we will end up not building with AF_XDP.
+    save_LIBS=$LIBS
+
+    AC_CHECK_HEADER([bpf/libbpf.h], [], [failed_dep="bpf/libbpf.h"])
+
+    if test "$failed_dep" = none; then
+      AC_CHECK_HEADER([linux/if_xdp.h], [], [failed_dep="linux/if_xdp.h"])
+    fi
+
+    if test "$failed_dep" = none; then
+      AC_CHECK_HEADER([xdp/xsk.h],
+        AC_DEFINE([HAVE_LIBXDP], [1], [xsk.h is supplied with libxdp]),
+        AC_CHECK_HEADER([bpf/xsk.h], [], [failed_dep="xsk.h"]))
+    fi
+
+    if test "$failed_dep" = none; then
+      AC_CHECK_FUNCS([pthread_spin_lock], [], [failed_dep="pthread_spin_lock"])
+    fi
+
+    if test "$failed_dep" = none; then
+      AC_SEARCH_LIBS([numa_alloc_onnode], [numa], [], [failed_dep="libnuma"])
+    fi
+    if test "$failed_dep" = none; then
+      AC_SEARCH_LIBS([libbpf_strerror], [bpf], [], [failed_dep="libbpf"])
+    fi
+
+    if test "$failed_dep" = none; then
+      AC_CHECK_FUNCS([bpf_xdp_query_id bpf_xdp_detach])
+      AC_SEARCH_LIBS([libxdp_strerror], [xdp], [],
+        [dnl Fail the auto discovery if we have libbpf >= 0.7 but not libxdp
+         dnl to avoid deprecation warnings during the build.
+         if test "$enable_afxdp" = auto -a "x$ac_cv_func_bpf_xdp_detach" = xyes; then
+            failed_dep="libxdp"
+         fi
+        ])
+    fi
+
+    if test "$failed_dep" = none; then
+      AC_DEFINE([HAVE_AF_XDP], [1],
+                [Define to 1 if AF_XDP support is available and enabled.])
+    elif test "$enable_afxdp" = yes; then
+      AC_MSG_ERROR([Missing $failed_dep dependency for AF_XDP support])
+    else
+      AC_MSG_WARN(m4_normalize(
+          [Cannot find $failed_dep, netdev-afxdp will not be supported
+           (you may use --disable-afxdp to suppress this warning).]))
+      AF_XDP_ENABLE=false
+      LIBS=$save_LIBS
+    fi
   fi
   AM_CONDITIONAL([HAVE_AF_XDP], test "$AF_XDP_ENABLE" = true)
 ])
diff --git a/debian/rules b/debian/rules
index 971bc1775..ddbd4dc5c 100755
--- a/debian/rules
+++ b/debian/rules
@@ -23,21 +23,26 @@  override_dh_auto_configure:
 	test -d _debian || mkdir _debian
 	cd _debian && ( \
 		test -e Makefile || \
-		../configure --prefix=/usr --localstatedir=/var --enable-ssl \
-					 --sysconfdir=/etc \
-					 $(DATAPATH_CONFIGURE_OPTS) \
-					 $(EXTRA_CONFIGURE_OPTS) \
-					 )
+		../configure --prefix=/usr --localstatedir=/var \
+					--enable-ssl \
+					--disable-afxdp \
+					--sysconfdir=/etc \
+					$(DATAPATH_CONFIGURE_OPTS) \
+					$(EXTRA_CONFIGURE_OPTS) \
+					)
 ifneq (,$(filter i386 amd64 ppc64el arm64, $(DEB_HOST_ARCH)))
 ifeq (,$(filter nodpdk, $(DEB_BUILD_OPTIONS)))
 	test -d _dpdk || mkdir _dpdk
 	cd _dpdk && ( \
 		test -e Makefile || \
-        ../configure --prefix=/usr --localstatedir=/var --enable-ssl \
-                     --with-dpdk=shared --sysconfdir=/etc \
-					 $(DATAPATH_CONFIGURE_OPTS) \
-					 $(EXTRA_CONFIGURE_OPTS) \
-					 )
+		../configure --prefix=/usr --localstatedir=/var \
+					--enable-ssl \
+					--disable-afxdp \
+					--with-dpdk=shared \
+					--sysconfdir=/etc \
+					$(DATAPATH_CONFIGURE_OPTS) \
+					$(EXTRA_CONFIGURE_OPTS) \
+					)
 endif
 endif
 
diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 6564d5252..fbfcdcf63 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -171,6 +171,8 @@  This package provides IPsec tunneling support for OVS tunnels.
 %endif
 %if %{with afxdp}
         --enable-afxdp \
+%else
+        --disable-afxdp \
 %endif
         --enable-ssl \
         --disable-static \