[ovs-dev] [PATCH v6 0/6] DPCLS Subtable ISA Optimization

[William Tu]

On Mon, Jul 6, 2020 at 4:57 AM Van Haaren, Harry
<harry.van.haaren at intel.com> wrote:
>
> > -----Original Message-----
> > From: William Tu <u9012063 at gmail.com>
> > Sent: Sunday, July 5, 2020 3:06 PM
> > To: Van Haaren, Harry <harry.van.haaren at intel.com>
> > Cc: ovs-dev <ovs-dev at openvswitch.org>; Stokes, Ian <ian.stokes at intel.com>; Ilya
> > Maximets <i.maximets at ovn.org>; Federico Iezzi <fiezzi at redhat.com>
> > Subject: Re: [PATCH v6 0/6] DPCLS Subtable ISA Optimization
> >
> > On Thu, Jul 2, 2020 at 10:42 AM Harry van Haaren
> > <harry.van.haaren at intel.com> wrote:
> > >
> > > v6 work done:
> > > - Fix as --64 unrecognized option
> > > - Fix build issues with avx512 library changes
> > > - Fix files left in build dir after distclean
> > > - Fix CPU arch dependant RTE_CPUFLAG_ usage
> > >
> > > Thanks William & Ian for review & help on v5.
> > > All known issues are fixed and working here.
>
> <snip cover letter details>
>
> > > Harry van Haaren (6):
> > >   dpif-netdev: implement subtable lookup validation.
> > >   dpif-netdev: add subtable lookup prio set command.
> > >   dpif-netdev: add subtable-lookup-prio-get command.
> > >   dpdk: enable cpu feature detection.
> > >   dpif-lookup: add avx512 gather implementation.
Hi Harry,

Another thing I noticed is that for the avx512 implementation, the
current series
only enables the three cases
DECLARE_OPTIMIZED_LOOKUP_FUNCTION(5, 1)
DECLARE_OPTIMIZED_LOOKUP_FUNCTION(4, 1)
DECLARE_OPTIMIZED_LOOKUP_FUNCTION(4, 0)

I did the following approach to collect how the current OVS uses the
miniflow bitmap.
apply this patch
diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index b2b17eed335a..c9513213270d 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -312,7 +312,8 @@ m4_divert_pop([PREPARE_TESTS])
 #
 #   OVS_VSWITCHD_STOP(["/expected error/d"])
 m4_define([OVS_VSWITCHD_STOP],
-  [AT_CHECK([check_logs $1])
+  [ovs-appctl dpctl/dump-flows -m;
+   AT_CHECK([check_logs $1])
    OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
    OVS_APP_EXIT_AND_WAIT([ovsdb-server])])

Run the test
$ make check TESTSUITEFLAGS='-j4 -d'

Collect the miniflow_bits
$ grep -r miniflow tests/testsuite.dir/* | sed -n
's/.*\(miniflow_bits.*\)/\1/p'  > miniflow.txt
$ sort miniflow.txt | uniq
miniflow_bits(11,1)
miniflow_bits(4,0)
miniflow_bits(4,1)
miniflow_bits(4,2)
miniflow_bits(4,3)
miniflow_bits(5,0)
miniflow_bits(5,1)
miniflow_bits(5,10)
miniflow_bits(5,2)
miniflow_bits(5,3)
miniflow_bits(6,0)
miniflow_bits(8,1)
miniflow_bits(9,0)
miniflow_bits(9,4)
miniflow_bits(9,5)

Does miniflow_bits(x, y) and (x + y) must be less than or equal to 8 here?
Thanks

William

> > >   docs/dpdk/bridge: add datapath performance section.
> > >
> >
> > The v6 looks good to me. It would be great if there is another pair of
> > eyes on this series.
> > It's a little hard for me to understand the avx512 gather
> > implementation (patch5),
> > but the idea of using autovalidator and it passes our test cases give
> > me more confidence
> > that it is correct.
> >
> > Acked-by: William Tu <u9012063 at gmail.com>
>
> I'm happy to see the Autovalidator concept in practice working well,
> it is likely a development/testing model to re-use in future for similar
> types of work to ensure consistency and validity of SIMD code.
>
> Thanks for your multiple reviews of the series over the last months.
> -Harry