On 3/20/2020 6:13 AM, Jerin Jacob wrote: > On Thu, Mar 19, 2020 at 7:21 PM Viacheslav Ovsiienko > <viachesl...@mellanox.com> wrote: >> >> The execution time of rx/tx burst routine depends on the burst size. >> It would be meaningful to research this dependency, the patch >> provides an extra profiling data per rx/tx burst size. >> >> Signed-off-by: Viacheslav Ovsiienko <viachesl...@mellanox.com> > >> +#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES >> + if (!(fwdprof_flags & (nrx_tx ? RECORD_CORE_CYCLES_TX >> + : RECORD_CORE_CYCLES_RX))) >> + return; >> + for (nb_pkt = 0; nb_pkt < MAX_PKT_BURST; nb_pkt++) { >> + nb_burst = nrx_tx ? pbs->pkt_retry_spread[nb_pkt] >> + : pbs->pkt_burst_spread[nb_pkt]; >> + if (nb_burst == 0) >> + continue; >> + printf(" CPU cycles/%u packet burst=%u (total cycles=" >> + "%"PRIu64" / total %s bursts=%u)\n", >> + (unsigned int)nb_pkt, >> + (unsigned int)(pbs->pkt_ticks_spread[nb_pkt] / >> nb_burst), >> + pbs->pkt_ticks_spread[nb_pkt], >> + nrx_tx ? "TX" : "RX", nb_burst); >> + } >> +#endif > > > # Thanks for this feature, IMO, It worth to mention in release notes. > > # I see a lot of code get added under RTE_TEST_PMD_RECORD_CORE_CYCLES. > I agree that it should be under RTE_TEST_PMD_RECORD_CORE_CYCLES. Since > this flag is not > by default, there is a risk of compilation issue when this flag is get > enabled. > IMO, it is better to move to _if (0)_ semantics to enable > - performance when compiler opt-out the code. > - It forces the compiler to check the compilation errors irrespective > of the RTE_TEST_PMD_RECORD_CORE_CYCLES state. > > Something like below, > > static __rte_always_inline int > testpmd_has_stats_feature(void) > { > #ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES > return RTE_TEST_PMD_RECORD_CORE_CYCLES ; > #else > return 0; > #endif > } > > > if (testpmd_has_stats_feature()) { > > } >
Hi Jerin, In this usage, compiler will removed the "if (0) { }" block, right? I think this is good idea, we can use it other places too, including this one.