[ovs-dev] [PATCH v11 5/5] dpif-netdev: Add specialized generic scalar functions

Van Haaren, Harry harry.van.haaren at intel.com
Wed Jul 17 10:29:34 UTC 2019


> -----Original Message-----
> From: Stokes, Ian
> Sent: Tuesday, July 16, 2019 10:07 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>; dev at openvswitch.org
> Cc: echaudro at redhat.com; i.maximets at samsung.com; malvika.gupta at arm.com
> Subject: Re: [PATCH v11 5/5] dpif-netdev: Add specialized generic scalar
> functions
> 
> On 7/15/2019 5:36 PM, Harry van Haaren wrote:
> > This commit adds a number of specialized functions, that handle

<snip>

> Thanks for the v11 Harry, some minor comments inline below.

Thanks for review!

> > v11:
> > - Use MACROs to declare and check optimized functions (Ilya)
> > - Use captial letter for commit message (Ilya)
> > - Rebase onto latest patchset changes
> > - Added NEWS entry for data-path subtable specialization (Ian/Harry)
> > - Checkpatch notes an "incorrect bracketing" in the MACROs, however I
> >    didn't find a solution that it does like.
> >
> > v10:
> > - Rebase changes from previous patches.
> > - Remove "restrict" keyword as windows CI failed, see here for details:
> >    https://ci.appveyor.com/project/istokes/ovs-q8fvv/builds/24398228
> >
> > v8:
> > - Rework to use blocks_cache from the dpcls instance, to avoid variable
> >    lenght arrays in the data-path.
> > ---
> >   NEWS                             |  4 +++
> >   lib/dpif-netdev-lookup-generic.c | 51 ++++++++++++++++++++++++++++++++
> >   lib/dpif-netdev-private.h        |  8 +++++
> >   lib/dpif-netdev.c                |  9 ++++--
> >   4 files changed, 70 insertions(+), 2 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 81130e667..4cfffb1bc 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -34,6 +34,10 @@ Post-v2.11.0
> >        * 'ovs-appctl exit' now implies cleanup of non-internal ports in
> userspace
> >          datapath regardless of '--cleanup' option. Use '--cleanup' to
> remove
> >          internal ports too.
> > +     * Datapath classifer code refactored to enable function pointers to
> select
> > +       the lookup implementation at runtime. This enables specialization of
> > +       specific subtables based on the miniflow attributes, enhancing the
> > +       performance of the subtable search.
> >      - OVSDB:
> >        * OVSDB clients can now resynchronize with clustered servers much
> more
> >          quickly after a brief disconnection, saving bandwidth and CPU time.
> > diff --git a/lib/dpif-netdev-lookup-generic.c b/lib/dpif-netdev-lookup-
> generic.c
> > index abd166fc3..259c36645 100644
> > --- a/lib/dpif-netdev-lookup-generic.c
> > +++ b/lib/dpif-netdev-lookup-generic.c
> > @@ -233,7 +233,58 @@ dpcls_subtable_lookup_generic(struct dpcls_subtable
> *subtable,
> >                                 const struct netdev_flow_key *keys[],
> >                                 struct dpcls_rule **rules)
> >   {
> > +    /* Here the runtime subtable->mf_bits counts are used, which forces the
> > +     * compiler to iterate normal for() loops. Due to this limitation in
> the
> > +     * compilers available optimizations, this function has lower
> performance
> > +     * than the below specialized functions.
> > +     */
> >       return lookup_generic_impl(subtable, blocks_scratch, keys_map, keys,
> rules,
> >                                  subtable->mf_bits_set_unit0,
> >                                  subtable->mf_bits_set_unit1);
> >   }
> > +
> > +/* Expand out specialized functions with U0 and U1 bit attributes */
> 
> Minor, missing period at end of comment (can fix this on commit if there
> are no other revisions).

Fixed.

<snip>
> > +DECLARE_OPTIMIZED_LOOKUP_FUNCTION(5,1)
> > +DECLARE_OPTIMIZED_LOOKUP_FUNCTION(4,1)
> > +DECLARE_OPTIMIZED_LOOKUP_FUNCTION(4,0)
> > +
> > +/* Check if a speicalized function is valid for the required subtable. */
> Minor, speicalized -> specialized, again can be fixed on commit otherwise.

Fixed.

> 
> > +#define CHECK_LOOKUP_FUNCTION(U0,U1)
> \
> > +    if (!f && u0_bits == U0 && u1_bits == U1) {
> \
> > +        f = dpcls_subtable_lookup_mf_u0w##U0##_u1w##U1;
> \
> > +    }
> > +
> > +/* Probe function to lookup an available specialized function.
> > + * If capable to run the requested miniflow fingerprint, this function
> returns
> > + * the most optimal implementation for that miniflow fingerprint.
> > + * @retval FunctionAddress A valid function to handle the miniflow bit
> pattern
> > + * @retval 0 The requested miniflow is not supported here, NULL is returned
> > + */
> > +dpcls_subtable_lookup_func
> > +dpcls_subtable_generic_probe(uint32_t u0_bits, uint32_t u1_bits)
> > +{
> > +    dpcls_subtable_lookup_func f = NULL;
> 
> In the comments you return FunctionAddress but you return f below,
> should this not be FunctionAddress or maybe another variable name if
> 'FunctionAddress' is a bit unwieldy?

Updated return value descriptions as Non-NULL and NULL, and updated comments to
read well. This better describes the code than "FunctionAddress".


<snip>
> > -    /* Assign the generic lookup - this works with any miniflow
> fingerprint. */
> > -    subtable->lookup_func = dpcls_subtable_lookup_generic;
> > +    /* Probe for a specialized generic lookup function. */
> > +    subtable->lookup_func = dpcls_subtable_generic_probe(unit0, unit1);
> > +
> > +    /* If not set, assign generic lookup. Generic works for any miniflow.
> */
> > +    if (!subtable->lookup_func) {
> > +        subtable->lookup_func = dpcls_subtable_lookup_generic;
> > +    }
> >
> >       cmap_insert(&cls->subtables_map, &subtable->cmap_node, mask->hash);
> >       /* Add the new subtable at the end of the pvector (with no hits yet)
> */
> Missing period above.


I'd prefer not fix this one - that code is patch context and isn't currently being modified.


More information about the dev mailing list