| Message ID | 20130717061925.GC6950@tarshish (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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
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.
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
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@arm.linux.org.uk as the email address please.
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
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.
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
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. */