[Adeos-main] [PATCH] Fix ipipe_tsc2ns for ARM

[Adeos-main] [PATCH] Fix ipipe_tsc2ns for ARM

[Sebastian Smolorz]

[-- Attachment #1: Type: text/plain, Size: 157 bytes --]

Hi,

the current implementation of ipipe_tsc2ns for ARM does not compile (undefined 
reference to `__udivdi3'). The attached patch fixes that.

--
Sebastian

[-- Attachment #2: ipipe-arm.patch --]
[-- Type: text/x-diff, Size: 629 bytes --]

--- ipipe/v2.6/common/include/asm-arm/ipipe.h	2006-11-29 16:06:33.000000000 +0100
+++ ipipe.work/v2.6/common/include/asm-arm/ipipe.h	2006-11-29 16:08:30.000000000 +0100
@@ -124,7 +124,12 @@ extern void __ipipe_mach_demux_irq(unsig
 #define __ipipe_read_timebase()		__ipipe_mach_get_tsc()
 
 #define ipipe_cpu_freq()	(HZ * __ipipe_mach_ticks_per_jiffy)
-#define ipipe_tsc2ns(t)		(((t) * 1000) / (ipipe_cpu_freq() / 1000000))
+#define ipipe_tsc2ns(t) \
+({ \
+	unsigned long long delta = (t)*1000; \
+	do_div(delta, ipipe_cpu_freq() / 1000000 + 1); \
+	(unsigned long)delta; \
+})
 
 /* Private interface -- Internal use only */
 

Re: [Adeos-main] [PATCH] Fix ipipe_tsc2ns for ARM

[Gilles Chanteperdrix]

Sebastian Smolorz wrote:
> Hi,
> 
> the current implementation of ipipe_tsc2ns for ARM does not compile (undefined 
> reference to `__udivdi3'). The attached patch fixes that.

the point of using ((t) * 1000) / (freq / 1000000) is to avoid using the
64 bits division. I would rather think a simple cast to unsigned is
missing. What value is a 64 bits when it fails,
__ipipe_mach_ticks_per_jiffy or t ?

-- 
                                                 Gilles Chanteperdrix

Re: [Adeos-main] [PATCH] Fix ipipe_tsc2ns for ARM

[Sebastian Smolorz]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

Gilles Chanteperdrix wrote:
> Sebastian Smolorz wrote:
> > Hi,
> >
> > the current implementation of ipipe_tsc2ns for ARM does not compile
> > (undefined reference to `__udivdi3'). The attached patch fixes that.
>
> the point of using ((t) * 1000) / (freq / 1000000) is to avoid using the
> 64 bits division. I would rather think a simple cast to unsigned is
> missing.

Ok, see my attached patch. But can we assume in all cases that ipipe_tsc2ns is 
used with a value smaller than a long long variable? The only usage of this 
macro so far is in __ipipe_print_delay() which calls it with a substraction 
of two unsigned long long variables.

> What value is a 64 bits when it fails, 
> __ipipe_mach_ticks_per_jiffy or t ?

Sorry, I'm not really sure what you mean here.

--
Sebastian

[-- Attachment #2: ipipe-arm_v2.patch --]
[-- Type: text/x-diff, Size: 560 bytes --]

--- ipipe/v2.6/common/include/asm-arm/ipipe.h	2006-11-12 20:33:33.000000000 +0100
+++ ipipe.work/v2.6/common/include/asm-arm/ipipe.h	2006-11-30 10:16:00.000000000 +0100
@@ -124,7 +124,7 @@ extern void __ipipe_mach_demux_irq(unsig
 #define __ipipe_read_timebase()		__ipipe_mach_get_tsc()
 
 #define ipipe_cpu_freq()	(HZ * __ipipe_mach_ticks_per_jiffy)
-#define ipipe_tsc2ns(t)		(((t) * 1000) / (ipipe_cpu_freq() / 1000000))
+#define ipipe_tsc2ns(t)		(((unsigned long)(t) * 1000) / (ipipe_cpu_freq() / 1000000))
 
 /* Private interface -- Internal use only */
 

Re: [Adeos-main] [PATCH] Fix ipipe_tsc2ns for ARM

[Gilles Chanteperdrix]

Sebastian Smolorz wrote:
> Ok, see my attached patch. But can we assume in all cases that ipipe_tsc2ns is 
> used with a value smaller than a long long variable? The only usage of this 
> macro so far is in __ipipe_print_delay() which calls it with a substraction 
> of two unsigned long long variables.

The patch using do_div also assumes that t * 1000 will never overflow 64
bits. But I agree, it is less restrictive.

>>What value is a 64 bits when it fails, 
>>__ipipe_mach_ticks_per_jiffy or t ?
> 
> 
> Sorry, I'm not really sure what you mean here.

Forget it, I got the answer, it is t that is a 64 bits value.

-- 
                                                 Gilles Chanteperdrix

Re: [Adeos-main] [PATCH] Fix ipipe_tsc2ns for ARM

[Sebastian Smolorz]

Gilles Chanteperdrix wrote:
> Sebastian Smolorz wrote:
> > Ok, see my attached patch. But can we assume in all cases that
> > ipipe_tsc2ns is used with a value smaller than a long long variable? The
> > only usage of this macro so far is in __ipipe_print_delay() which calls
> > it with a substraction of two unsigned long long variables.
>
> The patch using do_div also assumes that t * 1000 will never overflow 64
> bits. But I agree, it is less restrictive.
>
> >>What value is a 64 bits when it fails,
> >>__ipipe_mach_ticks_per_jiffy or t ?
> >
> > Sorry, I'm not really sure what you mean here.
>
> Forget it, I got the answer, it is t that is a 64 bits value.

So which patch are you going to check in?

--
Sebastian

Re: [Adeos-main] [PATCH] Fix ipipe_tsc2ns for ARM

[Gilles Chanteperdrix]

Sebastian Smolorz wrote:
> Gilles Chanteperdrix wrote:
> 
>>Sebastian Smolorz wrote:
>>
>>>Ok, see my attached patch. But can we assume in all cases that
>>>ipipe_tsc2ns is used with a value smaller than a long long variable? The
>>>only usage of this macro so far is in __ipipe_print_delay() which calls
>>>it with a substraction of two unsigned long long variables.
>>
>>The patch using do_div also assumes that t * 1000 will never overflow 64
>>bits. But I agree, it is less restrictive.
>>
>>
>>>>What value is a 64 bits when it fails,
>>>>__ipipe_mach_ticks_per_jiffy or t ?
>>>
>>>Sorry, I'm not really sure what you mean here.
>>
>>Forget it, I got the answer, it is t that is a 64 bits value.
> 
> 
> So which patch are you going to check in?

I have no preference, maybe Philippe has one ?

-- 
                                                 Gilles Chanteperdrix

Re: [Adeos-main] [PATCH] Fix ipipe_tsc2ns for ARM

[Philippe Gerum]

On Mon, 2006-12-04 at 12:15 +0100, Gilles Chanteperdrix wrote:
> Sebastian Smolorz wrote:
> > Gilles Chanteperdrix wrote:
> > 
> >>Sebastian Smolorz wrote:
> >>
> >>>Ok, see my attached patch. But can we assume in all cases that
> >>>ipipe_tsc2ns is used with a value smaller than a long long variable? The
> >>>only usage of this macro so far is in __ipipe_print_delay() which calls
> >>>it with a substraction of two unsigned long long variables.
> >>
> >>The patch using do_div also assumes that t * 1000 will never overflow 64
> >>bits. But I agree, it is less restrictive.
> >>
> >>
> >>>>What value is a 64 bits when it fails,
> >>>>__ipipe_mach_ticks_per_jiffy or t ?
> >>>
> >>>Sorry, I'm not really sure what you mean here.
> >>
> >>Forget it, I got the answer, it is t that is a 64 bits value.
> > 
> > 
> > So which patch are you going to check in?
> 
> I have no preference, maybe Philippe has one ?
> 

It's a detail, but ipipe_tsc2ns() was once designed as a helper for
implementing the now defunct I-pipe stats support, which has been
replaced by the tracer. The values (time delta between -normally- short
events) were guaranteed to be small enough to fit in 32bits, so other
ports just cast the input data within the macro. No other I-pipe code
should rely on better precision.

This said, in this case, my preferred option is the one of the Adeos
maintainer for ARM, who is... oh, yeah! you are. So, I hereby agree with
the choice you are going to make next.

-- 
Philippe.

Re: [Adeos-main] [PATCH] Fix ipipe_tsc2ns for ARM

[Gilles Chanteperdrix]

Philippe Gerum wrote:
> It's a detail, but ipipe_tsc2ns() was once designed as a helper for
> implementing the now defunct I-pipe stats support, which has been
> replaced by the tracer. The values (time delta between -normally- short
> events) were guaranteed to be small enough to fit in 32bits, so other
> ports just cast the input data within the macro. No other I-pipe code
> should rely on better precision.
> 
> This said, in this case, my preferred option is the one of the Adeos
> maintainer for ARM, who is... oh, yeah! you are. So, I hereby agree with
> the choice you are going to make next.

Ok, let us go for what other ports do then, let us cast the argument to
a 32 bits value.

-- 
                                                 Gilles Chanteperdrix