> -----Original Message----- > From: Stokes, Ian > Sent: Tuesday, July 16, 2019 10:07 PM > To: Van Haaren, Harry <[email protected]>; [email protected] > Cc: [email protected]; [email protected]; [email protected] > 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
