sched_clock always 0 and no process time accounting with 3.11-rc1

sched_clock always 0 and no process time accounting with 3.11-rc1

[Baruch Siach]

Hi Max,

On Mon, Jul 15, 2013 at 06:18:33PM +0400, Max Filippov wrote:
> with 3.11-rc1 printk timestamps are always zero (which means that
> sched_clock is always 0) and process time accounting always shows
> 0 for system/user time. I see it regardless of HZ_PERIODIC/NO_HZ_IDLE
> and HIGH_RES_TIMERS settings. Am I missing any necessary config
> options? Can you see the same issue?

The following patch fixes the issue for me:

diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index a326f27..0b479a6 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -121,7 +121,7 @@ void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
 	BUG_ON(bits > 32);
 	WARN_ON(!irqs_disabled());
 	read_sched_clock = read;
-	sched_clock_mask = (1 << bits) - 1;
+	sched_clock_mask = (1ULL << bits) - 1;
 	cd.rate = rate;
 
 	/* calculate the mult/shift to convert counter ticks to ns. */

Apparently the expression '(1 << 32)' evaluates to 1 on xtensa cross gcc, and 
x86_84 native gcc. According to my limited understanding, the C compiler is 
allowed to do so. This caused sched_clock_32() to return constant 0. I wonder 
how it didn't bite the ARM people who are using this code from quite some time 
(added LAKL to CC).

If this patch proves correct I'll send a formal patch to the timekeeping 
maintainers.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

sched_clock always 0 and no process time accounting with 3.11-rc1

[Max Filippov]

On Wed, Jul 17, 2013 at 10:19 AM, Baruch Siach <baruch@tkos.co.il> wrote:
> Hi Max,
>
> On Mon, Jul 15, 2013 at 06:18:33PM +0400, Max Filippov wrote:
>> with 3.11-rc1 printk timestamps are always zero (which means that
>> sched_clock is always 0) and process time accounting always shows
>> 0 for system/user time. I see it regardless of HZ_PERIODIC/NO_HZ_IDLE
>> and HIGH_RES_TIMERS settings. Am I missing any necessary config
>> options? Can you see the same issue?
>
> The following patch fixes the issue for me:
>
> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> index a326f27..0b479a6 100644
> --- a/kernel/time/sched_clock.c
> +++ b/kernel/time/sched_clock.c
> @@ -121,7 +121,7 @@ void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
>         BUG_ON(bits > 32);
>         WARN_ON(!irqs_disabled());
>         read_sched_clock = read;
> -       sched_clock_mask = (1 << bits) - 1;
> +       sched_clock_mask = (1ULL << bits) - 1;
>         cd.rate = rate;
>
>         /* calculate the mult/shift to convert counter ticks to ns. */
>
> Apparently the expression '(1 << 32)' evaluates to 1 on xtensa cross gcc, and
> x86_84 native gcc. According to my limited understanding, the C compiler is
> allowed to do so. This caused sched_clock_32() to return constant 0. I wonder
> how it didn't bite the ARM people who are using this code from quite some time
> (added LAKL to CC).

Cool, it works now, thanks Baruch.
According to C99 the behaviour of (1 << 32) is undefined on platforms with
32 bit int, so it could yield any value.

> If this patch proves correct I'll send a formal patch to the timekeeping
> maintainers.

-- 
Thanks.
-- Max

[Linux-Xtensa] Re: sched_clock always 0 and no process time accounting with 3.11-rc1

[Baruch Siach]

Hi Max,

On Wed, Jul 17, 2013 at 10:39:51AM +0400, Max Filippov wrote:
> On Wed, Jul 17, 2013 at 10:19 AM, Baruch Siach <baruch@tkos.co.il> wrote:
> > On Mon, Jul 15, 2013 at 06:18:33PM +0400, Max Filippov wrote:
> >> with 3.11-rc1 printk timestamps are always zero (which means that
> >> sched_clock is always 0) and process time accounting always shows
> >> 0 for system/user time. I see it regardless of HZ_PERIODIC/NO_HZ_IDLE
> >> and HIGH_RES_TIMERS settings. Am I missing any necessary config
> >> options? Can you see the same issue?
> >
> > The following patch fixes the issue for me:
> >
> > diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> > index a326f27..0b479a6 100644
> > --- a/kernel/time/sched_clock.c
> > +++ b/kernel/time/sched_clock.c
> > @@ -121,7 +121,7 @@ void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
> >         BUG_ON(bits > 32);
> >         WARN_ON(!irqs_disabled());
> >         read_sched_clock = read;
> > -       sched_clock_mask = (1 << bits) - 1;
> > +       sched_clock_mask = (1ULL << bits) - 1;
> >         cd.rate = rate;
> >
> >         /* calculate the mult/shift to convert counter ticks to ns. */
> >
> > Apparently the expression '(1 << 32)' evaluates to 1 on xtensa cross gcc, and
> > x86_64 native gcc. According to my limited understanding, the C compiler is
> > allowed to do so. This caused sched_clock_32() to return constant 0. I wonder
> > how it didn't bite the ARM people who are using this code from quite some time
> > (added LAKL to CC).
> 
> Cool, it works now, thanks Baruch.

Thanks for testing.

> According to C99 the behaviour of (1 << 32) is undefined on platforms with
> 32 bit int, so it could yield any value.

But so is ARM, isn't it?

> > If this patch proves correct I'll send a formal patch to the timekeeping
> > maintainers.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[Linux-Xtensa] Re: sched_clock always 0 and no process time accounting with 3.11-rc1

[Chris Zankel]

On 07/16/2013 11:47 PM, Baruch Siach wrote:
>> According to C99 the behaviour of (1 << 32) is undefined on platforms with
>> 32 bit int, so it could yield any value.
> But so is ARM, isn't it?

The handling of constants might be target-architecture dependent.

On a somewhat related issue, are you using a 32-bit host or 64-bit host?
Note that 64-bit cross compilation is known to be broken for Xtensa, I
had a problem running glibc when compiled with cross compiler on a
64-bit host.

Cheers!
-Chris

[Linux-Xtensa] Re: sched_clock always 0 and no process time accounting with 3.11-rc1

[Baruch Siach]

Hi Chris,

On Wed, Jul 17, 2013 at 12:02:10AM -0700, Chris Zankel wrote:
> On 07/16/2013 11:47 PM, Baruch Siach wrote:
> >> According to C99 the behaviour of (1 << 32) is undefined on platforms with
> >> 32 bit int, so it could yield any value.
> > But so is ARM, isn't it?
> 
> The handling of constants might be target-architecture dependent.
> 
> On a somewhat related issue, are you using a 32-bit host or 64-bit host?
> Note that 64-bit cross compilation is known to be broken for Xtensa, I
> had a problem running glibc when compiled with cross compiler on a
> 64-bit host.

Yes, my host is x86_64. I haven't encountered any problem yet. But thanks for 
the warning, I'll keep that in mind. I think it's worth a mention in the 
linux-xtensa wiki. It surly worth a fix as well, I guess. Are there any 
samples of code that are known to be break with 64 bit host?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

sched_clock always 0 and no process time accounting with 3.11-rc1

[Russell King - ARM Linux]

On Wed, Jul 17, 2013 at 09:19:25AM +0300, Baruch Siach wrote:
> Apparently the expression '(1 << 32)' evaluates to 1 on xtensa cross gcc, and 
> x86_84 native gcc. According to my limited understanding, the C compiler is 
> allowed to do so. This caused sched_clock_32() to return constant 0. I wonder 
> how it didn't bite the ARM people who are using this code from quite some time 
> (added LAKL to CC).

It (a) used to be only 32-bit, and (b) <<32 on ARM evaluates to zero in
a 32-bit context (it's not a rotate).

Patch looks fine.  I wonder who takes it... after all, I used to look
after that code (after all, I'm the author of it), but now I guess as
it's been moved under kernel/time, it's someone elses responsibility.

sched_clock always 0 and no process time accounting with 3.11-rc1

[Baruch Siach]

Hi Russell,

On Wed, Jul 17, 2013 at 10:03:55AM +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 17, 2013 at 09:19:25AM +0300, Baruch Siach wrote:
> > Apparently the expression '(1 << 32)' evaluates to 1 on xtensa cross gcc, and 
> > x86_84 native gcc. According to my limited understanding, the C compiler is 
> > allowed to do so. This caused sched_clock_32() to return constant 0. I wonder 
> > how it didn't bite the ARM people who are using this code from quite some time 
> > (added LAKL to CC).
> 
> It (a) used to be only 32-bit, and (b) <<32 on ARM evaluates to zero in
> a 32-bit context (it's not a rotate).
> 
> Patch looks fine.

Thanks for the confirmation, I'll take it as an ack if you don't mind.

> I wonder who takes it... after all, I used to look
> after that code (after all, I'm the author of it), but now I guess as
> it's been moved under kernel/time, it's someone elses responsibility.

Since the code move went through the tree of the timekeepers, John Stultz and 
Thomas Gleixner, I guess they should handle it.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

sched_clock always 0 and no process time accounting with 3.11-rc1

[Russell King - ARM Linux]

On Wed, Jul 17, 2013 at 12:12:09PM +0300, Baruch Siach wrote:
> Hi Russell,
> 
> On Wed, Jul 17, 2013 at 10:03:55AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Jul 17, 2013 at 09:19:25AM +0300, Baruch Siach wrote:
> > > Apparently the expression '(1 << 32)' evaluates to 1 on xtensa cross gcc, and 
> > > x86_84 native gcc. According to my limited understanding, the C compiler is 
> > > allowed to do so. This caused sched_clock_32() to return constant 0. I wonder 
> > > how it didn't bite the ARM people who are using this code from quite some time 
> > > (added LAKL to CC).
> > 
> > It (a) used to be only 32-bit, and (b) <<32 on ARM evaluates to zero in
> > a 32-bit context (it's not a rotate).
> > 
> > Patch looks fine.
> 
> Thanks for the confirmation, I'll take it as an ack if you don't mind.

If you want it as an ack, use rmk+kernel at arm.linux.org.uk as the email
address please.

[Linux-Xtensa] Re: sched_clock always 0 and no process time accounting with 3.11-rc1

[Marc Gauthier]

Hi Baruch,

On 2013-07-16, at 11:19 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> -    sched_clock_mask = (1 << bits) - 1;
> +    sched_clock_mask = (1ULL << bits) - 1;

Might it be more efficient to use:

   sched_clock_mask = 0xffffffff >> (32 - bits);

to avoid using 64 bit ints?
(assuming bits > 0, didn't check)

-Marc

[Linux-Xtensa] Re: sched_clock always 0 and no process time accounting with 3.11-rc1

[Russell King - ARM Linux]

On Wed, Jul 17, 2013 at 08:15:08AM -0700, Marc Gauthier wrote:
> Hi Baruch,
> 
> On 2013-07-16, at 11:19 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> > -    sched_clock_mask = (1 << bits) - 1;
> > +    sched_clock_mask = (1ULL << bits) - 1;
> 
> Might it be more efficient to use:
> 
>    sched_clock_mask = 0xffffffff >> (32 - bits);
> 
> to avoid using 64 bit ints?
> (assuming bits > 0, didn't check)

bits being 0 would be rather silly (it implies a mask of zero, and therefore
no bits available for the clock.)  We could modify the BUG_ON() above to
make that point, or add it as commentry against the function.

Given that, and that sched_clock_mask is a u32, then I agree with you - the
above would be a better way to do this.  Personally, I'd suggest ~0u rather
than 0xffffffff because its less typing.

[Linux-Xtensa] Re: sched_clock always 0 and no process time accounting with 3.11-rc1

[Marc Gauthier]

Russell King - ARM Linux wrote:
> On Wed, Jul 17, 2013 at 08:15:08AM -0700, Marc Gauthier wrote:
> > Hi Baruch,
> >
> > On 2013-07-16, at 11:19 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> > > -    sched_clock_mask = (1 << bits) - 1;
> > > +    sched_clock_mask = (1ULL << bits) - 1;
> >
> > Might it be more efficient to use:
> >
> >    sched_clock_mask = 0xffffffff >> (32 - bits);
> >
> > to avoid using 64 bit ints?
> > (assuming bits > 0, didn't check)
>
> bits being 0 would be rather silly (it implies a mask of
> zero, and therefore
> no bits available for the clock.)  We could modify the
> BUG_ON() above to
> make that point, or add it as commentry against the function.

Sounds good.


> Given that, and that sched_clock_mask is a u32, then I agree
> with you - the above would be a better way to do this.

> Personally, I'd suggest ~0u rather
> than 0xffffffff because its less typing.

Am not sure though, on machines with 64-bit int, ~0u might
end up being 0xffffffffffffffff which is unintended.

-Marc