[ovs-dev] [v13 04/12] dpif-avx512: Add ISA implementation of dpif.
Flavio Leitner
fbl at sysclose.org
Mon Jun 21 16:39:21 UTC 2021
On Mon, Jun 21, 2021 at 04:13:12PM +0000, Ferriter, Cian wrote:
> Hi Flavio,
>
> Thanks for the review. My responses are inline.
>
> Cian
>
> > -----Original Message-----
> > From: Flavio Leitner <fbl at sysclose.org>
> > Sent: Sunday 20 June 2021 21:09
> > To: Ferriter, Cian <cian.ferriter at intel.com>
> > Cc: ovs-dev at openvswitch.org; Amber, Kumar <kumar.amber at intel.com>; i.maximets at ovn.org
> > Subject: Re: [ovs-dev] [v13 04/12] dpif-avx512: Add ISA implementation of dpif.
> >
> >
> > Hi,
> >
> > I am still reviewing the patch, but I thought worth to discuss
> > few items below.
> >
> > On Thu, Jun 17, 2021 at 05:18:17PM +0100, Cian Ferriter wrote:
> > > From: Harry van Haaren <harry.van.haaren at intel.com>
> > >
> > > This commit adds the AVX512 implementation of DPIF functionality,
> > > specifically the dp_netdev_input_outer_avx512 function. This function
> > > only handles outer (no re-circulations), and is optimized to use the
> > > AVX512 ISA for packet batching and other DPIF work.
> > >
> > > Sparse is not able to handle the AVX512 intrinsics, causing compile
> > > time failures, so it is disabled for this file.
> > >
> > > Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> > > Co-authored-by: Cian Ferriter <cian.ferriter at intel.com>
> > > Signed-off-by: Cian Ferriter <cian.ferriter at intel.com>
> > > Co-authored-by: Kumar Amber <kumar.amber at intel.com>
> > > Signed-off-by: Kumar Amber <kumar.amber at intel.com>
> > >
> > > ---
> > >
> > > v13:
> > > - Squash "Add HWOL support" commit into this commit.
> > > - Add NEWS item about this feature here rather than in a later commit.
> > > - Add #define NUM_U64_IN_ZMM_REG 8.
> > > - Add comment describing operation of while loop handling HWOL->EMC->SMC
> > > lookups in dp_netdev_input_outer_avx512().
> > > - Add EMC and SMC batch insert functions for better handling of EMC and
> > > SMC in AVX512 DPIF.
> > > - Minor code refactor to address review comments.
> > > ---
> > > NEWS | 2 +
> > > lib/automake.mk | 5 +-
> > > lib/dpif-netdev-avx512.c | 327 +++++++++++++++++++++++++++++++
> > > lib/dpif-netdev-private-dfc.h | 25 +++
> > > lib/dpif-netdev-private-dpif.h | 32 +++
> > > lib/dpif-netdev-private-thread.h | 11 +-
> > > lib/dpif-netdev-private.h | 25 +++
> > > lib/dpif-netdev.c | 103 ++++++++--
> > > 8 files changed, 514 insertions(+), 16 deletions(-)
> > > create mode 100644 lib/dpif-netdev-avx512.c
> > > create mode 100644 lib/dpif-netdev-private-dpif.h
> > >
> > > diff --git a/NEWS b/NEWS
> > > index 96b3a61c8..6a4a7b76d 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -10,6 +10,8 @@ Post-v2.15.0
> > > * Auto load balancing of PMDs now partially supports cross-NUMA polling
> > > cases, e.g if all PMD threads are running on the same NUMA node.
> > > * Refactor lib/dpif-netdev.c to multiple header files.
> > > + * Add avx512 implementation of dpif which can process non recirculated
> > > + packets. It supports partial HWOL, EMC, SMC and DPCLS lookups.
> > > - ovs-ctl:
> > > * New option '--no-record-hostname' to disable hostname configuration
> > > in ovsdb on startup.
> > > diff --git a/lib/automake.mk b/lib/automake.mk
> > > index 3a33cdd5c..660cd07f0 100644
> > > --- a/lib/automake.mk
> > > +++ b/lib/automake.mk
> > > @@ -33,11 +33,13 @@ lib_libopenvswitchavx512_la_CFLAGS = \
> > > -mavx512f \
> > > -mavx512bw \
> > > -mavx512dq \
> > > + -mbmi \
> > > -mbmi2 \
> > > -fPIC \
> > > $(AM_CFLAGS)
> > > lib_libopenvswitchavx512_la_SOURCES = \
> > > - lib/dpif-netdev-lookup-avx512-gather.c
> > > + lib/dpif-netdev-lookup-avx512-gather.c \
> > > + lib/dpif-netdev-avx512.c
> > > lib_libopenvswitchavx512_la_LDFLAGS = \
> > > -static
> > > endif
> > > @@ -114,6 +116,7 @@ lib_libopenvswitch_la_SOURCES = \
> > > lib/dpif-netdev-private-dfc.c \
> > > lib/dpif-netdev-private-dfc.h \
> > > lib/dpif-netdev-private-dpcls.h \
> > > + lib/dpif-netdev-private-dpif.h \
> > > lib/dpif-netdev-private-flow.h \
> > > lib/dpif-netdev-private-hwol.h \
> > > lib/dpif-netdev-private-thread.h \
> > > diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> > > new file mode 100644
> > > index 000000000..0e55b0be2
> > > --- /dev/null
> > > +++ b/lib/dpif-netdev-avx512.c
> > > @@ -0,0 +1,327 @@
> > > +/*
> > > + * Copyright (c) 2021 Intel Corporation.
> > > + *
> > > + * 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.
> > > + */
> > > +
> > > +#ifdef __x86_64__
> > > +/* Sparse cannot handle the AVX512 instructions. */
> > > +#if !defined(__CHECKER__)
> > > +
> > > +#include <config.h>
> > > +
> > > +#include "dpif-netdev.h"
> > > +#include "dpif-netdev-perf.h"
> > > +
> > > +#include "dpif-netdev-private.h"
> > > +#include "dpif-netdev-private-dpcls.h"
> > > +#include "dpif-netdev-private-flow.h"
> > > +#include "dpif-netdev-private-thread.h"
> > > +#include "dpif-netdev-private-hwol.h"
> >
> > The -private.h already includes a few of the above, but
> > not all, so the interface doesn't seem to be well defined.
> > For example, in -private.h we have dpcls_lookup() while
> > other dpcls functions are in -private-dpcls.h. In this
> > case, the following would be enough:
> >
> > #include "dpif-netdev-private.h"
> > #include "dpif-netdev-private-hwol.h"
> >
> > But then I don't know why other headers are included in the
> > interface but not the -private-hwol.h.
> >
> >
>
> Good point. This can be cleaned up. I've included lib/dpif-netdev-private-hwol.h in lib/dpif-netdev-private.h and removed the headers included by lib/dpif-netdev-private.h from lib/dpif-netdev-avx512.c.
>
> I'll move the prototype for dpcls_lookup() too, it makes more sense if it's in lib/dpif-netdev-private-dpcls.h.
Before you spend time on it, please consider if the refactoring is
really required. I think refactoring the code usually is a nice
thing to do when the result is a clean interface, but it seems that
will conflict with some other patches being reviewed. Then, instead
of you and/or others have to fix patches approaching the deadline
maybe it would be better to leave optional refactoring to a follow
up patch.
Another point to consider is that this refactoring is affecting an
important part of OVS, so it will require careful review and perhaps
additional follow ups until everyone is happy. If you can reduce that
impact, it would reduce the risk which helps to get the work accepted.
What do you think?
Thanks,
--
fbl
More information about the dev
mailing list