Sigh. This is going to break virtually every ARM platform which
provides a high resolution sched_clock.
We've had this issue before when printk_clock was linked to sched_clock.
We worked around that breakage by defining our own printk_clock().
But then some bright spark thought it would be a good idea to get
rid of printk_clock().
If we want to couple the clock used by printk to sched_clock() by
default, can we please have printk_clock() back so ARM can provide
an implementation based solely upon jiffies, because basing it on
hardware sources Just Doesn't Work - the reason why...
... you can not access high resolution clocks because peripherals
are MMIO. There's no standard high resolution clocks, so there's
no standard place to map. And mappings are setup after the first
printk happens.
So, while we want to be able to use high resolution clocks for the
scheduler, the scheduler clock must _not_ be used for printk.
----- Forwarded message from Bill Gatliff <[email protected]> -----
Delivery-date: Tue, 29 Jul 2008 23:21:39 +0100
Date: Sat, 26 Jul 2008 12:18:43 -0500
From: Bill Gatliff <[email protected]>
To: linux-arm-kernel <[email protected]>
Subject: Commit 76a2a6ee8a0660a29127f05989ac59ae1ce865fa breaks PXA270 (at
least)?
Guys:
I'm tracking this git repo:
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
The commit 76a2a6ee8a0660a29127f05989ac59ae1ce865fa causes my PXA270
board to hang at boot. If I surgically remove that change, things seem
to work fine.
I'm not quite sure what this patch is doing. Anyone having similar
problems?
commit 76a2a6ee8a0660a29127f05989ac59ae1ce865fa
Author: Peter Zijlstra <[email protected]>
Date: Fri Jun 27 13:41:15 2008 +0200
sched: sched_clock_cpu() based cpu_clock()
with sched_clock_cpu() being reasonably in sync between cpus (max 1
jiffy
difference) use this to provide cpu_clock().
Signed-off-by: Peter Zijlstra <[email protected]>
Cc: Srivatsa Vaddagiri <[email protected]>
Cc: Mike Galbraith <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
diff --git a/kernel/sched.c b/kernel/sched.c
index 874b6da..eb3454c 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -818,82 +818,6 @@ static inline u64 global_rt_runtime(void)
return (u64)sysctl_sched_rt_runtime * NSEC_PER_USEC;
}
-unsigned long long time_sync_thresh = 100000;
-
-static DEFINE_PER_CPU(unsigned long long, time_offset);
-static DEFINE_PER_CPU(unsigned long long, prev_cpu_time);
-
-/*
- * Global lock which we take every now and then to synchronize
- * the CPUs time. This method is not warp-safe, but it's good
- * enough to synchronize slowly diverging time sources and thus
- * it's good enough for tracing:
- */
-static DEFINE_SPINLOCK(time_sync_lock);
-static unsigned long long prev_global_time;
-
-static unsigned long long __sync_cpu_clock(unsigned long long time, int
cpu)
-{
- /*
- * We want this inlined, to not get tracer function calls
- * in this critical section:
- */
- spin_acquire(&time_sync_lock.dep_map, 0, 0, _THIS_IP_);
- __raw_spin_lock(&time_sync_lock.raw_lock);
-
- if (time < prev_global_time) {
- per_cpu(time_offset, cpu) += prev_global_time - time;
- time = prev_global_time;
- } else {
- prev_global_time = time;
- }
-
- __raw_spin_unlock(&time_sync_lock.raw_lock);
- spin_release(&time_sync_lock.dep_map, 1, _THIS_IP_);
-
- return time;
-}
-
-static unsigned long long __cpu_clock(int cpu)
-{
- unsigned long long now;
-
- /*
- * Only call sched_clock() if the scheduler has already been
- * initialized (some code might call cpu_clock() very early):
- */
- if (unlikely(!scheduler_running))
- return 0;
-
- now = sched_clock_cpu(cpu);
-
- return now;
-}
-
-/*
- * For kernel-internal use: high-speed (but slightly incorrect) per-cpu
- * clock constructed from sched_clock():
- */
-unsigned long long cpu_clock(int cpu)
-{
- unsigned long long prev_cpu_time, time, delta_time;
- unsigned long flags;
-
- local_irq_save(flags);
- prev_cpu_time = per_cpu(prev_cpu_time, cpu);
- time = __cpu_clock(cpu) + per_cpu(time_offset, cpu);
- delta_time = time-prev_cpu_time;
-
- if (unlikely(delta_time > time_sync_thresh)) {
- time = __sync_cpu_clock(time, cpu);
- per_cpu(prev_cpu_time, cpu) = time;
- }
- local_irq_restore(flags);
-
- return time;
-}
-EXPORT_SYMBOL_GPL(cpu_clock);
-
#ifndef prepare_arch_switch
# define prepare_arch_switch(next) do { } while (0)
#endif
diff --git a/kernel/sched_clock.c b/kernel/sched_clock.c
index ce05271..3c696db 100644
--- a/kernel/sched_clock.c
+++ b/kernel/sched_clock.c
@@ -244,3 +244,15 @@ unsigned long long __attribute__((weak))
sched_clock(void)
{
return (unsigned long long)jiffies * (NSEC_PER_SEC / HZ);
}
+
+unsigned long long cpu_clock(int cpu)
+{
+ unsigned long long clock;
+ unsigned long flags;
+
+ raw_local_irq_save(flags);
+ clock = sched_clock_cpu(cpu);
+ raw_local_irq_restore(flags);
+
+ return clock;
+}
--
Bill Gatliff
[email protected]
-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ: http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette: http://www.arm.linux.org.uk/mailinglists/etiquette.php
----- End forwarded message -----
On Tue, 29 Jul 2008 23:31:05 +0100
Russell King - ARM Linux <[email protected]> wrote:
> But then some bright spark thought it would be a good idea to get
> rid of printk_clock().
<does git-log, searches for printk_clock>
commit 86faf39d0fc04272b05fab1db6d683f3ac7199d1
Author: Ingo Molnar <[email protected]>
Date: Fri Jan 25 21:07:59 2008 +0100
sched: remove printk_clock references from ia64
remove remaining printk_clock references from ia64.
Signed-off-by: Ingo Molnar <[email protected]>
commit b842271fbb9c8b5fd0e1c3e1895a3b67ba5bcc54
Author: Ingo Molnar <[email protected]>
Date: Fri Jan 25 21:07:59 2008 +0100
sched: remove printk_clock()
printk_clock() is obsolete - it has been replaced with cpu_clock().
Signed-off-by: Ingo Molnar <[email protected]>
That's it? It rates a 0.5/10 for changelogging :(
Looking further on...
commit e97126cd9056b3b42cdc862ace2ed66f8026f55b
Author: Russell King <[email protected]>
Date: Mon Jan 8 19:49:12 2007 +0000
[ARM] Provide basic printk_clock() implementation
Current sched_clock() implementations on ARM cause unbootable kernels
with PRINTK_TIME support enabled. To avoid this, provide a basic
printk_clock() implementation which avoids sched_clock() being called
before the page tables have been set up.
which I assume is why arm is crashing again?
Really, I think arch-overrideable printk_clock() was a good idea.
printk is just _special_. It's called wildly early and it is called in
all conceivable contexts and it just must work no matter what. It's
totally understandable that an architecture would need to override
printk's timestamp generator.
On Tue, Jul 29, 2008 at 03:46:05PM -0700, Andrew Morton wrote:
> On Tue, 29 Jul 2008 23:31:05 +0100
> Russell King - ARM Linux <[email protected]> wrote:
>
> > But then some bright spark thought it would be a good idea to get
> > rid of printk_clock().
>
> <does git-log, searches for printk_clock>
>
> commit 86faf39d0fc04272b05fab1db6d683f3ac7199d1
> Author: Ingo Molnar <[email protected]>
> Date: Fri Jan 25 21:07:59 2008 +0100
>
> sched: remove printk_clock references from ia64
>
> remove remaining printk_clock references from ia64.
>
> Signed-off-by: Ingo Molnar <[email protected]>
>
> commit b842271fbb9c8b5fd0e1c3e1895a3b67ba5bcc54
> Author: Ingo Molnar <[email protected]>
> Date: Fri Jan 25 21:07:59 2008 +0100
>
> sched: remove printk_clock()
>
> printk_clock() is obsolete - it has been replaced with cpu_clock().
>
> Signed-off-by: Ingo Molnar <[email protected]>
>
>
> That's it? It rates a 0.5/10 for changelogging :(
>
> Looking further on...
>
> commit e97126cd9056b3b42cdc862ace2ed66f8026f55b
> Author: Russell King <[email protected]>
> Date: Mon Jan 8 19:49:12 2007 +0000
>
> [ARM] Provide basic printk_clock() implementation
>
> Current sched_clock() implementations on ARM cause unbootable kernels
> with PRINTK_TIME support enabled. To avoid this, provide a basic
> printk_clock() implementation which avoids sched_clock() being called
> before the page tables have been set up.
>
>
> which I assume is why arm is crashing again?
Precisely - Ingo has effectively progressively backed out my bug fix
in a round-about manner by making printk require a working sched_clock
again.
> Really, I think arch-overrideable printk_clock() was a good idea.
> printk is just _special_. It's called wildly early and it is called in
> all conceivable contexts and it just must work no matter what. It's
> totally understandable that an architecture would need to override
> printk's timestamp generator.
That's one solution, the other is to have that commit reverted. I
don't really mind which, only that we have a solution ASAP so that
the current popular ARM SoCs have a bootable kernel again.
On Tue, 2008-07-29 at 23:56 +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 29, 2008 at 03:46:05PM -0700, Andrew Morton wrote:
> > On Tue, 29 Jul 2008 23:31:05 +0100
> > Russell King - ARM Linux <[email protected]> wrote:
> >
> > > But then some bright spark thought it would be a good idea to get
> > > rid of printk_clock().
> >
> > <does git-log, searches for printk_clock>
> >
> > commit 86faf39d0fc04272b05fab1db6d683f3ac7199d1
> > Author: Ingo Molnar <[email protected]>
> > Date: Fri Jan 25 21:07:59 2008 +0100
> >
> > sched: remove printk_clock references from ia64
> >
> > remove remaining printk_clock references from ia64.
> >
> > Signed-off-by: Ingo Molnar <[email protected]>
> >
> > commit b842271fbb9c8b5fd0e1c3e1895a3b67ba5bcc54
> > Author: Ingo Molnar <[email protected]>
> > Date: Fri Jan 25 21:07:59 2008 +0100
> >
> > sched: remove printk_clock()
> >
> > printk_clock() is obsolete - it has been replaced with cpu_clock().
> >
> > Signed-off-by: Ingo Molnar <[email protected]>
> >
> >
> > That's it? It rates a 0.5/10 for changelogging :(
> >
> > Looking further on...
> >
> > commit e97126cd9056b3b42cdc862ace2ed66f8026f55b
> > Author: Russell King <[email protected]>
> > Date: Mon Jan 8 19:49:12 2007 +0000
> >
> > [ARM] Provide basic printk_clock() implementation
> >
> > Current sched_clock() implementations on ARM cause unbootable kernels
> > with PRINTK_TIME support enabled. To avoid this, provide a basic
> > printk_clock() implementation which avoids sched_clock() being called
> > before the page tables have been set up.
> >
> >
> > which I assume is why arm is crashing again?
>
> Precisely - Ingo has effectively progressively backed out my bug fix
> in a round-about manner by making printk require a working sched_clock
> again.
>
> > Really, I think arch-overrideable printk_clock() was a good idea.
> > printk is just _special_. It's called wildly early and it is called in
> > all conceivable contexts and it just must work no matter what. It's
> > totally understandable that an architecture would need to override
> > printk's timestamp generator.
>
> That's one solution, the other is to have that commit reverted. I
> don't really mind which, only that we have a solution ASAP so that
> the current popular ARM SoCs have a bootable kernel again.
Can't you make sched_clock() return 0 when its called before its up and
running? Otherwise - perhaps you can play with the location of
sched_clock_init(). cpu_clock() will return 0 before that.
On Wed, 2008-07-30 at 09:10 +0200, Peter Zijlstra wrote:
> On Tue, 2008-07-29 at 23:56 +0100, Russell King - ARM Linux wrote:
> > On Tue, Jul 29, 2008 at 03:46:05PM -0700, Andrew Morton wrote:
> > > On Tue, 29 Jul 2008 23:31:05 +0100
> > > Russell King - ARM Linux <[email protected]> wrote:
> > >
> > > > But then some bright spark thought it would be a good idea to get
> > > > rid of printk_clock().
> > >
> > > <does git-log, searches for printk_clock>
> > >
> > > commit 86faf39d0fc04272b05fab1db6d683f3ac7199d1
> > > Author: Ingo Molnar <[email protected]>
> > > Date: Fri Jan 25 21:07:59 2008 +0100
> > >
> > > sched: remove printk_clock references from ia64
> > >
> > > remove remaining printk_clock references from ia64.
> > >
> > > Signed-off-by: Ingo Molnar <[email protected]>
> > >
> > > commit b842271fbb9c8b5fd0e1c3e1895a3b67ba5bcc54
> > > Author: Ingo Molnar <[email protected]>
> > > Date: Fri Jan 25 21:07:59 2008 +0100
> > >
> > > sched: remove printk_clock()
> > >
> > > printk_clock() is obsolete - it has been replaced with cpu_clock().
> > >
> > > Signed-off-by: Ingo Molnar <[email protected]>
> > >
> > >
> > > That's it? It rates a 0.5/10 for changelogging :(
> > >
> > > Looking further on...
> > >
> > > commit e97126cd9056b3b42cdc862ace2ed66f8026f55b
> > > Author: Russell King <[email protected]>
> > > Date: Mon Jan 8 19:49:12 2007 +0000
> > >
> > > [ARM] Provide basic printk_clock() implementation
> > >
> > > Current sched_clock() implementations on ARM cause unbootable kernels
> > > with PRINTK_TIME support enabled. To avoid this, provide a basic
> > > printk_clock() implementation which avoids sched_clock() being called
> > > before the page tables have been set up.
> > >
> > >
> > > which I assume is why arm is crashing again?
> >
> > Precisely - Ingo has effectively progressively backed out my bug fix
> > in a round-about manner by making printk require a working sched_clock
> > again.
> >
> > > Really, I think arch-overrideable printk_clock() was a good idea.
> > > printk is just _special_. It's called wildly early and it is called in
> > > all conceivable contexts and it just must work no matter what. It's
> > > totally understandable that an architecture would need to override
> > > printk's timestamp generator.
> >
> > That's one solution, the other is to have that commit reverted. I
> > don't really mind which, only that we have a solution ASAP so that
> > the current popular ARM SoCs have a bootable kernel again.
>
> Can't you make sched_clock() return 0 when its called before its up and
> running? Otherwise -
> perhaps you can play with the location of
> sched_clock_init(). cpu_clock() will return 0 before that.
Sorry - that's only relevant for HAVE_UNSTABLE_SCHED_CLOCK and x86 seems
to be the only one using that (which just shows the platforms quality
</sarcasm>).
On Wed, Jul 30, 2008 at 09:55:54AM +0200, Peter Zijlstra wrote:
> Sorry - that's only relevant for HAVE_UNSTABLE_SCHED_CLOCK and x86 seems
> to be the only one using that (which just shows the platforms quality
> </sarcasm>).
Can you please copy Bill on your responses. I may not be around
for most of today, whereas I'm sure Bill would be willing to test
any patches...
Russell King - ARM Linux wrote:
> On Wed, Jul 30, 2008 at 09:55:54AM +0200, Peter Zijlstra wrote:
>> Sorry - that's only relevant for HAVE_UNSTABLE_SCHED_CLOCK and x86 seems
>> to be the only one using that (which just shows the platforms quality
>> </sarcasm>).
Hmmm. That one defaults to =y, right? It has for me for years... :)
b.g.
--
Bill Gatliff
[email protected]
Guys:
I just turned off PRINTK_TIME, and verified that the platform boots.
I can live with this workaround short-term, especially if doing so provides time
to find the right fix. I wouldn't want to see this still broken in .28, of
course... ;)
b.g.
--
Bill Gatliff
[email protected]
* Andrew Morton <[email protected]> wrote:
> On Tue, 29 Jul 2008 23:31:05 +0100
> Russell King - ARM Linux <[email protected]> wrote:
>
> > But then some bright spark thought it would be a good idea to get
> > rid of printk_clock().
>
> <does git-log, searches for printk_clock>
i think this is a fresh regression via the introduction of
kernel/sched_clock.c. We lost the (known) early-init behavior of
cpu_clock() in the !UNSTABLE_SCHED_CLOCK case. The fix would be to
restore that, not to reintroduce printk_clock().
Peter, any ideas?
Ingo
On Thu, 2008-07-31 at 23:37 +0200, Ingo Molnar wrote:
> * Andrew Morton <[email protected]> wrote:
>
> > On Tue, 29 Jul 2008 23:31:05 +0100
> > Russell King - ARM Linux <[email protected]> wrote:
> >
> > > But then some bright spark thought it would be a good idea to get
> > > rid of printk_clock().
> >
> > <does git-log, searches for printk_clock>
>
> i think this is a fresh regression via the introduction of
> kernel/sched_clock.c. We lost the (known) early-init behavior of
> cpu_clock() in the !UNSTABLE_SCHED_CLOCK case. The fix would be to
> restore that, not to reintroduce printk_clock().
>
> Peter, any ideas?
How about something like this, it builds an atificial delay, exactly
like we already have for the HAVE_UNSTABLE_SCHED_CLOCK case.
This keeps cpu_clock() 0 until after sched_clock_init().
Russell, Bill, is this sufficient?
Signed-off-by: Peter Zijlstra <[email protected]>
---
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 1b26ed2..d13264b 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1550,16 +1550,10 @@ static inline int set_cpus_allowed(struct task_struct *p, cpumask_t new_mask)
extern unsigned long long sched_clock(void);
-#ifndef CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
-static inline void sched_clock_init(void)
-{
-}
-
-static inline u64 sched_clock_cpu(int cpu)
-{
- return sched_clock();
-}
+extern void sched_clock_init(void);
+extern u64 sched_clock_cpu(int cpu);
+#ifndef CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
static inline void sched_clock_tick(void)
{
}
@@ -1583,8 +1577,6 @@ static inline void sched_clock_tick_start(int cpu)
#endif
#else /* CONFIG_HAVE_UNSTABLE_SCHED_CLOCK */
-extern void sched_clock_init(void);
-extern u64 sched_clock_cpu(int cpu);
extern void sched_clock_tick(void);
extern void sched_clock_idle_sleep_event(void);
extern void sched_clock_idle_wakeup_event(u64 delta_ns);
diff --git a/kernel/sched_clock.c b/kernel/sched_clock.c
index 22ed55d..a804582 100644
--- a/kernel/sched_clock.c
+++ b/kernel/sched_clock.c
@@ -32,6 +32,7 @@
#include <linux/ktime.h>
#include <linux/module.h>
+static __read_mostly int sched_clock_running;
#ifdef CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
@@ -71,8 +72,6 @@ static inline struct sched_clock_data *cpu_sdc(int cpu)
return &per_cpu(sched_clock_data, cpu);
}
-static __read_mostly int sched_clock_running;
-
void sched_clock_init(void)
{
u64 ktime_now = ktime_to_ns(ktime_get());
@@ -319,6 +318,21 @@ void sched_clock_idle_wakeup_event(u64 delta_ns)
}
EXPORT_SYMBOL_GPL(sched_clock_idle_wakeup_event);
+#else /* CONFIG_HAVE_UNSTABLE_SCHED_CLOCK */
+
+void sched_clock_init(void)
+{
+ sched_clock_running = 1;
+}
+
+u64 sched_clock_cpu(int cpu)
+{
+ if (unlikely(!sched_clock_running))
+ return 0;
+
+ return sched_clock();
+}
+
#endif
/*
On Thu, Jul 31, 2008 at 11:47:55PM +0200, Peter Zijlstra wrote:
> On Thu, 2008-07-31 at 23:37 +0200, Ingo Molnar wrote:
> > * Andrew Morton <[email protected]> wrote:
> >
> > > On Tue, 29 Jul 2008 23:31:05 +0100
> > > Russell King - ARM Linux <[email protected]> wrote:
> > >
> > > > But then some bright spark thought it would be a good idea to get
> > > > rid of printk_clock().
> > >
> > > <does git-log, searches for printk_clock>
> >
> > i think this is a fresh regression via the introduction of
> > kernel/sched_clock.c. We lost the (known) early-init behavior of
> > cpu_clock() in the !UNSTABLE_SCHED_CLOCK case. The fix would be to
> > restore that, not to reintroduce printk_clock().
> >
> > Peter, any ideas?
>
> How about something like this, it builds an atificial delay, exactly
> like we already have for the HAVE_UNSTABLE_SCHED_CLOCK case.
>
> This keeps cpu_clock() 0 until after sched_clock_init().
>
> Russell, Bill, is this sufficient?
It looks like it should. Bill - can you test the patch in Peter's mail
please?
Thanks.
Russell King - ARM Linux wrote:
> On Thu, Jul 31, 2008 at 11:47:55PM +0200, Peter Zijlstra wrote:
>> On Thu, 2008-07-31 at 23:37 +0200, Ingo Molnar wrote:
>>> * Andrew Morton <[email protected]> wrote:
>>>
>>>> On Tue, 29 Jul 2008 23:31:05 +0100
>>>> Russell King - ARM Linux <[email protected]> wrote:
>>>>
>>>>> But then some bright spark thought it would be a good idea to get
>>>>> rid of printk_clock().
>>>> <does git-log, searches for printk_clock>
>>> i think this is a fresh regression via the introduction of
>>> kernel/sched_clock.c. We lost the (known) early-init behavior of
>>> cpu_clock() in the !UNSTABLE_SCHED_CLOCK case. The fix would be to
>>> restore that, not to reintroduce printk_clock().
>>>
>>> Peter, any ideas?
>> How about something like this, it builds an atificial delay, exactly
>> like we already have for the HAVE_UNSTABLE_SCHED_CLOCK case.
>>
>> This keeps cpu_clock() 0 until after sched_clock_init().
>>
>> Russell, Bill, is this sufficient?
>
> It looks like it should. Bill - can you test the patch in Peter's mail
> please?
I've got one foot out the door headed towards a business trip. I can check it
out Monday or Tuesday.
b.g.
--
Bill Gatliff
[email protected]