> -----Original Message----- > From: Eelco Chaudron <[email protected]> > Sent: Tuesday, November 2, 2021 9:10 AM > To: Amber, Kumar <[email protected]> > Cc: [email protected]; [email protected]; Van Haaren, Harry > <[email protected]>; Stokes, Ian <[email protected]>; > [email protected] > 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> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
