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

Eelco Chaudron echaudro at redhat.com
Wed Nov 3 09:34:25 UTC 2021



On 3 Nov 2021, at 9:34, Van Haaren, Harry wrote:

>> -----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>

Thanks for taking care of this! Will try to review it next week if available.

//Eelco



More information about the dev mailing list