[ovs-dev] [PATCH v9 1/5] dpif-netdev: implement function pointers/subtable

Ian Stokes ian.stokes at intel.com
Tue Jul 2 15:44:35 UTC 2019


On 7/2/2019 4:02 PM, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Stokes, Ian
>> Sent: Tuesday, July 2, 2019 3:40 PM
>> To: Van Haaren, Harry <harry.van.haaren at intel.com>; ovs-dev at openvswitch.org
>> Cc: i.maximets at samsung.com
>> Subject: Re: [ovs-dev] [PATCH v9 1/5] dpif-netdev: implement function
>> pointers/subtable
>>
>> On 5/15/2019 6:02 PM, Ian Stokes wrote:
>>> On 5/8/2019 4:13 PM, Harry van Haaren wrote:
>>>> This allows plugging-in of different subtable hash-lookup-verify
>>>> routines, and allows special casing of those functions based on
>>>> known context (eg: # of bits set) of the specific subtable.
>>>>
>>>> Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
>>>>
>>>> ---
>>>>
>>>> v9:
>>>> - Use count_1bits in favour of __builtin_popcount (Ilya)
>>>>
>>>> v6:
>>>> - Implement subtable effort per packet "lookups_match" counter  (Ilya)
>>>> - Remove double newline (Eelco)
>>>> - Remove doubel * before comments (Eelco)
>>>> - Reword comments in dpcls_lookup() for clarity (Harry)
>>>> ---
>>>>    lib/dpif-netdev.c | 135 +++++++++++++++++++++++++++++++---------------
>>>>    1 file changed, 93 insertions(+), 42 deletions(-)
>>>>
>>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>>> index 5a6f2abac..e2e2c14e7 100644
>>>> --- a/lib/dpif-netdev.c
>>>> +++ b/lib/dpif-netdev.c
>>>> @@ -7572,6 +7572,27 @@ dpif_dummy_register(enum dummy_level level)
>>>>    
> 
>>>>    /* Datapath Classifier. */
>>>> +/* forward declaration for lookup_func typedef */
>>>
>>> Minor nit above, general rule of thumb for comment style, start with
>>> capital letter and finish with period.
>>>
>>>> +struct dpcls_subtable;
>>>> +
>>>> +/* Lookup function for a subtable in the dpcls. This function is called
>>>> + * by each subtable with an array of packets, and a bitmask of
>>>> packets to
>>>> + * perform the lookup on. Using a function pointer gives flexibility to
>>>> + * optimize the lookup function based on subtable properties and the
>>>> + * CPU instruction set available at runtime.
>>>> + */
>>>> +typedef uint32_t (*dpcls_subtable_lookup_func)(struct dpcls_subtable
>>>> *subtable,
>>>> +                uint32_t keys_map, const struct netdev_flow_key *keys[],
>>>> +                struct dpcls_rule **rules);
>>>
>>> Alignment for the arguments above looks odd, typically arguments are
>>> kept vertically in line with one another *similar to
>>> dpcls_subtable_lookup_generic below).
>>>
>>>> +
>>>> +/* Prototype for generic lookup func, using same code path as before */
>>>
>>> Period at end of comment above.
>>>
>>>> +uint32_t
>>>> +dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
>>>> +                              uint32_t keys_map,
>>>> +                              const struct netdev_flow_key *keys[],
>>>> +                              struct dpcls_rule **rules);
>>>> +
>>>> +
>>
>> One minor follow up, extra whitespace seems to have been added here, cab
>> be reduced to 1 new line.
> 
> Will fix.
> 
> <snip>
> 
>>>
>>>> +        uint32_t pkts_matched = count_1bits(found_map);
>>>
>>> Just to clarify, at this point found_map has been returned and it only
>>> contains matches where packets were found? i.e. it doesn't contain
>>> matches from possible hash collisions?
>>>
>>>> +        lookups_match += pkts_matched * subtable_pos;
>>
>> Hi Harry, can you clarify above why '* subtable_pos' is used? Is that
>> right? I'm just thinking that you already have the number of packets
>> matched from the count_1bits(found_map).
> 
> The "lookups match" counter variable name is a bit misleading, but I'd
> change it in this patchset as its orthogonal to the DPCLS function pointer
> concept.
> 
> The counter counts "number of subtables searched per hit packet", so to
> speak. See Ilya's input and my response here:
> https://www.mail-archive.com/ovs-dev@openvswitch.org/msg31442.html
> 
> This is a pseudo code of expected behavior:
> 1st subtable packet hits are += 1,
> 2nd subtable packet hits are += 2,
> 3rd subtable packet hits are += 3  etc.
> 
> At the end of a burst, we have a bitmask of packets hit, and a counter for
> "subtable effort per packet matched".
> 
> Given two experienced OVS folks asked ~ the same question, hints that its
> time for a cleanup & refactor, perhaps even a /* descriptive comment */ :)
> 

Ya that makes more sense, confirms what i suspected :).

Sure a description of the purpose would help here in the next revision.

Thanks
Ian




More information about the dev mailing list