[ovs-dev] [PATCH v3] dpcls: Change info-get function to fetch dpcls usage stats.

Van Haaren, Harry harry.van.haaren at intel.com
Wed Nov 3 08:34:17 UTC 2021


> -----Original Message-----
> From: Eelco Chaudron <echaudro at redhat.com>
> Sent: Tuesday, November 2, 2021 9:10 AM
> To: Amber, Kumar <kumar.amber at intel.com>
> Cc: ovs-dev at openvswitch.org; fbl at sysclose.org; Van Haaren, Harry
> <harry.van.haaren at intel.com>; Stokes, Ian <ian.stokes at intel.com>;
> i.maximets at ovn.org
> Subject: Re: [PATCH v3] dpcls: Change info-get function to fetch dpcls usage
> stats.
> 
> Hi Kumar,
> 
> Thanks for fixing the previously mentioned issues, however, we still need to
> finish the API discussion. I copied over the discussion from v2 below.
> 
> //Eelco
> 
> On 28 Oct 2021, at 11:50, Kumar Amber wrote:

<snip large chunk of code review, thanks for review Eelco>

> >  dpcls_subtable_lookup_func
> > -dpcls_subtable_get_best_impl(uint32_t u0_bit_count, uint32_t
> u1_bit_count);
> > +dpcls_subtable_get_best_impl(uint32_t u0_bit_count, uint32_t u1_bit_count,
> > +                             dpcls_subtable_lookup_func old_func,
> > +                             bool will_use);
> >
> 
> First some cut/pastes from the V2 discussion which was not completed:
> 
> EC> Guess you overload this function to do a counter update, which I do not like
> (see comments below).
> 
> HH> Yes - correct. I'd prefer keep separate APIs too, but in reality the
> implementation to increment a counter
> HH> would require a search through all subtables (which we've already just
> done). So iterating all dpcls implementations
> HH> a second time, doing a brute-force search in order to increment a variable
> seemed too much effort, as well as
> HH> maintaining 2x implementations.
> 
> HH> The goal of adding the "will use result" parameter (overloading API) was to
> reduce code maintenance effort...
> 
> ---
> 
> EC> First, the 0, should probably be a false bool. But in general, I do not like this
> API
> EC> overload. Why would you call a dpcls_subtable_get_best_impl() to reset a
> EC> counter? Can we not add a new API in form or
> EC> dpcls_update_impl_use_count(struct dpcls_subtable *ubtable, bool
> increase), or
> EC> something else thats is more clear?
> 
> HH> As per above, the "dpcls_update_impl_use_count()" would require a brute-
> force search through the tables again.
> HH> By incorporating the "increment" in the API, we can just iterate the
> implementations once.
> HH> I think it is worth the complexity to avoid re-iterating implementations a 2nd
> time.
> 
> ---
> 
> EC> If dpcls_subtable_get_best_impl() would always increase the use count, no
> EC> changes should be needed here.
> 
> HH> I see where you're going with this approach - see above comments.
> HH> Would documenting the reasoning clearly as to why the API is overloaded,
> and keeping
> HH> implementation (1 function, overloeaded with will_use_result) the same as
> is today be acceptable?
> 
> ---
> 
> I think we should keep the API as clean as possible, and adding flags for all kinds
> of exceptions is not the way forward. This will result in a mess.
> 
> I’m sure you guys can come up with a nice solution. Without giving this much
> thought, you could maybe have dpcls_subtable_get_best_impl() return a pointer
> to dpcls_subtable_lookup_info_t and store/use that in dpcls_subtable, and have
> dpcls_update_impl_use_count() use a pointer to dpcls_subtable-
> >dpcls_subtable_lookup_info_t?

Good summarizing, OK, will take another look at the code and see if returning a pointer
to (and possibly caching it) subtable implementation is a good fit.

Only commenting on design here, Amber and I will spin-up a +1 version over the next
days.

<snip away unit-test patch content>


More information about the dev mailing list