[ovs-dev] [PATCH dpdk-latest v4] build: Add support for DPDK meson build.

Pai G, Sunil sunil.pai.g at intel.com
Tue Nov 10 06:52:35 UTC 2020


Hi Ilya , 

Thank you for the comments , please see response inline.

> -----Original Message-----
> From: Ilya Maximets <i.maximets at ovn.org>
> Sent: Tuesday, November 10, 2020 2:27 AM
> To: Pai G, Sunil <sunil.pai.g at intel.com>; dev at openvswitch.org
> Cc: Stokes, Ian <ian.stokes at intel.com>; i.maximets at ovn.org;
> david.marchand at redhat.com; Richardson, Bruce
> <bruce.richardson at intel.com>; christian.ehrhardt at canonical.com;
> i.maximets at ovn.org; Kevin Traynor <ktraynor at redhat.com>
> Subject: Re: [PATCH dpdk-latest v4] build: Add support for DPDK meson
> build.
> 
> On 9/2/20 8:06 PM, Sunil Pai G wrote:
> > Make based build is deprecated in DPDK. Meson based build to be used
> > for future DPDK releases.
> >
> > This updates travis, configure script and documentation for using DPDK
> > Meson with OVS.
> >
> > Tested-at:
> > https://travis-ci.org/github/Sunil-Pai-G/ovs-copy/builds/723510063
> > Signed-off-by: Sunil Pai G <sunil.pai.g at intel.com>
> > ---
> 
> Sorry for delay.  Following up with things that I think should be fixed before
> this patch merged to master.

Not a problem :)

> 
> > v3->v4:
> > - Fix checkpatch errors
> >
> > v2->v3:
> > - Update Documentation for vhost-user
> >
> > v1->v2:
> > - Update Documentation
> > - Simplify the pkg-config parsing script
> > - Rename and move the pkg-config parsing script to python dir
> > - Update travis to:
> >    - install DPDK to cached dir
> >    - disable DPDK tests
> >    - removed fPIC flag for DPDK
> >    - removed cross compilation for aarch64
> > ---
> >  .travis.yml                              |  3 ++
> >  .travis/linux-build.sh                   | 39 ++++++++++-----
> >  .travis/linux-prepare.sh                 |  1 +
> >  Documentation/intro/install/afxdp.rst    |  2 +-
> >  Documentation/intro/install/dpdk.rst     | 63 ++++++++++++++++++++----
> >  Documentation/topics/dpdk/vhost-user.rst | 18 +------
> >  acinclude.m4                             | 44 +++++++++++------
> >  python/automake.mk                       |  3 +-
> >  python/build/pkgcfg.py                   | 30 +++++++++++
> >  9 files changed, 149 insertions(+), 54 deletions(-)  create mode
> > 100644 python/build/pkgcfg.py
> >
> > diff --git a/.travis.yml b/.travis.yml index 3dd5d1d23..a8f9a4d79
> > 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -27,6 +27,9 @@ addons:
> >        - selinux-policy-dev
> >        - libunbound-dev
> >        - libunwind-dev
> > +      - python3-setuptools
> > +      - python3-wheel
> > +      - ninja-build
> >
> >
<snipped>

> > +    CC=gcc meson $DPDK_OPTS build
> > +    ninja -C build
> > +    sudo ninja -C build install
> 
> Why we need 'sudo' here?  We're installing to a local folder, should work
> without 'sudo', IIUC.

Sure , Will try removing the sudo.

> 
> > +
> > +    # Update the library paths.
> > +    sudo ldconfig
> >
> > -    make -j4 CC=gcc EXTRA_CFLAGS='-fPIC'
> > -    EXTRA_OPTS="$EXTRA_OPTS --with-dpdk=$(pwd)/build"
> > -    echo "Installed DPDK source in $(pwd)"
> > +    echo "Installed DPDK source"
> 
> It's probably better to keep this line as is.

Sure.

> 
> >      popd
> >      echo "${DPDK_VER}" > ${VERSION_FILE}  } diff --git
> > a/.travis/linux-prepare.sh b/.travis/linux-prepare.sh index
> > 71eb347e8..1baa11641 100755
> > --- a/.travis/linux-prepare.sh
> > +++ b/.travis/linux-prepare.sh
> > @@ -22,6 +22,7 @@ cd ..
> >
> >  pip3 install --disable-pip-version-check --user flake8 hacking
> >  pip3 install --user --upgrade docutils
> > +pip3 install --user  'meson==0.47.1'
> >
> >  if [ "$M32" ]; then
> >      # Installing 32-bit libraries.
> > diff --git a/Documentation/intro/install/afxdp.rst
> > b/Documentation/intro/install/afxdp.rst
> > index 3c8f78825..327f2b3df 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=<dpdk path>
> > +  ./configure  --enable-afxdp --with-dpdk=shared|static|<dpdk path>
> 
> There should be no '<dpdk path>' option.  See below.
Sure .

> 
> >    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 39544f835..cd7e51c75 100644
> > --- a/Documentation/intro/install/dpdk.rst
> > +++ b/Documentation/intro/install/dpdk.rst
> > @@ -62,6 +62,8 @@ Detailed system requirements can be found at `DPDK
> requirements`_.
> >  .. _DPDK supported NIC: http://dpdk.org/doc/nics  .. _DPDK
> > requirements: http://dpdk.org/doc/guides/linux_gsg/sys_reqs.html
> >
> > +.. _dpdk-install:
> > +
> >  Installing
> >  ----------
> >
> > @@ -76,10 +78,31 @@ Install DPDK
> >         $ export DPDK_DIR=/usr/src/dpdk-stable-19.11.2
> >         $ cd $DPDK_DIR
> >
> > +#. Configure and install DPDK using Meson
> > +
> > +   Meson is the preferred tool to build recent DPDK releases
> > +   as Make support is deprecated and will be removed from DPDK 20.11.
> > +   OVS supports DPDK Meson builds from DPDK 19.11 onwards.
> 
> Do we support meson builds with 19.11?  I'm not sure that it works,
> otherwise why we need this patch?

We do, but it doesn’t work as expected.
Christian had added the initial support. 

> 
> We support builds with DPDK libs provided by pkg-config, but that doesn't
> mean that we support build with meson.  IIRC, it's not possible to link agains
> DPDK built with meson unless you're crafting a combined library by hands.  I
> can't name this as 'support'.
> 

Sure . 

> > +
> > +   Build and install the DPDK library::
> > +
> > +       $ export DPDK_TARGET=x86_64-native-linuxapp-gcc
> > +       $ export DPDK_BUILD=$DPDK_DIR/$DPDK_TARGET
> > +       $ meson $DPDK_TARGET
> 
> This looks confusing.  Makes an impression that this actually affects target
> you build for.  But that is just a name of a folder and build will be for your
> current architecture with a default compiler.
> We should not use word 'TARGET' here as it is misleading.
> 

Sure , let me remove the DPDK_TARGET and have something like below:
$ export DPDK_BUILD=$DPDK_DIR/build


> > +       $ ninja -C $DPDK_TARGET
> > +       $ sudo ninja -C $DPDK_TARGET install
> > +       $ sudo ldconfig
> > +
> > +   Detailed information can be found at `DPDK documentation`_.
> > +
> >  #. (Optional) Configure DPDK as a shared library
> >
> > -   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
> > +   When using Meson, DPDK is built both as static and shared library.
> > +   So no extra configuration is required in this case.
> > +
> > +   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``.
> 
> 'make' build system is no longe a way to build DPDK.  We should remove
> support of build with 'make' along with documentation that describes how to
> do that.
> 

Sure , will remove make documentation.

> >
> >     .. note::
> > @@ -87,7 +110,7 @@ Install DPDK
> >        Minor performance loss is expected when using OVS with a shared
> DPDK
> >        library compared to a static DPDK library.
> >
> > -#. Configure and install DPDK
> > +#. Configure and install DPDK using Make
> 
> Same point.  This section of a document should just be removed.

Sure.

> 
> >
> >     Build and install the DPDK library::
> >
> > @@ -97,12 +120,22 @@ Install DPDK
> >
> >  #. (Optional) Export the DPDK shared library location
> >
> > -   If DPDK was built as a shared library, export the path to this library for
> > -   use when building OVS::
> > +   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
> 
> This should be versioned link, i.e. link to 20.11 version of docs.

Sure , will link to 20.11 version.

> 
> >
> >  Install OVS
> >  ~~~~~~~~~~~
> > @@ -121,17 +154,27 @@ has to be configured to build against the DPDK
> library (``--with-dpdk``).
> >
> >  #. Bootstrap, if required, as described in
> > :ref:`general-bootstrapping`
> >
> > -#. Configure the package using the ``--with-dpdk`` flag::
> > +#. 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)::
> 
> Default option should be 'static', i.e. empty option should mean 'static'.
> Anyway, code below has a switch between 'static', 'shared' and 'yes'.  There
> is no empty option.

The empty option defaults to "yes" is what I found IIRC.

Regarding the default option being static ,
Christian's initial support had a bug which was causing the
DPDK shared libraries to be used instead of static ones though
the intention was to use the static DPDK libs.
This was because of the libtool re arranging the --whole-archive and --no-whole-archive flags.
In order to fix this, I added the pkgcfg.py file below.

I didn’t want to break this existing behavior although faulty.
I wouldn’t mind changing it to static. Let me know your thoughts.


> 
> > +
> > +       $ ./configure --with-dpdk=shared
> > +
> > +   When DPDK is built using Meson, and OVS must consume DPDK static
> libraries::
> > +
> > +       $ ./configure --with-dpdk=static
> > +
<snipped>

> > +DPDK sources to VM and build DPDK as described in :ref:`dpdk-install`.
> >
> >  Setup huge pages and DPDK devices using UIO::
> >
> > diff --git a/acinclude.m4 b/acinclude.m4 index 84f344da0..412b2dd55
> > 100644
> > --- a/acinclude.m4
> > +++ b/acinclude.m4
> > @@ -334,8 +334,10 @@ dnl
> >  dnl Configure DPDK source tree
> >  AC_DEFUN([OVS_CHECK_DPDK], [
> >    AC_ARG_WITH([dpdk],
> > -              [AC_HELP_STRING([--with-dpdk=/path/to/dpdk],
> > -                              [Specify the DPDK build directory])],
> > +
> > + [AC_HELP_STRING([--with-dpdk=static|shared|/path/to/dpdk],
> 
> static|shared|yes

Sure 

> 
> > +                              [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])],
> >                [have_dpdk=true])
> >
> >    AC_MSG_CHECKING([whether dpdk is enabled]) @@ -345,13 +347,24
> @@
> > AC_DEFUN([OVS_CHECK_DPDK], [
> >    else
> >      AC_MSG_RESULT([yes])
> >      case "$with_dpdk" in
> > -      yes)
> > +      "shared" | "static" | "yes")
> >          DPDK_AUTO_DISCOVER="true"
> > -        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"])
> > +        case "$with_dpdk" in
> > +          "shared" | "yes")
> 
> "yes" should mean "static" as it was before. See
> PKG_CHECK_MODULES_STATIC() above.

Sure.

> 
> > +             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"
> 
> All the 'DPDK_AUTO_DISCOVER="false"'-related code should be removed
> along with support for 'make' builds.

Sure 

> 
> > @@ -424,8 +437,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
> >        [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, Use --with-dpdk
> > -            to specify the DPDK library installed in non-standard location]))
> > +            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
> > @@ -451,10 +465,12 @@ AC_DEFUN([OVS_CHECK_DPDK], [
> >      # 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.
> 
> At least part of the above comment is no more valid.
Sure , will update the comment.

> 
> > -    case "$DPDK_LIB" in
> > -      *whole-archive*) DPDK_vswitchd_LDFLAGS=$DPDK_LIB;;
> > -      *) DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-
> whole-archive
> > -    esac
> > +    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
> > +
> >      AC_SUBST([DPDK_vswitchd_LDFLAGS])
> >      AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.])
> >    fi
> > diff --git a/python/automake.mk b/python/automake.mk index
> > 2f08c7701..69d9800f9 100644
> > --- a/python/automake.mk
> > +++ b/python/automake.mk
> > @@ -47,7 +47,8 @@ ovs_pyfiles = \
> >  EXTRA_DIST += \
> >  	python/build/__init__.py \
> >  	python/build/nroff.py \
> > -	python/build/soutil.py
> > +	python/build/soutil.py \
> > +	python/build/pkgcfg.py
> 
> Should be added to FLAKE8_PYFILES.  This will likely expose some PEP8 issues
> that should be fixed.
Sure , will add it to FLAKE8_PYFILES.

> 
> >
> >  # PyPI support.
> >  EXTRA_DIST += \
> > diff --git a/python/build/pkgcfg.py b/python/build/pkgcfg.py new file
> > mode 100644 index 000000000..7cee3cb03
> > --- /dev/null
> > +++ b/python/build/pkgcfg.py
> > @@ -0,0 +1,30 @@
> 
> shebang is missing.
Sure , will add it .

> 
> > +# Copyright (c) 2020 Intel, Inc.
> > +#
> > +# Licensed under the Apache License, Version 2.0 (the "License") #
> > +You may not use this file except in compliance with the License.
> > +# You may obtain a copy of the License at:
> > +#
> > +#     http://www.apache.org/licenses/LICENSE-2.0
> > +#
> > +# Unless required by applicable law or agreed to in writing, software
> > +# Distributed under the License is distributed on an "AS IS" BASIS, #
> > +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> > +# 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.
> 
> What exactly this script does and why it's needed?
> More extensive comment required.  Probably, with a short example.
> Maybe it makes sense to move comment from acinclude.m4 here and keep
> only the short description there.

This goes back to the libtool re-arranging the --whole-archive and --no-whole-archive flags.
Libtool seems to pack the linker flags together causing :
-L/usr/local/lib/x86_64-linux-gnu -Wl,--whole-archive,<libs>, -Wl,--no-whole-archive, <libs>
to appear as :
... -Wl,--whole-archive  -Wl,--no-whole-archive ...
which removes the effect of these linker flags altogether

So , I added this script , which keeps only one entry for -Wl, so that it doesn’t get jumbled.

This was also probably why even incase of make , we had something like this in the acinclude.m4 , where -Wl was only provided only once.
"DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive"

Will add these comments in the file as well.

> 
> > +
> > +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
> > +    # Ld only understands -lpthread.
> > +    final_string = final_string.replace('-pthread','-lpthread')
> > +    return final_string
> > +
> > +if __name__ == "__main__":
> > +    print(parse_pkg_cfg_libs(sys.argv[1:]))
> >
> 
> In general, please, add some spaces after commas and around binary
> operators.

Sure , will address this.

Thanks and regards,
Sunil Pai


More information about the dev mailing list