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

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

> -----Original Message-----
> From: Chalupnik, KamilX
> Sent: Tuesday, May 8, 2018 10:48 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
> 
> Hi Pablo,
> 
> > -----Original Message-----
> > From: De Lara Guarch, Pablo
> > Sent: Tuesday, May 8, 2018 11:08 AM
> > 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
> >
> > 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.
> >
> 
> So should I create and use in driver new guard for 'offload_time' and add it to
> config/common_base (or set it in Makefile when RTE_TEST_BBDEV is set). Are
> you ok for such exception to the DPDK code guidelines in this case?

I would leave "offload_time" always available in the structure, so its size doesn't change
depending on this option.

It is better to avoid adding these options, but if we do it for perf reasons (if it affects data path code),
then at least it should be well designed, so there should be a separate option (either under TURBO_SW or BBDEV),
and I think it should be disabled by default.

The test app is enabled by default, so users most likely would have this enabled without needing it,
impacting performance. Therefore, I'd say it is better to avoid relating this to the test app.
You can document that this option should be enabled if offload times are wanted
(and maybe check in the test app if the compilation flag is set and show a message to enable it if user is interested).

> 
> Best regards,
> Kamil