Re: [dpdk-dev] [PATCH] app/testpmd: add profiling for Rx/Tx burst routines

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Jun 28, 2019 at 02:45:13PM +0100, Iremonger, Bernard wrote:
> Hi Bruce, Slava,
> 
> > -----Original Message-----
> > From: Slava Ovsiienko [mailto:viacheslavo@mellanox.com]
> > Sent: Thursday, June 27, 2019 5:49 AM
> > To: Richardson, Bruce <bruce.richardson@intel.com>
> > Cc: dev@dpdk.org; Iremonger, Bernard <bernard.iremonger@intel.com>;
> > Yigit, Ferruh <ferruh.yigit@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH] app/testpmd: add profiling for Rx/Tx burst
> > routines
> > 
> > OK, what do you think about this:
> > 
> > #ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
> >  if (record_cycle & RECORD_TX_CORE_CYCLES) {
> >    .. do measurement stuff ..
> >  }
> > #endif
> > 
> > + add some new command to config in runtime: "set record_cycle 3"
> > 
> > We keep existing RTE_TEST_PMD_RECORD_CORE_CYCLES, do not introduce
> > new build-time configs and get some new runtime configuring.
> > 
> > WBR,
> > Slava
> > 
> > > -----Original Message-----
> > > From: Bruce Richardson <bruce.richardson@intel.com>
> > > Sent: Wednesday, June 26, 2019 16:21
> > > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > Cc: dev@dpdk.org; bernard.iremonger@intel.com; ferruh.yigit@intel.com
> > > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: add profiling for Rx/Tx
> > > burst routines
> > >
> > > On Wed, Jun 26, 2019 at 01:19:24PM +0000, Slava Ovsiienko wrote:
> > > > Hi, Bruce
> > > >
> > > > Do you mean using "if (core_rx_cycle_enabled) {...}" instead of #ifdef ?
> > > >
> > > > No, I did not try runtime control settings.
> > > > Instead I compared performance with all RECORD_CORE_XX_CYCLES
> > > options
> > > > enabled/disabled builds and have seen the ~1-2% performance
> > > > difference
> > > on my setups (mainly fwd txonly with retry).
> > > > So, ticks measuring is not free.
> > > >
> > > > With best regards,
> > > > Slava
> > > >
> > > Yes, I realise that measuring ticks is going to have a performance impact.
> > > However, what I was referring to was exactly the former - using an "if"
> > > rather than an "ifdef". I would hope with ticks disable using this
> > > option shows no perf impact, and we can reduce the use of build-time
> > configs.
> > >
> > > /Bruce
> 
> Given that  RTE_TEST_PMD_RECORD_CORE_CYCLES is already in the config file.
> I think it is better to be consistent and add the new RECORD macros there.
> 
> Would it be reasonable to have runtime settings available as well?
> 
That configuration option is only present right now for the make builds, so
I'd like to see it replaced with a runtime option rather than see about
adding more config options to the meson build. The first step should be to
avoid adding more config options and just look to use dynamic ones. Ideally
the existing build option should be replaced at the same time.

/Bruce