[ovs-dev] [PATCH v2 1/1] dpdk: Update to use DPDK v20.11.

[Luca Boccassi]

On Thu, 2020-12-10 at 13:54 +0100, Ilya Maximets wrote:
> On 12/10/20 11:06 AM, Luca Boccassi wrote:
> > On Wed, 2020-12-09 at 22:42 +0100, Ilya Maximets wrote:
> > > On 12/9/20 7:01 PM, Ilya Maximets wrote:
> > > > On 12/9/20 6:50 PM, Richardson, Bruce wrote:
> > > > > > -----Original Message-----
> > > > > > From: Ilya Maximets <ilya.maximets at gmail.com>
> > > > > > Sent: Wednesday, December 9, 2020 4:29 PM
> > > > > > To: Luca Boccassi <bluca at debian.org>; Ilya Maximets <i.maximets at ovn.org>;
> > > > > > Stokes, Ian <ian.stokes at intel.com>; dev at openvswitch.org; Richardson, Bruce
> > > > > > <bruce.richardson at intel.com>
> > > > > > Cc: ktraynor at redhat.com; david.marchand at redhat.com; Pai G, Sunil
> > > > > > <sunil.pai.g at intel.com>; elibr at nvidia.com;
> > > > > > christian.ehrhardt at canonical.com
> > > > > > Subject: Re: [PATCH v2 1/1] dpdk: Update to use DPDK v20.11.
> > > > > > 
> > > > > > On 12/9/20 5:21 PM, Luca Boccassi wrote:
> > > > > > > On Wed, 2020-12-09 at 16:53 +0100, Ilya Maximets wrote:
> > > > > > > > On 12/2/20 2:01 PM, Ian Stokes wrote:
> > > > > > > > > This commit adds support for DPDK v20.11, it includes the following
> > > > > > > > > changes.
> > > > > > > 
> > > > > > > ..
> > > > > > > 
> > > > > > > > >      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"
> > > > > > > > 
> > > > > > > > I noticed that DPDK puts -march=something into clfags.  That is not a
> > > > > > good thing
> > > > > > > > to have as this could mess up user-provided flags to build OVS.  This
> > > > > > 'march' was
> > > > > > > > used to build DPDK and should not be passed to OVS.
> > > > > > > > We should, probably, strip this thing out.
> > > > > > > 
> > > > > > > We do that intentionally because it's a minimum requirement for all
> > > > > > > applications linking to DPDK libraries. I do not recall exactly where
> > > > > > > the requirement comes from, I think it was from some static inline
> > > > > > > functions defined in some public headers and used by applications.
> > > > > > > 
> > > > > > 
> > > > > > After a brief discussion with David it seems that this flag could be
> > > > > > removed since RTE_MACHINE_CPUFLAG_XX removed in 20.11.
> > > > > > David, Bruce, could you, please, comment on this.
> > > > > > 
> > > > > > Anyway, this flag raises the minimal CPU requirement for those who
> > > > > > doesn't want to use DPDK, but uses OVS that happened to be compiled
> > > > > > with DPDK support.  Maybe that is OK, I'm not sure.  But this also
> > > > > > suppresses compiler optimizations for those who building OVS with
> > > > > > DPDK provided by some distribution. e.g. distributions will, probably,
> > > > > > build DPDK with -Dmachine=default and that will force -march=corei7
> > > > > > for the OVS regardless of the current CPU the user wants to build on.
> > > > > > 
> > > > > > Best regards, Ilya Maximets.
> > > > > 
> > > > > I've run some tests using the DPDK examples and unfortunately there are still
> > > > > some header files which will throw compiler errors if the -march flag is not
> > > > > present, because they assume at minimum of SSE4.2.
> > > > 
> > > > OVS forces -mssse3 for build with dpdk.  Is it enough or we really need to
> > > > have -msse4.2 ?
> > > 
> > > I added following code to strip out -march argument and build works fine:
> > > 
> > > diff --git a/acinclude.m4 b/acinclude.m4
> > > index 2fd9aa255..3df496666 100644
> > > --- a/acinclude.m4
> > > +++ b/acinclude.m4
> > > @@ -421,6 +421,10 @@ AC_DEFUN([OVS_CHECK_DPDK], [
> > >  
> > >      CFLAGS="$ovs_save_CFLAGS"
> > >      LDFLAGS="$ovs_save_LDFLAGS"
> > > +    # Stripping out possible instruction set specific configuration that DPDK
> > > +    # forces in pkg-config since this could override user-specified options.
> > > +    # It's enough to have -mssse3 to build with DPDK headers.
> > > +    DPDK_INCLUDE=$(echo "$DPDK_INCLUDE" | sed 's/-march=[[a-z0-9]]*//g')
> > >      OVS_CFLAGS="$OVS_CFLAGS $DPDK_INCLUDE"
> > >      OVS_ENABLE_OPTION([-mssse3])
> > >  
> > > ---
> > > 
> > > So, I'm assuming that -mssse3 is enough build time dependency.
> > > Are there possible runtime issues or it's only a build-time problem?
> > > 
> > > > > Question: how serious an issue is having an SSE4.2 minimum requirement? If it's
> > > > > not a big deal, I'd suggest we just update the .pc file from having an "march="
> > > > > flag to having "-msse4.2" flag to avoid being too proscriptive. (i.e. avoid the
> > > > > second case mentioned where the -march=corei7 overrides the user's choice)
> > > > > If it is a more serious issue, we'll have to check what can be done for the
> > > > > individual headers that require SSE4.2 and see if we can remove that requirement.
> > > 
> > > I'd vote for having -mssse3 in .pc file if this is enough.  IMHO, there should be
> > > only minimal requirement for the CPU instruction set.
> > > 
> > > Ideally, of course, library headers should not have such build requirements that
> > > affects build process of the application.
> > > 
> > > Best regards, Ilya Maximets.
> > 
> > That might be enough for _ovs_ but it doesn't mean it is for everything
> > else. Also, hardcoding x86 flags means it will break arm and ppc
> > builds. Let's not introduce regressions, please. We added this for a
> > reason.
> > 
> 
> I understand.  I'm OK with having -msse4.2, but -march= is too heavy and it
> overrides user-provided options.
> 
> Of course, -msse4.2 should be added only for x86 platforms.
> 
> Luka, Christian, are there 2 different packages for OVS in distributions
> (one with DPDK and one without)?  Or it's a single package for both use-cases?
> If it's a single package it will not be possible to run OVS (w/o DPDK) on
> systems lower than corei7 if we'll keep -march.
> 
> For OVS, for now, my suggestion is to strip out -march and, optionally,
> replace OVS_ENABLE_OPTION([-mssse3]) with OVS_ENABLE_OPTION([-msse4.2]),
> since sse4.2 is a requirement starting from DPDK 17.08.
> 
> Best regards, Ilya Maximets.

Sorry, but I must insist: that is not enough. The reason we added this
to pkg-config is that it _must_ match what DPDK used at build time. If
DPDK is built with AVX512, OVS needs to be built with it too. There are
too many things leaking in the public headers as static inline to
maintain it manually. I know because we tried, and things kept breaking
up all the time.

In Debian and Ubuntu, there are 2 different builds of OVS - one with
DPDK, one without.

-- 
Kind regards,
Luca Boccassi