> -----Original Message-----
> From: Stokes, Ian
> Sent: Wednesday, July 10, 2019 4:30 PM
> To: Van Haaren, Harry <harry.van.haa...@intel.com>; d...@openvswitch.org
> Cc: i.maxim...@samsung.com; malvika.gu...@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.haa...@intel.com>
> > Tested-by: Malvika Gupta <malvika.gu...@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!
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
