Re: [dpdk-dev] [PATCH v2 3/3] app/testpmd: qualify profiling statistics on burst size

[Jerin Jacob <jerinjacobk@gmail.com>]

On Thu, Apr 9, 2020 at 5:16 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 3/20/2020 6:13 AM, Jerin Jacob wrote:
> > On Thu, Mar 19, 2020 at 7:21 PM Viacheslav Ovsiienko
> > <viacheslavo@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 <viacheslavo@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,

Hi Ferruh,

> In this usage, compiler will removed the "if (0) { }" block, right?

Yes.

> I think this is good idea, we can use it other places too, including this one.

Yes.