[ovs-dev] [v4 03/12] dpif-netdev: Add study function to select the best mfex function

[Stokes, Ian]

> > -----Original Message-----
> > From: Stokes, Ian <ian.stokes at intel.com>
> > Sent: Thursday, June 24, 2021 2:20 PM
> > To: Amber, Kumar <kumar.amber at intel.com>; dev at openvswitch.org; Van
> > Haaren, Harry <harry.van.haaren at intel.com>
> > Cc: Amber, Kumar <kumar.amber at intel.com>; i.maximets at ovn.org
> > Subject: RE: [ovs-dev] [v4 03/12] dpif-netdev: Add study function to select the
> > best mfex function
> >
> > > The study function runs all the available implementations
> > > of miniflow_extract and makes a choice whose hitmask has
> > > maximum hits and sets the mfex to that function.
> >
> > Hi Amber/Harry,
> >
> > Thanks for the patch, a few comments inline below.
> 
> Thanks for review. Just addressing the stats get/TLS topic here.
> <snip other patch changes>
> 
> > > +/* Struct to hold miniflow study stats. */
> > > +struct study_stats {
> > > +    uint32_t pkt_count;
> > > +    uint32_t impl_hitcount[MFEX_IMPLS_MAX_SIZE];
> > > +};
> > > +
> > > +/* Define per thread data to hold the study stats. */
> > > +DEFINE_PER_THREAD_MALLOCED_DATA(struct study_stats *, study_stats);
> > > +
> > > +/* Allocate per thread PMD pointer space for study_stats. */
> > > +static inline struct study_stats *
> > > +get_study_stats(void)
> >
> > Would maybe suggest a name change here, get_study_stats sounds as if info is
> > being returned whereas whats actually happening is that the memory for the
> > stats are being provisioned.
> 
> More context for explaining below...
> 
> > > +{
> > > +    struct study_stats *stats = study_stats_get();
> > > +    if (OVS_UNLIKELY(!stats)) {
> > > +       stats = xzalloc(sizeof *stats);
> > > +       study_stats_set_unsafe(stats);
> > Can you explain why above is set unsafe? Where does that function originate
> > from?
> 
> Yes, this is how the OVS "per thread data" (also called "Thread Local Storage" or
> TLS)
> is implemented. The "get()" function indeed allocates the memory first time that
> this
> thread actually accesses it, and any time after that it just returns the per-thread
> allocated
> data pointer.
> 

Ah that makes more sense, have followed up on the existing code since and indeed it follows the same logic.

> The "unsafe" is essentially the API used to change a TLS variable. It must only be
> called
> by the thread that's using it itself, hence the unsafe() AFAIK.
> 
> The same function naming etc is used in DPCLS already, where this was the
> recommended
> method of getting/using TLS data.
> 
> dpif-netdev-lookup-generic.c +47   function has "get_blocks_scratch()" which
> performs
> approximately the same functionality as here.
> 
> Hope that clears up that topic, regards, -Harry

Thanks for clarifying.

BR
Ian