On 3 Nov 2021, at 9:34, Van Haaren, Harry wrote:
>> -----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> Thanks for taking care of this! Will try to review it next week if available. //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
