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

Van Haaren, Harry harry.van.haaren at intel.com
Tue Jul 2 15:02:36 UTC 2019


> -----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 */ :)

<snip>

Thanks for review / comments! -Harry


More information about the dev mailing list