[ovs-dev] [PATCH dpdk-latest v1] build: Remove DPDK make build references.
Ilya Maximets
i.maximets at ovn.org
Wed Nov 18 16:56:12 UTC 2020
On 11/17/20 5:38 PM, Stokes, Ian wrote:
>> Building DPDK using Make is removed since DPDK 20.08.
>> Hence, remove its references in OVS as well.
>> While at it, address few comments on the commit [1].
>>
>
> Thanks for the patch, just a few comments below.
>
> @Ilya, I think Sunil has addressed most of the concerns you flagged, however I'd like your opinion on the python script?
Most of the commants are covered, but I didn't check if this patch
cleans everything that needs to be cleaned. Only checked what is
in the patch. Few comments inline.
>
> I'd like to have this patch merged to dpdk-latest before creating our RFC patch to move OVS master to use 20.11 so it would be nice to have this reviewed/reworked so as to include the changes.
>
>> [1] 540e70fba6d5 ("build: Add support for DPDK meson build")
> Probably going forward it's better to use the Fixes tag, so
>
> Fixes: 540e70fba6d5 ("build: Add support for DPDK meson build")
>>
>> Tested-at: https://travis-ci.org/github/Sunil-Pai-G/ovs/builds/742699277
>> Signed-off-by: Sunil Pai G <sunil.pai.g at intel.com>
>> ---
>
> A summary of the changes would be good hear, they will be stripped off when be applied.
> Although in the case it is a new patch rather than part of a revision so I understand why
> You ma not have put it here.
>
>> .travis/linux-build.sh | 4 +-
>> Documentation/intro/install/afxdp.rst | 2 +-
>> Documentation/intro/install/dpdk.rst | 73 ++++++++-----------------
>> acinclude.m4 | 78 ++++++++-------------------
>> python/automake.mk | 1 +
>> python/build/pkgcfg.py | 24 +++++++--
>> 6 files changed, 65 insertions(+), 117 deletions(-)
>>
>> diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
>> index 917bbf962..e085780e9 100755
>> --- a/.travis/linux-build.sh
>> +++ b/.travis/linux-build.sh
>> @@ -145,12 +145,12 @@ function install_dpdk()
>>
>> CC=gcc meson $DPDK_OPTS build
>> ninja -C build
>> - sudo ninja -C build install
>> + ninja -C build install
>>
>> # Update the library paths.
>> sudo ldconfig
>>
>> - echo "Installed DPDK source"
>> + echo "Installed DPDK source in $(pwd)"
>> popd
>> echo "${DPDK_VER}" > ${VERSION_FILE}
>> }
>> diff --git a/Documentation/intro/install/afxdp.rst
>> b/Documentation/intro/install/afxdp.rst
>> index 327f2b3df..aad0aeebe 100644
>> --- a/Documentation/intro/install/afxdp.rst
>> +++ b/Documentation/intro/install/afxdp.rst
>> @@ -396,7 +396,7 @@ PVP using vhostuser device
>> --------------------------
>> First, build OVS with DPDK and AFXDP::
>>
>> - ./configure --enable-afxdp --with-dpdk=shared|static|<dpdk path>
>> + ./configure --enable-afxdp --with-dpdk=shared|static
>> make -j4 && make install
>>
>> Create a vhost-user port from OVS::
>> diff --git a/Documentation/intro/install/dpdk.rst
>> b/Documentation/intro/install/dpdk.rst
>> index 7a1852bc5..ecc4c7931 100644
>> --- a/Documentation/intro/install/dpdk.rst
>> +++ b/Documentation/intro/install/dpdk.rst
>> @@ -82,60 +82,39 @@ Install DPDK
>>
>> Meson is the preferred tool to build recent DPDK releases
>> as Make support is deprecated and will be removed from DPDK 20.11.
It's already removed, IIUC. We shouldn't talk about it in the future tense.
Also it's not "preferred", but the only tool.
>> - OVS supports DPDK Meson builds from DPDK 19.11 onwards.
>>
>> Build and install the DPDK library::
>>
>> - $ export DPDK_TARGET=x86_64-native-linuxapp-gcc
>> - $ export DPDK_BUILD=$DPDK_DIR/$DPDK_TARGET
>> - $ meson $DPDK_TARGET
>> - $ ninja -C $DPDK_TARGET
>> - $ sudo ninja -C $DPDK_TARGET install
>> + $ export DPDK_BUILD=$DPDK_DIR/build
>> + $ meson build
>> + $ ninja -C build
>> + $ sudo ninja -C build install
>> $ sudo ldconfig
>>
>> Detailed information can be found at `DPDK documentation`_.
>>
>> -#. (Optional) Configure DPDK as a shared library
>> +#. (Optional) Configure and export the DPDK shared library location
>>
>> - When using Meson, DPDK is built both as static and shared library.
>> - So no extra configuration is required in this case.
>> + Since DPDK is built both as static and shared library by default, no extra
>> + configuration is required for the build.
>>
>> - In case of Make, DPDK can be built as either a static library or a shared
>> - library. By default, it is configured for the former. If you wish to use
>> - the latter, set
>> - ``CONFIG_RTE_BUILD_SHARED_LIB=y`` in
>> ``$DPDK_DIR/config/common_base``.
>> + Exporting the path to library is not necessary if the DPDK libraries are
>> + system installed. For libraries installed using a prefix
>> + (assuming $DPDK_INSTALL in the below case), export the path to this
>> + library and also update the $PKG_CONFIG_PATH for use before building OVS::
>> +
>> + $ export $DPDK_LIB=$DPDK_INSTALL/lib/x86_64-linux-gnu
>> + $ export LD_LIBRARY_PATH=$DPDK_LIB/:$LD_LIBRARY_PATH
>> + $ export PKG_CONFIG_PATH=$DPDK_BUILD/lib/x86_64-linux-
>> gnu/pkgconfig/
What is $DPDK_BUILD? Should be $DPDK_INSTALL?
>>
>> .. note::
>>
>> Minor performance loss is expected when using OVS with a shared DPDK
>> library compared to a static DPDK library.
>>
>> -#. Configure and install DPDK using Make
>> -
>> - Build and install the DPDK library::
>> -
>> - $ export DPDK_TARGET=x86_64-native-linuxapp-gcc
>> - $ export DPDK_BUILD=$DPDK_DIR/$DPDK_TARGET
>> - $ make install T=$DPDK_TARGET DESTDIR=install
>> -
>> -#. (Optional) Export the DPDK shared library location
>> -
>> - If DPDK was built as a shared library using Make, export the path to this
>> - library for use when building OVS::
>> -
>> - $ export LD_LIBRARY_PATH=$DPDK_DIR/x86_64-native-linuxapp-gcc/lib
>> -
>> - In case of Meson, exporting the path to library is not necessary if
>> - the DPDK libraries are system installed. For libraries installed using
>> - a prefix(assuming $DPDK_INSTALL in the below case), export the path to this
>> - library and also update the $PKG_CONFIG_PATH for use before building OVS::
>> -
>> - $ export $DPDK_LIB=$DPDK_INSTALL/lib/x86_64-linux-gnu
>> - $ export LD_LIBRARY_PATH=$DPDK_LIB/:$LD_LIBRARY_PATH
>> - $ export PKG_CONFIG_PATH=$DPDK_LIB/pkgconfig/:$PKG_CONFIG_PATH
>> -
>> .. _DPDK sources: http://dpdk.org/rel
>> -.. _DPDK documentation:
>> https://doc.dpdk.org/guides/linux_gsg/build_dpdk.html
>> +.. _DPDK documentation:
>> + https://doc.dpdk.org/guides-20.08/linux_gsg/build_dpdk.html
>
> This references the DPDK 20.08 documentation, I think it should be 20.11?
I guess 20.11 docs are not yet avaialble, but, yes, it should be 20.11.
>
>>
>> Install OVS
>> ~~~~~~~~~~~
>> @@ -156,24 +135,14 @@ has to be configured to build against the DPDK library
>> (``--with-dpdk``).
>>
>> #. Configure the package using the ``--with-dpdk`` flag:
>>
>> - Depending on the tool used to build DPDK and the type of
>> - DPDK library to use, one can configure OVS as follows:
>> -
>> - When DPDK is built using Meson, and OVS must consume DPDK shared
>> libraries
>> - (also equivalent to leaving --with-dpdk option empty)::
>> -
>> - $ ./configure --with-dpdk=shared
>> -
>> - When DPDK is built using Meson, and OVS must consume DPDK static
>> libraries::
>> + If OVS must consume DPDK static libraries::
There should be no '::'.
>> + (also equivalent to --with-dpdk=yes )::
It should be quoted, i.e. ``--with-dpdk=yes``.
>>
>> $ ./configure --with-dpdk=static
>>
>> - When DPDK is built using Make(for shared or static)::
>> + If OVS must consume DPDK shared libraries::
>>
>> - $ ./configure --with-dpdk=$DPDK_BUILD
>> -
>> - where ``DPDK_BUILD`` is the path to the built DPDK library. This can be
>> - skipped if DPDK library is installed in its default location.
>> + $ ./configure --with-dpdk=shared
>>
>> .. note::
>> While ``--with-dpdk`` is required, you can pass any other configuration
>> diff --git a/acinclude.m4 b/acinclude.m4
>> index 061afda4e..3479c5010 100644
>> --- a/acinclude.m4
>> +++ b/acinclude.m4
>> @@ -334,10 +334,9 @@ dnl
>> dnl Configure DPDK source tree
>> AC_DEFUN([OVS_CHECK_DPDK], [
>> AC_ARG_WITH([dpdk],
>> - [AC_HELP_STRING([--with-dpdk=static|shared|/path/to/dpdk],
>> + [AC_HELP_STRING([--with-dpdk=static|shared|yes],
>> [Specify "static" or "shared" depending on the
>> - DPDK libraries to use only if built using Meson
>> - OR the DPDK build directory in case of Make])],
>> + DPDK libraries to use])],
>> [have_dpdk=true])
>>
>> AC_MSG_CHECKING([whether dpdk is enabled])
>> @@ -347,46 +346,25 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>> else
>> AC_MSG_RESULT([yes])
>> case "$with_dpdk" in
>> - "shared" | "static" | "yes")
>> - DPDK_AUTO_DISCOVER="true"
>> - case "$with_dpdk" in
>> - "shared" | "yes")
>> - PKG_CHECK_MODULES([DPDK], [libdpdk], [
>> - DPDK_INCLUDE="$DPDK_CFLAGS"
>> - DPDK_LIB="$DPDK_LIBS"], [
>> - DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk"
>> - DPDK_LIB="-ldpdk"])
>> - ;;
>> - "static")
>> - PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk], [
>> - DPDK_INCLUDE="$DPDK_CFLAGS"
>> - DPDK_LIB="$DPDK_LIBS"], [
>> - DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk"
>> - DPDK_LIB="-ldpdk"])
>> - ;;
>> - esac
>> - ;;
>> - *)
>> - DPDK_AUTO_DISCOVER="false"
>> - DPDK_INCLUDE_PATH="$with_dpdk/include"
>> - # If 'with_dpdk' is passed install directory, point to headers
>> - # installed in $DESTDIR/$prefix/include/dpdk
>> - if test -e "$DPDK_INCLUDE_PATH/rte_config.h"; then
>> - DPDK_INCLUDE="-I$DPDK_INCLUDE_PATH"
>> - elif test -e "$DPDK_INCLUDE_PATH/dpdk/rte_config.h"; then
>> - DPDK_INCLUDE="-I$DPDK_INCLUDE_PATH/dpdk"
>> - fi
>> - DPDK_LIB_DIR="$with_dpdk/lib"
>> - DPDK_LIB="-ldpdk"
>> - ;;
>> + "shared")
>> + PKG_CHECK_MODULES([DPDK], [libdpdk], [
>> + DPDK_INCLUDE="$DPDK_CFLAGS"
>> + DPDK_LIB="$DPDK_LIBS"], [
>> + DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk"
>> + DPDK_LIB="-ldpdk"])
>> + ;;
>> + "static" | "yes")
>> + PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk], [
>> + DPDK_INCLUDE="$DPDK_CFLAGS"
>> + DPDK_LIB="$DPDK_LIBS"], [
>> + DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk"
>> + DPDK_LIB="-ldpdk"])
>> + ;;
>> esac
>>
>> ovs_save_CFLAGS="$CFLAGS"
>> ovs_save_LDFLAGS="$LDFLAGS"
>> CFLAGS="$CFLAGS $DPDK_INCLUDE"
>> - if test "$DPDK_AUTO_DISCOVER" = "false"; then
>> - LDFLAGS="$LDFLAGS -L${DPDK_LIB_DIR}"
>> - fi
>>
>> AC_CHECK_HEADERS([rte_config.h], [], [
>> AC_MSG_ERROR([unable to find rte_config.h in $with_dpdk])
>> @@ -435,21 +413,14 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>> [AC_MSG_RESULT([yes])
>> DPDKLIB_FOUND=true],
>> [AC_MSG_RESULT([no])
>> - if test "$DPDK_AUTO_DISCOVER" = "true"; then
>> - AC_MSG_ERROR(m4_normalize([
>> - Could not find DPDK library in default search path, update
>> - PKG_CONFIG_PATH for pkg-config to find the .pc file in
>> - non-standard location]))
>> - else
>> - AC_MSG_ERROR([Could not find DPDK libraries in $DPDK_LIB_DIR])
>> - fi
>> + AC_MSG_ERROR(m4_normalize([
>> + Could not find DPDK library in default search path, update
>> + PKG_CONFIG_PATH for pkg-config to find the .pc file in
>> + non-standard location]))
>> ])
>>
>> CFLAGS="$ovs_save_CFLAGS"
>> LDFLAGS="$ovs_save_LDFLAGS"
>> - if test "$DPDK_AUTO_DISCOVER" = "false"; then
>> - OVS_LDFLAGS="$OVS_LDFLAGS -L$DPDK_LIB_DIR"
>> - fi
>> OVS_CFLAGS="$OVS_CFLAGS $DPDK_INCLUDE"
>> OVS_ENABLE_OPTION([-mssse3])
>>
>> @@ -462,14 +433,7 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>> # These options are specified inside a single -Wl directive to prevent
>> # autotools from reordering them.
>> #
>> - # OTOH newer versions of dpdk pkg-config (generated with Meson)
>> - # will already have flagged just the right set of libs with
>> - # --whole-archive - in those cases do not wrap it once more.
>> - if [[ "$pkg_failed" != "no" ]];then
>> - DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-
>> archive
>> - else
>> - DPDK_vswitchd_LDFLAGS=`python3 ${srcdir}/python/build/pkgcfg.py
>> $DPDK_LIB`
>> - fi
>> + DPDK_vswitchd_LDFLAGS=`python3 ${srcdir}/python/build/pkgcfg.py
We should probably use $PYTHON3 variable here, as we're actualy don not
know if 'python3' is avaialble, or what version of python user specified.
>> $DPDK_LIB`
>>
>> AC_SUBST([DPDK_vswitchd_LDFLAGS])
>> AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.])
>> diff --git a/python/automake.mk b/python/automake.mk
>> index 69d9800f9..f805e1a17 100644
>> --- a/python/automake.mk
>> +++ b/python/automake.mk
>> @@ -68,6 +68,7 @@ FLAKE8_PYFILES += \
>> python/setup.py \
>> python/build/__init__.py \
>> python/build/nroff.py \
>> + python/build/pkgcfg.py \
>> python/ovs/dirs.py.template
>>
>> nobase_pkgdata_DATA = $(ovs_pyfiles) $(ovstest_pyfiles)
>> diff --git a/python/build/pkgcfg.py b/python/build/pkgcfg.py
>> index 7cee3cb03..8401df4d2 100644
>> --- a/python/build/pkgcfg.py
>> +++ b/python/build/pkgcfg.py
>> @@ -1,3 +1,5 @@
>> +#!/usr/bin/env python3
>> +
>> # Copyright (c) 2020 Intel, Inc.
>> #
>> # Licensed under the Apache License, Version 2.0 (the "License")
>> @@ -12,19 +14,31 @@
>> # See the License for the specific language governing permissions and
>> # Limitations under the License.
>>
>> -# The purpose of this script is to parse the libraries
>> -# From pkg-config in case of DPDK Meson builds.
>> +# This script wraps the DPDK libraries inside a single -Wl directive
>> +# after comma separation to prevent autotools from reordering them.
>> +# Ex: -L/usr/local/lib/x86_64-linux-gnu
>> +# -Wl,--whole-archive <libs>
>> +# -Wl,--no-whole-archive <libs>
>> +#
>> +# is changed to :
>> +#
>> +# -L/usr/local/lib/x86_64-linux-gnu
>> +# -Wl,--whole-archive,<comma separated libs>
>> +# --no-whole-archive,<comma separated libs>
>>
>
> I don't have an issue with above, I think the comment explains the purpose, however Ilya might have a different approach without the script?
Script is fine, but we likely want it in build-aux directory
instead of python/build as it's not usable for anything else
beside this particular case.
On the other hand, this looks like a very basic string processing
that could be easily done with tr and sed wihout involving
python related complications. Maybe something lke this:
# Ld only understands -lpthread.
DPDK_LIB=$(echo "$DPDK_LIB" | sed 's/-pthread/-lpthread/')
pattern='\(.*\)-Wl,--whole-archive \(.*\) -Wl,--no-whole-archive\(.*\)'
whole_archive=$(echo "$DPDK_LIB" | sed "s/$pattern/\2/" | tr ' ' ',')
DPDK_vswitchd_LDFLAGS=$(echo "$DPDK_LIB" | \
sed "s/$pattern/\1 -Wl,--whole-archive,$whole_archive,--no-whole-archive \3/")
?
>
> Regards
> Ian
>
>
>> import sys
>> +
>> +
>> def parse_pkg_cfg_libs(arg):
>> linker_prefix = "-Wl,"
>> # Libtool expects libraries to be comma separated
>> # And -Wl must appear only once.
>> - final_string = ','.join(map(str.strip,arg[1:])).replace('-Wl,','')
>> - final_string = arg[0]+" "+linker_prefix+final_string
>> + final_string = ','.join(map(str.strip, arg[1:])).replace('-Wl,', '')
>> + final_string = arg[0] + " " + linker_prefix + final_string
>> # Ld only understands -lpthread.
>> - final_string = final_string.replace('-pthread','-lpthread')
>> + final_string = final_string.replace('-pthread', '-lpthread')
>> return final_string
>>
>> +
>> if __name__ == "__main__":
>> print(parse_pkg_cfg_libs(sys.argv[1:]))
>> --
>> 2.17.1
>
More information about the dev
mailing list