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

[Van Haaren, Harry]

Hi Ian,

Comments inline, and lots of <snips> to trim message size.

Thanks for review! -H


> -----Original Message-----
> From: Stokes, Ian
> Sent: Wednesday, May 15, 2019 6:03 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>; ovs-dev at openvswitch.org
> Cc: aconole at redhat.com; echaudro at redhat.com; i.maximets at samsung.com
> Subject: Re: [PATCH v9 1/5] dpif-netdev: implement function
> pointers/subtable
<snip>

> >   /* 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.

Sure, will keep in mind for the other patches too.

> > +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).

This one is a bit odd - the "const struct netdev_flow_key *keys[]" spills over
the 80 char limit if aligned using vertical alignment with spaces.

I've reworked to the vertical alignment but the line is now greater than 80 chars.

Guidance on what is "the best bad solution" here welcome :)


> > +/* Prototype for generic lookup func, using same code path as before */
> 
> Period at end of comment above.

Done.


<snip>
> > @@ -7640,6 +7668,10 @@ dpcls_create_subtable(struct dpcls *cls, const
> struct netdev_flow_key *mask)
> >       cmap_init(&subtable->rules);
> >       subtable->hit_cnt = 0;
> >       netdev_flow_key_clone(&subtable->mask, mask);
> > +
> > +    /* decide which hash/lookup/verify function to use */
> > +    subtable->lookup_func = dpcls_subtable_lookup_generic;
> 
> So by default dpcls_subtable_lookup_generic is set as the look up
> function. Makes sense as this is the only look up available at this
> stage. I assume it's later in the series a mechanism to select a
> different lookup is introduced. Or by default does the lookup always
> start off as the generic option when a subtable is created even when
> other options are possible?

Yes the generic is the default, which maintains current ovs' current dpcls code.

Correct that later in the series there is a dpcls_subtable_generic_probe() function
added, which can optionally provide a specialized lookup function for this subtable.

If the probe() function returns NULL, then the generic flavor is used. If the probe()
sets a function-pointer, then we use that.


<snip>
> > +        /* Lookup. */
> > +        const struct cmap_node *nodes[NETDEV_MAX_BURST];
> > +        uint32_t found_map =
> 
> Minor nit. Variable declaration. You may see a mix of approaches WRT
> where variables are declared. Personally I prefer at the beginning of
> the function to keep with the int variable you have already declared.
> looking at the original dpcls_lookup() where this code is moved from it
> looks like they also declared variables at the beginning. Would be good
> to follow that example.

Sure, moved variable declaration up to above its use.


<snip>
> > -        ULLONG_FOR_EACH_1(i, found_map) {
> > -            struct dpcls_rule *rule;
> > +        /* Count the number of subtables searched for this packet match.
> This
> > +         * estimates the "spread" of subtables looked at per matched
> packet */
> 
> Minor, missing period in comment above.

Done.


> > +        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?

Hash collisions are resolved inside the lookup implementation, hence
any bits set in pkts_matched are hash-matches AND passed the rule-verify too.

<snip>
> > -        keys_map &= ~found_map;             /* Clear the found rules. */
> > +        /* Clear the found rules, and return early if all packets are
> found */
> Missing period in comment above.

Fixed.


> > +        keys_map &= ~found_map;
> >           if (!keys_map) {
> >               if (num_lookups_p) {
> >                   *num_lookups_p = lookups_match;
> >               }
> > -            return true;              /* All found. */
> > +            return true;
> >           }
> >           subtable_pos++;
> >       }
> > +
> >       if (num_lookups_p) {
> >           *num_lookups_p = lookups_match;
> >       }
> > -    return false;                     /* Some misses. */
> > +    return false;
> >   }
> >
> 
> I've also run some performance tests on this patch but I didn't see any
> notable performance degradation from the function implementation which
> is good.
> 
> Ian

Thanks for review, I'll address other feedback on patchset and post a v10 then.