2.6.18-rt7: rollover with 32-bit cycles_t

2.6.18-rt7: rollover with 32-bit cycles_t

[Kevin Hilman]

On ARM, I'm noticing the 'bug' message from check_critical_timing()
where two calls to get_cycles() are compared and the 2nd is assumed to
be >= the first.

This isn't properly handling the case of rollover which occurs
relatively often with fast hardware clocks and 32-bit cycle counters.

Is this really a bug?  If the get_cycles() can be assumed to run between
0 and (cycles_t)~0, using the right unsigned math could get a proper
delta even in the rollover case.  Is this a safe assumption?

Kevin

Re: 2.6.18-rt7: rollover with 32-bit cycles_t

[john cooper]

Kevin Hilman wrote:
> On ARM, I'm noticing the 'bug' message from check_critical_timing()
> where two calls to get_cycles() are compared and the 2nd is assumed to
> be >= the first.
> 
> This isn't properly handling the case of rollover which occurs
> relatively often with fast hardware clocks and 32-bit cycle counters.
> 
> Is this really a bug?  If the get_cycles() can be assumed to run between
> 0 and (cycles_t)~0, using the right unsigned math could get a proper
> delta even in the rollover case.  Is this a safe assumption?

I was concerned about that as well back when I was getting the
instrumentation functional on the pxa270 as the rollover could
be as short as ~8s which wasn't really long enough for test
validation.  However there is a /64 prescaler on that ARM core
(and others) which can function as a bandaid and extend the range
to ~8minutes.

I wasn't able to convince myself in a quick read of the code if
roll over was being detected/corrected (though it certainly may).
But even if not the only requirement needed to do so would be to
assure any two consecutive data points logged per CPU were spaced
apart less than the counter's wrap interval.  Given the periodic
events being logged in the normal course of operation this condition
would certainly be met.

-john

-- 
john.cooper@third-harmonic.com

Re: 2.6.18-rt7: rollover with 32-bit cycles_t

[Daniel Walker]

On Tue, 2006-11-07 at 17:36 -0800, Kevin Hilman wrote:
> On ARM, I'm noticing the 'bug' message from check_critical_timing()
> where two calls to get_cycles() are compared and the 2nd is assumed to
> be >= the first.
> 
> This isn't properly handling the case of rollover which occurs
> relatively often with fast hardware clocks and 32-bit cycle counters.
> 
> Is this really a bug?  If the get_cycles() can be assumed to run between
> 0 and (cycles_t)~0, using the right unsigned math could get a proper
> delta even in the rollover case.  Is this a safe assumption?

Seems like the check should really be using something like time_before()
time_after() which takes the rollover into account .. What I don't
understand is why we don't see those on x86 ..

Daniel

Re: 2.6.18-rt7: rollover with 32-bit cycles_t

[john cooper]

Daniel Walker wrote:

> Seems like the check should really be using something like time_before()
> time_after() which takes the rollover into account .. What I don't
> understand is why we don't see those on x86 ..

Probably due to the fact it is a 64-bit counter.  Even with
a free running rate of 10Ghz it would take nearly 60 years
to wrap.

On PPC and ARM 32-bit counters seems to be common which limits
range unless a prescaler is available.  The better solution is
to detect wrap as a prescaled 32 bit measurement will eventually
run out of usable resolution as core frequency increases.  That is
assuming 64-bit counters don't eventually show up in these
architectures.

-john

-- 
john.cooper@third-harmonic.com

Re: 2.6.18-rt7: rollover with 32-bit cycles_t

[Kevin Hilman]

john cooper wrote:
> Kevin Hilman wrote:
>> On ARM, I'm noticing the 'bug' message from check_critical_timing()
>> where two calls to get_cycles() are compared and the 2nd is assumed to
>> be >= the first.
>>
>> This isn't properly handling the case of rollover which occurs
>> relatively often with fast hardware clocks and 32-bit cycle counters.
>>
>> Is this really a bug?  If the get_cycles() can be assumed to run between
>> 0 and (cycles_t)~0, using the right unsigned math could get a proper
>> delta even in the rollover case.  Is this a safe assumption?
> 
> I was concerned about that as well back when I was getting the
> instrumentation functional on the pxa270 as the rollover could
> be as short as ~8s which wasn't really long enough for test
> validation.  However there is a /64 prescaler on that ARM core
> (and others) which can function as a bandaid and extend the range
> to ~8minutes.
> 
> I wasn't able to convince myself in a quick read of the code if
> roll over was being detected/corrected (though it certainly may).
> But even if not the only requirement needed to do so would be to
> assure any two consecutive data points logged per CPU were spaced
> apart less than the counter's wrap interval.  Given the periodic
> events being logged in the normal course of operation this condition
> would certainly be met.

Looking closer, given the assumptions you stated, I'm pretty convinced
that that check and 'bug' is unncessary in the normal case.  The delta
generated with unsigned math is correct.

However, it seems this check is more intended to check for
broken/backward-running clocks.  In which case, time_after() or
equivalent should be used.

Here's a patch to use time_after() in the 32-bit case, but we probably
really need time_after() macros which can handle 32- or 64-bit types.


Index: linux-2.6.18/kernel/latency_trace.c
===================================================================
--- linux-2.6.18.orig/kernel/latency_trace.c
+++ linux-2.6.18/kernel/latency_trace.c
@@ -1609,8 +1609,14 @@ check_critical_timing(int cpu, struct cp
 	 * is fair to be reported):
 	 */
 	T2 = get_cycles();
-	if (T2 < T1)
+
+	/* check for buggy clocks, handling wrap for 32-bit clocks */
+	if (TYPE_EQUAL(cycles_t, unsigned long)) {
+		if (time_after(T1, T2))
+			printk("bug: %08x < %08x!\n", T2, T1);
+	} else if (T2 < T1)
 		printk("bug: %016Lx < %016Lx!\n", T2, T1);
+	
 	delta = T2-T0;

 	latency = cycles_to_usecs(delta);

Re: 2.6.18-rt7: rollover with 32-bit cycles_t

[Ingo Molnar]

* Kevin Hilman <khilman@mvista.com> wrote:

> -	if (T2 < T1)
> +
> +	/* check for buggy clocks, handling wrap for 32-bit clocks */
> +	if (TYPE_EQUAL(cycles_t, unsigned long)) {
> +		if (time_after(T1, T2))
> +			printk("bug: %08x < %08x!\n", T2, T1);
> +	} else if (T2 < T1)
>  		printk("bug: %016Lx < %016Lx!\n", T2, T1);
> +	

ok, i have applied this one.

	Ingo