Hi Ferruh,
Thanks for your comments.
<snip>
> Subject: Re: [PATCH v2 1/5] app/testpmd: clock gettime call in throughput
> calculation
>
> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> > The throughput calculation requires a counter that measures passing of
> > time. However, the kernel saves and restores the PMU state when a
> > thread is unscheduled and scheduled. This ensures that the PMU cycles
> > are not counted towards a thread that is not scheduled. Hence, when
> > RTE_ARM_EAL_RDTSC_USE_PMU is enabled, the PMU cycles do not
> represent
> > the passing of time.
>
> Does this mean 'rte_rdtsc()' is broken for Arm?
It depends on the kernel I think. IMO, for isolated CPUs it should be fine. It
is currently getting fixed through a kernel patch. Once the kernel patch is up
streamed, we will make the required changes in DPDK.
> Wouldn't it cause more serious issue than testpmd throughput stats?
Within DPDK, it does not seem to be used for anything critical (I have fixed
one).
>
> > This results in incorrect calculation of throughput numbers.
> > Use clock_gettime system call to calculate the time passed since last
> > call.
> >
> > Bugzilla ID: 450
> > Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
> > Cc: zhihong.w...@intel.com
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> > Reviewed-by: Phil Yang <phil.y...@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.w...@arm.com>
> > Tested-by: Phil Yang <phil.y...@arm.com>
> > Tested-by: Ali Alnubani <alia...@mellanox.com>
>
> <...>
>
> > @@ -195,10 +204,17 @@ nic_stats_display(portid_t port_id)
> > }
> > }
> >
> > - diff_cycles = prev_cycles[port_id];
> > - prev_cycles[port_id] = rte_rdtsc();
> > - if (diff_cycles > 0)
> > - diff_cycles = prev_cycles[port_id] - diff_cycles;
> > + diff_ns = 0;
> > + if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
>
> I guess 'rte_rdtsc()' is faster, but since this is not in the data path, I
> think it is
> OK.
>
> <...>
>
> > @@ -217,10 +233,10 @@ nic_stats_display(portid_t port_id)
> > (stats.obytes - prev_bytes_tx[port_id]) : 0;
> > prev_bytes_rx[port_id] = stats.ibytes;
> > prev_bytes_tx[port_id] = stats.obytes;
> > - mbps_rx = diff_cycles > 0 ?
> > - diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
> > - mbps_tx = diff_cycles > 0 ?
> > - diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
> > + mbps_rx = diff_ns > 0 ?
> > + (double)diff_bytes_rx / diff_ns * NS_PER_SEC : 0;
> > + mbps_tx = diff_ns > 0 ?
> > + (double)diff_bytes_tx / diff_ns * NS_PER_SEC : 0;
>
> The calculation also fixes other issue in the stats.
> With previous method, if the sampling between two stats is a little long,
> "diff_pkts_rx * rte_get_tsc_hz()" can overflow and produce wrong 'bps'.
>
>
> Reviewed-by: Ferruh Yigit <ferruh.yi...@intel.com>
