diff mbox series

[v2,2/2] eal: roundup tsc frequency when estimating it

Message ID 20190316070302.32432-2-pbhagavatula@marvell.com (mailing list archive)
State Superseded, archived
Headers
Series [v2,1/2] eal: add macro to align value to the nearest multiple |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Pavan Nikhilesh Bhagavatula March 16, 2019, 7:03 a.m. UTC 
  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.

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(-)

--
2.21.0

Comments

Wiles, Keith March 16, 2019, 2:42 p.m. UTC | #1 
> 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.
> 
> 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(-)
> 
> 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);
> }
> 
> 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;
> }
> 
> 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.
> 	}
> #endif
> 	return 0;
> --
> 2.21.0
> 

Regards,
Keith
Pavan Nikhilesh Bhagavatula March 16, 2019, 3:06 p.m. UTC | #2 
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.
> > 
> > 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(-)
> > 
> > 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);
> > }
> > 
> > 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." 

> > 	}
> > #endif
> > 	return 0;
> > --
> > 2.21.0
> > 
> 
> Regards,
> Keith
>
Wiles, Keith March 16, 2019, 5:18 p.m. UTC | #3 
> 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
Pavan Nikhilesh Bhagavatula March 16, 2019, 5:56 p.m. UTC | #4 
On Sat, 2019-03-16 at 17:18 +0000, Wiles, Keith wrote:
> > 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.

Example in case of RTE_ARM_EAL_RDTSC_USE_PMU enabled 

Before roundup : 1400000979
After roundup : 1400000000
EAL: TSC frequency is ~1400000000 Hz


Before roundup : 1399999060
After roundup : 1400000000
EAL: TSC frequency is ~1400000000 Hz

> > > > 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.

1E7 ~ 10Mhz will convert to a macro.

> > > > }
> > > > 
> > > > 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 + (1000 - 1)) / 1000) * 1000
> 	(10001 + 999)        / 1000000
> 	20000                / 1000000
> Result: 0

((10001 + (1000 - 1) / 1000) * 1000
((10001 + 999) / 1000) * 1000
(11000/1000) * 1000
11 * 1000 

Result : 11000

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

Result : 1000

> 
> 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.

Unless we have machines that run at freq < 10Mhz this scheme will
always work.
If we have such machines lets hope that they have a CPU instruction
that tells us the cyc/sec.

> 
> 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
> 

Regards,
Pavan.
Wiles, Keith March 16, 2019, 6:22 p.m. UTC | #5 
> On Mar 16, 2019, at 12:56 PM, Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> wrote:
> 
> On Sat, 2019-03-16 at 17:18 +0000, Wiles, Keith wrote:
>>> 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.
> 
> Example in case of RTE_ARM_EAL_RDTSC_USE_PMU enabled 
> 
> Before roundup : 1400000979
> After roundup : 1400000000
> EAL: TSC frequency is ~1400000000 Hz
> 
> 
> Before roundup : 1399999060
> After roundup : 1400000000
> EAL: TSC frequency is ~1400000000 Hz
> 
>>>>> 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.
> 
> 1E7 ~ 10Mhz will convert to a macro.
> 
>>>>> }
>>>>> 
>>>>> 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 + (1000 - 1)) / 1000) * 1000
>> 	(10001 + 999)        / 1000000
>> 	20000                / 1000000
>> Result: 0
> 
> ((10001 + (1000 - 1) / 1000) * 1000
> ((10001 + 999) / 1000) * 1000
> (11000/1000) * 1000
> 11 * 1000 
> 
> Result : 11000
> 
>> 
>> RTE_ALIGN_MUL_FLOOR(10001, 1000)
>> 	(10001 / (1000 * 1000))
> (10001 / 1000) * 1000
>> 	(10001 / 1000000)
>> Result: 0
> 10.001 * 1000
> 
> Result : 1000

Ooops, too many parans and missed it.

Then we can get a new version and that should be OK.

I will add my $0.02 then:

Reviewed-by: Keith Wiles<keith.wiles>

> 
>> 
>> 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.
> 
> Unless we have machines that run at freq < 10Mhz this scheme will
> always work.
> If we have such machines lets hope that they have a CPU instruction
> that tells us the cyc/sec.
> 
>> 
>> 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
>> 
> 
> Regards,
> Pavan.

Regards,
Keith
Pavan Nikhilesh Bhagavatula March 16, 2019, 6:27 p.m. UTC | #6 
On Sat, 2019-03-16 at 18:22 +0000, Wiles, Keith wrote:
> > On Mar 16, 2019, at 12:56 PM, Pavan Nikhilesh Bhagavatula <
> > pbhagavatula@marvell.com> wrote:
> > 
> > On Sat, 2019-03-16 at 17:18 +0000, Wiles, Keith wrote:
> > > > 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.
> > 
> > Example in case of RTE_ARM_EAL_RDTSC_USE_PMU enabled 
> > 
> > Before roundup : 1400000979
> > After roundup : 1400000000
> > EAL: TSC frequency is ~1400000000 Hz
> > 
> > 
> > Before roundup : 1399999060
> > After roundup : 1400000000
> > EAL: TSC frequency is ~1400000000 Hz
> > 
> > > > > > 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.
> > 
> > 1E7 ~ 10Mhz will convert to a macro.
> > 
> > > > > > }
> > > > > > 
> > > > > > 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 + (1000 - 1)) / 1000) * 1000
> > > 	(10001 + 999)        / 1000000
> > > 	20000                / 1000000
> > > Result: 0
> > 
> > ((10001 + (1000 - 1) / 1000) * 1000
> > ((10001 + 999) / 1000) * 1000
> > (11000/1000) * 1000
> > 11 * 1000 
> > 
> > Result : 11000
> > 
> > > RTE_ALIGN_MUL_FLOOR(10001, 1000)
> > > 	(10001 / (1000 * 1000))
> > (10001 / 1000) * 1000
> > > 	(10001 / 1000000)
> > > Result: 0
> > 10.001 * 1000
> > 
> > Result : 10000
> 
> Ooops, too many parans and missed it.
> 
> Then we can get a new version and that should be OK.

Yup, thanks for reviewing :-).

> 
> I will add my $0.02 then:
> 
> Reviewed-by: Keith Wiles<keith.wiles>
> 
> > > 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.
> > 
> > Unless we have machines that run at freq < 10Mhz this scheme will
> > always work.
> > If we have such machines lets hope that they have a CPU instruction
> > that tells us the cyc/sec.
> > 
> > > 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
> > > 
> > 
> > Regards,
> > Pavan.
> 
> Regards,
> Keith
>
diff mbox series

Patch

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);
 }

 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;
 }

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);
 	}
 #endif
 	return 0;