On 17.07.2019 13:18, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Ilya Maximets [mailto:[email protected]] >> Sent: Wednesday, July 17, 2019 10:52 AM >> To: Van Haaren, Harry <[email protected]>; [email protected] >> Cc: [email protected]; [email protected]; Stokes, Ian >> <[email protected]> >> 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> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
