I think we should compromise: keep existing RTE_TEST_PMD_RECORD_CORE_CYCLES and extend with runtime switch under this build-time option:
#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES if (record_tx) .. gather tx related stats... if (record_rx) .. gather rx related stats... #endif This is very specific feature, it is needed while debugging and testing datapath routines, and It seems this feature with appropriate overhead should not be always enabled. existing build-time configuration options looks OK as for me. Bruce, if proposed runtime extension is acceptable - I will update the patch. WBR, Slava > -----Original Message----- > From: Bruce Richardson <bruce.richard...@intel.com> > Sent: Friday, June 28, 2019 17:20 > To: Iremonger, Bernard <bernard.iremon...@intel.com> > Cc: Slava Ovsiienko <viachesl...@mellanox.com>; dev@dpdk.org; Yigit, > Ferruh <ferruh.yi...@intel.com> > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: add profiling for Rx/Tx burst > routines > > On Fri, Jun 28, 2019 at 02:45:13PM +0100, Iremonger, Bernard wrote: > > Hi Bruce, Slava, > > > > > -----Original Message----- > > > From: Slava Ovsiienko [mailto:viachesl...@mellanox.com] > > > Sent: Thursday, June 27, 2019 5:49 AM > > > To: Richardson, Bruce <bruce.richard...@intel.com> > > > Cc: dev@dpdk.org; Iremonger, Bernard <bernard.iremon...@intel.com>; > > > Yigit, Ferruh <ferruh.yi...@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.richard...@intel.com> > > > > Sent: Wednesday, June 26, 2019 16:21 > > > > To: Slava Ovsiienko <viachesl...@mellanox.com> > > > > Cc: dev@dpdk.org; bernard.iremon...@intel.com; > > > > ferruh.yi...@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
