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

Van Haaren, Harry harry.van.haaren at intel.com
Wed Jul 17 10:18:01 UTC 2019


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

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

<snip>


More information about the dev mailing list