Re: [dpdk-dev] [EXT] Re: [PATCH v2 2/2] eal: roundup tsc frequency when estimating it

["Wiles, Keith" <keith.wiles@intel.com>]

> On Mar 16, 2019, at 10:06 AM, Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> wrote:
> 
> On Sat, 2019-03-16 at 14:42 +0000, Wiles, Keith wrote:
>>> On Mar 16, 2019, at 2:03 AM, Pavan Nikhilesh Bhagavatula <
>>> pbhagavatula@marvell.com> wrote:
>>> 
>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> 
>>> When estimating tsc frequency using sleep/gettime round it up to
>>> the
>>> nearest multiple of 10Mhz for more accuracy.

How does rounding up the TSC value become more accurate, If the value is 1 cycles more then it should be then your macro would round down and if it is 1 cycle greater than 1E7 it would round up.
>>> 
>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> ---
>>> Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
>>> get_tsc_freq_arch() will return 0 as there is no instruction to
>>> determine
>>> the clk of PMU and eal falls back to sleep(1).
>>> 
>>> lib/librte_eal/common/eal_common_timer.c | 4 ++--
>>> lib/librte_eal/linuxapp/eal/eal_timer.c  | 2 +-
>>> 2 files changed, 3 insertions(+), 3 deletions(-)

It appears you did not use the head of the master as linuxapp is now just linux and freebsdapp is freebsd. You need to rebase to the head of master and send a new version.
>>> 
>>> diff --git a/lib/librte_eal/common/eal_common_timer.c
>>> b/lib/librte_eal/common/eal_common_timer.c
>>> index dcf26bfea..1358bbed0 100644
>>> --- a/lib/librte_eal/common/eal_common_timer.c
>>> +++ b/lib/librte_eal/common/eal_common_timer.c
>>> @@ -69,7 +69,7 @@ estimate_tsc_freq(void)
>>> 	/* assume that the sleep(1) will sleep for 1 second */
>>> 	uint64_t start = rte_rdtsc();
>>> 	sleep(1);
>>> -	return rte_rdtsc() - start;
>>> +	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, 1E7);

The 1E7 is a magic number convert this to a meaningful define.
>>> }
>>> 
>>> void
>>> @@ -83,7 +83,7 @@ set_tsc_freq(void)
>>> 	if (!freq)
>>> 		freq = estimate_tsc_freq();
>>> 
>>> -	RTE_LOG(DEBUG, EAL, "TSC frequency is ~%" PRIu64 " KHz\n", freq
>>> / 1000);
>>> +	RTE_LOG(INFO, EAL, "TSC frequency is ~%" PRIu64 " Hz\n", freq);
>>> 	eal_tsc_resolution_hz = freq;
> 
> I missed this log will remove it in the next version.
> 
>>> }
>>> 
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> b/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> index bc8f05199..864d6ef29 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
>>> @@ -248,7 +248,7 @@ get_tsc_freq(void)
>>> 
>>> 		double secs = (double)ns/NS_PER_SEC;
>>> 		tsc_hz = (uint64_t)((end - start)/secs);
>>> -		return tsc_hz;
>>> +		return RTE_ALIGN_MUL_NEAR(tsc_hz, 1E7);
>> 
>> Maybe I missed an email about this, but why would I want the TSC hz
>> rounded here? I do not mind the macro just the fact that we are
>> changing TSC hz value. If the TSC value is wrong then we need to fix
>> the value, but I do not see it being wrong here.
> 
> Since in this function nanosleep might not be cycle accurate we need to
> round it up.
> 
> Please note that estimation only applies when  get_tsc_freq_arch()
> fails. i.e there is no CPU instruction that specifies the cyc/sec.
> 
> As I mentioned in the patch notes
> "Useful in case of ARM64 if we enable RTE_ARM_EAL_RDTSC_USE_PMU,
> get_tsc_freq_arch() will return 0 as there is no instruction to
> determine the clock of PMU and eal falls back to sleep(1)/nanosleep.” 

OK, I looked at the changes and the code for setting the TSC again. I would have not handled the setting of TSC in the way it was done, but that is not your problem. I agree the changes do look ok, the only problem I have is the new macro will roundup or down depending on the value. In your statement you are wanting to roundup the values.

If the sleep/nanosleep is less than a second for some reason, then your macro would round it down is that what we wanted? I guess my point is you are assuming the TSC calculation will always be less than a second (with sleep) and the macro would round up depending on the value calculated using the sleep/nanosleep.

I was playing with these MUL macros and I am not sure they do what we expect in the case of the multiple value is much closer to the value passed.

If we have a v = 10001 and multiple to 1000 we have the following:

RTE_ALIGN_MUL_CEIL(10001, 1000)
	(10001 + (1000 - 1)) / (1000 * 1000)
	(10001 + 999)        / 1000000
	20000                / 1000000
Result: 0

RTE_ALIGN_MUL_FLOOR(10001, 1000)
	(10001 / (1000 * 1000))
	(10001 / 1000000)
Result: 0

Unless I am wrong here the value v must be over a 1,000,000 to make these macros work or the value v to be greater than (mul * mul) in all cases or zero is the result. It may work for the TSC values as we are using a small mul value compared to the TSC value. If DPDK was ported to a slower machine it could be also zero.

I think we need to fix the macros and rethink how TSC is set here.

> 
>>> 	}
>>> #endif
>>> 	return 0;
>>> --
>>> 2.21.0
>>> 
>> 
>> Regards,
>> Keith

Regards,
Keith