[ovs-dev] [PATCH v11 5/5] dpif-netdev: Add specialized generic scalar functions

Ilya Maximets i.maximets at samsung.com
Wed Jul 17 10:24:02 UTC 2019


On 17.07.2019 13:18, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets at samsung.com]
>> Sent: Wednesday, July 17, 2019 10:52 AM
>> To: Van Haaren, Harry <harry.van.haaren at intel.com>; dev at openvswitch.org
>> Cc: echaudro at redhat.com; malvika.gupta at arm.com; Stokes, Ian
>> <ian.stokes at intel.com>
>> Subject: Re: [PATCH v11 5/5] dpif-netdev: Add specialized generic scalar
>> functions
> 
> <snip>
> 
>>> +DECLARE_OPTIMIZED_LOOKUP_FUNCTION(5,1)
>>> +DECLARE_OPTIMIZED_LOOKUP_FUNCTION(4,1)
>>> +DECLARE_OPTIMIZED_LOOKUP_FUNCTION(4,0)
>>
>> Some spaces needed after the comma.
> 
> Will fix in v12.
> 
>>> +/* Check if a speicalized function is valid for the required subtable. */
>>> +#define CHECK_LOOKUP_FUNCTION(U0,U1)
>> \
>>> +    if (!f && u0_bits == U0 && u1_bits == U1) {
>> \
>>> +        f = dpcls_subtable_lookup_mf_u0w##U0##_u1w##U1;
>> \
>>> +    }
>>
>> The reason why I initially placed this macro inside the function is that
>> 'u0_bits' and 'u1_bits' only makes sense inside the function.
>> IMHO, it's better to move this inside, but it's up to you.
> 
> I'd prefer leave outside the function - this seems to be the standard way of
> doing MACRO defines in OVS? Eg in dpif-netdev other macros are also outside the
> functions, so I'll follow that method.
> 
> 
> <snip>
>>> +dpcls_subtable_lookup_func
>>> +dpcls_subtable_generic_probe(uint32_t u0_bits, uint32_t u1_bits)
>>> +{
>>> +    dpcls_subtable_lookup_func f = NULL;
>>> +
>>> +    CHECK_LOOKUP_FUNCTION(5, 1);
>>> +    CHECK_LOOKUP_FUNCTION(4, 1);
>>> +    CHECK_LOOKUP_FUNCTION(4, 0);
>>> +
>>> +    if (f) {
>>> +        VLOG_INFO("Subtable using Generic Optimized for u0 %d, u1 %d\n",
>>> +                  u0_bits, u1_bits);
>>
>> Can we move this message out to 'dpcls_create_subtable' and log the
>> non-optimized case too?
>> Also, this needs to be DBG as all other subtable related logs are DBG logs.
>>
>> We could just extend the 'Creating subtable' log message to include this
>> information:
>>
>>     VLOG_DBG("Creating %"PRIuSIZE". subtable %p for in_port %d with a %s "
>>              "lookup function (units: %"PRIu32", %"PRIu32").",
>>              cmap_count(&cls->subtables_map), subtable, cls->in_port,
>>              (subtable->lookup_func == dpcls_subtable_lookup_generic)
>>              ? "generic" : "specialized", unit0, unit1);
> 
> 
> This might seem a nice idea now, however in future we can plug in other optimized
> flavors of lookups, and then the dpcls_create_subtable() function won't know
> the details of the implementation. Hence having the log in the implementation
> is the better solution.

If there will be other implementations we'll have to change the prototype of
'dpcls_subtable_generic_probe' and refactor the code around anyway. So, changing
the log message would be a minor thing. Your version of 'dpcls_create_subtable'
already highly depends on the implementation of 'dpcls_subtable_generic_probe'.

> 
> Will change to DEBUG instead of INFO, good idea thanks.
> 
> <snip>
> 


More information about the dev mailing list