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

Van Haaren, Harry harry.van.haaren at intel.com
Wed Jul 10 15:40:48 UTC 2019


> -----Original Message-----
> From: Stokes, Ian
> Sent: Wednesday, July 10, 2019 4:30 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>; dev at openvswitch.org
> Cc: i.maximets at samsung.com; malvika.gupta at arm.com
> Subject: Re: [PATCH v10 1/5] dpif-netdev: implement function pointers/subtable
> 
> On 7/9/2019 1:34 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>
> > Tested-by: Malvika Gupta <malvika.gupta at arm.com>
> >
> 
> Thanks for this harry, few minor comments below.

Hey, cool I'll snip away irrelevant code sections for readability.


<snip>

> > +uint32_t
> > +dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
> > +                              uint32_t keys_map,
> > +                              const struct netdev_flow_key *keys[],
> > +                              struct dpcls_rule **rules)
> > +{
> > +        int i;
> > +        uint32_t found_map;
> 
> I was thinking should this be initialized to 0, but  I don't think there
> is a need. the call to cmap_find_batch() below will set it to 0 in the
> case of no math, otherwise set the i-th bit for a match so I thinks it's ok.

Correct - the value is written before being used - always.
Initializing it to zero is pointless, and only waste and instruction.


<snip>
> > @@ -7849,16 +7931,12 @@ dpcls_lookup(struct dpcls *cls, const struct
> >       if (cnt != MAP_BITS) {
> >           keys_map >>= MAP_BITS - cnt; /* Clear extra bits. */
> > @@ -7866,6 +7944,7 @@ dpcls_lookup(struct dpcls *cls, const struct
> netdev_flow_key *keys[],
> >       memset(rules, 0, cnt * sizeof *rules);
> >
> >       int lookups_match = 0, subtable_pos = 1;
> > +    uint32_t found_map;
> 
> Same as above, should be OK as it wont be used until after the call to
> the generic look up function, which will set it to 0 in the event of no
> matches.

Correct - written before use - no use in initializing to any value.


> > -            /* None of the found rules was a match.  Reset the i-th bit to
> > -             * keep searching this key in the next subtable. */
> > -            ULLONG_SET0(found_map, i);  /* Did not match. */
> > -        next:
> > -            ;                     /* Keep Sparse happy. */
> > -        }
> > -        keys_map &= ~found_map;             /* Clear the found rules. */
> > +        /* Clear the found rules, and return early if all packets are
> found. */
> > +        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 thought about whether an item should be added to the NEWS doc
> (possibly not in this patch but perhaps later in the series), it's hard
> to say as the changes are under the hood and not configurable by a user.
> Thoughts?

Correct the user shouldn't identify the actual changes, except for small
gains in performance if the specialized subtables are in use.

I think it would be good to add a NEWS item, as people profiling OVS's functions
will see different function names, and having called out the DPCLS Function Pointer
changes, and why they are there would be beneficial.

I will draft a separate patch for NEWS item - so we can keep it parallel to this patch set.
Shout if that is not a good approach :)

> Regards
> Ian

Thanks for review!



More information about the dev mailing list