[dpdk-stable] [PATCH v2 1/5] app/testpmd: clock gettime call in throughput calculation

[Ferruh Yigit]

On 7/6/2020 5:53 PM, Honnappa Nagarahalli wrote:
> 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).

OK, thanks for clarification.

> 
>>
>>> 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.wang at intel.com
>>> Cc: stable at dpdk.org
>>>
>>> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli at arm.com>
>>> Reviewed-by: Phil Yang <phil.yang at arm.com>
>>> Reviewed-by: Ruifeng Wang <ruifeng.wang at arm.com>
>>> Tested-by: Phil Yang <phil.yang at arm.com>
>>> Tested-by: Ali Alnubani <alialnu at 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.yigit at intel.com>