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

Ian Stokes ian.stokes at intel.com
Wed Jul 17 11:28:04 UTC 2019


On 7/17/2019 11:29 AM, Van Haaren, Harry wrote:
>> -----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.

Ah, good catch, sounds good to me.

Ian
> 



More information about the dev mailing list