> -----Original Message----- > From: Ilya Maximets [mailto:i.maxim...@samsung.com] > Sent: Wednesday, July 17, 2019 10:52 AM > To: Van Haaren, Harry <harry.van.haa...@intel.com>; d...@openvswitch.org > Cc: echau...@redhat.com; malvika.gu...@arm.com; Stokes, Ian > <ian.sto...@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> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev