Re: [dpdk-dev] [PATCH 09/13] bbdev: measure offload cost

["De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>]

Hi Kamil,

> -----Original Message-----
> From: Chalupnik, KamilX
> Sent: Tuesday, May 8, 2018 8:56 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Cc: Mokhtar, Amr <amr.mokhtar@intel.com>
> Subject: RE: [PATCH 09/13] bbdev: measure offload cost
> 
> 
> 
> > -----Original Message-----
> > From: De Lara Guarch, Pablo
> > Sent: Monday, May 7, 2018 3:29 PM
> > To: Chalupnik, KamilX <kamilx.chalupnik@intel.com>; dev@dpdk.org
> > Cc: Mokhtar, Amr <amr.mokhtar@intel.com>
> > Subject: RE: [PATCH 09/13] bbdev: measure offload cost
> >
> >
> >
> > > -----Original Message-----
> > > From: Chalupnik, KamilX
> > > Sent: Thursday, April 26, 2018 2:30 PM
> > > To: dev@dpdk.org
> > > Cc: Mokhtar, Amr <amr.mokhtar@intel.com>; De Lara Guarch, Pablo
> > > <pablo.de.lara.guarch@intel.com>; Chalupnik, KamilX
> > > <kamilx.chalupnik@intel.com>
> > > Subject: [PATCH 09/13] bbdev: measure offload cost
> > >
> >
> > ...
> >
> > > --- a/lib/librte_bbdev/rte_bbdev.h
> > > +++ b/lib/librte_bbdev/rte_bbdev.h
> > > @@ -239,6 +239,10 @@ struct rte_bbdev_stats {
> > >  	uint64_t enqueue_err_count;
> > >  	/** Total error count on operations dequeued */
> > >  	uint64_t dequeue_err_count;
> > > +#ifdef RTE_TEST_BBDEV
> > > +	/** It stores offload time. */
> >
> > Just "offload time" is fine.
> >
> > > +	uint64_t offload_time;
> > > +#endif
> >
> > Again, I don't think it is a good idea to have this compilation check.
> > RTE_TEST_BBDEV is used to enable the compilation of the test app, so
> > it shouldn't be used for anything else.
> > Also, in DPDK, we are avoiding the usage of this conditionals to
> > enable/disable pieces of code.
> 
> > If you want to avoid the computation of this time, add a configuration
> > option in bbdev configuration structure (rte_bbdev_queue_conf?), so
> > the decision is made at runtime.
> 
> Ok, but this 'offload_time' variable is used only for testing purposes so we
> decided that it should exist only when test application is being built.
> In case where we move the decision to runtime phase it may have bad impact on
> driver performance because then each time we have to check if 'offload_time'
> has to be calculated or not.

I understand the performance penalty. Anyway, if you go for build time check,
you should have another option name. The test app option should not affect the code in a library/PMD.
Also, this option should probably be disabled, to avoid the extra calculations unnecessarily.
Lastly, I think it would be better to always have "offload_time" field in the structure,
so remove the check there.

Thanks,
Pablo

> 
> > >  };
> > >
> > >  /**
> > > --
> > > 2.5.5