[PATCH] U300 sched_clock implementation

[PATCH] U300 sched_clock implementation

[Linus Walleij]

This adds the long debated sched_clock() override for the weak
in-kernel jiffybased sched_clock(). The implementation is more or
less identical to the one used in arch/arm/plat-omap/common.c
and at last attempt to merge this the merge was postponed at the
request of Peter Zijlstra due to pending discussions regarding
generalized clocksource-based sched_clock() implementations by
adding a flag to the clocksource. However that discussion ended
up with the generic code needing to be rewritten and Paul Mundt
see no reason not to proceed with this for the time being as it
can be easily converted once the generic code is in place.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Paul Mundt <lethal@linux-sh.org>
---
 arch/arm/mach-u300/timer.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-u300/timer.c b/arch/arm/mach-u300/timer.c
index cce5320..63b5186 100644
--- a/arch/arm/mach-u300/timer.c
+++ b/arch/arm/mach-u300/timer.c
@@ -346,6 +346,22 @@ static struct clocksource clocksource_u300_1mhz = {
 	.flags          = CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
+/*
+ * Override the global weak sched_clock symbol with this
+ * local implementation which uses the clocksource to get some
+ * better resolution when scheduling the kernel. We accept that
+ * this wraps around for now, since it is just a relative time
+ * stamp. (Inspired by OMAP implementation.)
+ */
+unsigned long long notrace sched_clock(void)
+{
+	unsigned long long ret;
+
+	ret = (unsigned long long) u300_get_cycles(&clocksource_u300_1mhz);
+	ret = (ret * clocksource_u300_1mhz.mult_orig) >>
+		clocksource_u300_1mhz.shift;
+	return ret;
+}
 
 /*
  * This sets up the system timers, clock source and clock event.
-- 
1.6.2.1

Re: [PATCH] U300 sched_clock implementation

[Paul Mundt]

On Tue, Jul 07, 2009 at 10:01:58AM +0200, Linus Walleij wrote:
> Adding in Paul M since it was his patch that was supposed to fix up a
> generic solution...
> 
> 2009/7/7 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Tue, Jun 02, 2009 at 11:00:12AM +0200, Peter Zijlstra wrote:
> >> I think its best if we continue with the patch Paul Mundt has been
> >> proposing.
> >
> > [added Tim Bird to CC]
> >
> > So what do we do? ?There's apparantly been zero movement on this for
> > over a month, and Tim Bird is reposting his patch adding __notrace
> > to ARMs existing sched_clock implementations.
> >
> > Given that it seems the generic approach has died a death, I suggest we
> > merge Linus' U300 patch, and get Tim to redo his patch to take account
> > of that, and apply both.
> >
> > Then, if the generic approach eventually happens, everything can then be
> > fixed up.
> >
> > Alternatively, if there is movement on the generic approach...
> >
> > Discuss.
> >
> 
> I would really like to see Pauls work finalized, it looked very promising,
> and I think there was actually a rough consensus about his last patch.
> But I guess that will be in the 2.6.32 merge window earliest?
> 
There was a consensus up until the point John noted that sched_clock()
can't wrap, so the generic approach is going to need a bit more work to
take the cycle shift and so on in to account. I've gotten momentarily
sidetracked with other things, but I'll post an updated version shortly.

Having said that, I don't see any reason why this should block progress
on the ARM side, once folks are happy with the generic version then most
of the implementations can be killed off, so a few more isn't going to
make much of a difference one way or the other.

The only reason I haven't done an SH-specific sched_clock() is because
none of our clocksources are architecture specific, and they all reside
in drivers/.

Re: [PATCH] U300 sched_clock implementation

[Linus Walleij]

Adding in Paul M since it was his patch that was supposed to fix up a
generic solution...

2009/7/7 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Tue, Jun 02, 2009 at 11:00:12AM +0200, Peter Zijlstra wrote:
>> On Mon, 2009-06-01 at 09:46 +0200, Linus Walleij wrote:
>> > 2009/5/25 Peter Zijlstra <peterz@infradead.org>:
>> > > On Mon, 2009-05-25 at 14:13 +0200, Linus Walleij wrote:
>> > >> 2009/5/24 Peter Zijlstra <peterz@infradead.org>:
>> > >>
>> > >> > On Sat, 2009-05-23 at 23:46 +0200, Linus Walleij wrote:
>> > >> >
>> > >> >> This overrides the global sched_clock() symbol in the Linux
>> > >> >> scheduler with a local implementation which takes advantage of
>> > >> >> the timesource in U300 giving a scheduling resolution of 1us. The
>> > >> >> solution is the same as found in the OMAP2 core code.
>> > >> >
>> > >> > We assume sched_clock() to return time in ns (e-9) resolution.
>> > >>
>> > >> Yep okay and in this case:
>> > >>
>> > >> >> +       ret = (unsigned long long) u300_get_cycles();
>> > >> >> +       ret = (ret * clocksource_u300_1mhz.mult_orig) >>
>> > >> >> +               clocksource_u300_1mhz.shift;
>> > >> >> +       return ret;
>> > >>
>> > >> (mult_orig >> shift) == 1000
>> > >
>> > > Ah, ok -- missed that little detail ;-)
>> > >
>> > >> So for each cycle in cyclecount register we return 1000 * cycles
>> > >> i.e 1000ns.
>> > >>
>> > >> If it looks nicer we can of course simply:
>> > >> return (unsigned long long) u300_get_cycles * 1000;
>> > >>
>> > >> But the question here is whether this resolution is enough for
>> > >> sched_clock() or if it is irrelevant to override sched_clock()
>> > >> if it cannot schedule with better precision than 1000 ns.
>> > >
>> > > No anything better than jiffies is good, 1us certainly is worth the
>> > > trouble.
>> >
>> > Can I interpret this as Acked-by: Peter Zijlstra <peterz@infradead.org> ?
>>
>> I think its best if we continue with the patch Paul Mundt has been
>> proposing.
>
> [added Tim Bird to CC]
>
> So what do we do?  There's apparantly been zero movement on this for
> over a month, and Tim Bird is reposting his patch adding __notrace
> to ARMs existing sched_clock implementations.
>
> Given that it seems the generic approach has died a death, I suggest we
> merge Linus' U300 patch, and get Tim to redo his patch to take account
> of that, and apply both.
>
> Then, if the generic approach eventually happens, everything can then be
> fixed up.
>
> Alternatively, if there is movement on the generic approach...
>
> Discuss.
>

I would really like to see Pauls work finalized, it looked very promising,
and I think there was actually a rough consensus about his last patch.
But I guess that will be in the 2.6.32 merge window earliest?

Yours,
Linus Walleij

Re: [PATCH] U300 sched_clock implementation

[Russell King - ARM Linux]

On Tue, Jun 02, 2009 at 11:00:12AM +0200, Peter Zijlstra wrote:
> On Mon, 2009-06-01 at 09:46 +0200, Linus Walleij wrote:
> > 2009/5/25 Peter Zijlstra <peterz@infradead.org>:
> > > On Mon, 2009-05-25 at 14:13 +0200, Linus Walleij wrote:
> > >> 2009/5/24 Peter Zijlstra <peterz@infradead.org>:
> > >>
> > >> > On Sat, 2009-05-23 at 23:46 +0200, Linus Walleij wrote:
> > >> >
> > >> >> This overrides the global sched_clock() symbol in the Linux
> > >> >> scheduler with a local implementation which takes advantage of
> > >> >> the timesource in U300 giving a scheduling resolution of 1us. The
> > >> >> solution is the same as found in the OMAP2 core code.
> > >> >
> > >> > We assume sched_clock() to return time in ns (e-9) resolution.
> > >>
> > >> Yep okay and in this case:
> > >>
> > >> >> +       ret = (unsigned long long) u300_get_cycles();
> > >> >> +       ret = (ret * clocksource_u300_1mhz.mult_orig) >>
> > >> >> +               clocksource_u300_1mhz.shift;
> > >> >> +       return ret;
> > >>
> > >> (mult_orig >> shift) == 1000
> > >
> > > Ah, ok -- missed that little detail ;-)
> > >
> > >> So for each cycle in cyclecount register we return 1000 * cycles
> > >> i.e 1000ns.
> > >>
> > >> If it looks nicer we can of course simply:
> > >> return (unsigned long long) u300_get_cycles * 1000;
> > >>
> > >> But the question here is whether this resolution is enough for
> > >> sched_clock() or if it is irrelevant to override sched_clock()
> > >> if it cannot schedule with better precision than 1000 ns.
> > >
> > > No anything better than jiffies is good, 1us certainly is worth the
> > > trouble.
> > 
> > Can I interpret this as Acked-by: Peter Zijlstra <peterz@infradead.org> ?
> 
> I think its best if we continue with the patch Paul Mundt has been
> proposing.

[added Tim Bird to CC]

So what do we do?  There's apparantly been zero movement on this for
over a month, and Tim Bird is reposting his patch adding __notrace
to ARMs existing sched_clock implementations.

Given that it seems the generic approach has died a death, I suggest we
merge Linus' U300 patch, and get Tim to redo his patch to take account
of that, and apply both.

Then, if the generic approach eventually happens, everything can then be
fixed up.

Alternatively, if there is movement on the generic approach...

Discuss.

Re: [PATCH] U300 sched_clock implementation

[Peter Zijlstra]

On Mon, 2009-06-01 at 09:46 +0200, Linus Walleij wrote:
> 2009/5/25 Peter Zijlstra <peterz@infradead.org>:
> > On Mon, 2009-05-25 at 14:13 +0200, Linus Walleij wrote:
> >> 2009/5/24 Peter Zijlstra <peterz@infradead.org>:
> >>
> >> > On Sat, 2009-05-23 at 23:46 +0200, Linus Walleij wrote:
> >> >
> >> >> This overrides the global sched_clock() symbol in the Linux
> >> >> scheduler with a local implementation which takes advantage of
> >> >> the timesource in U300 giving a scheduling resolution of 1us. The
> >> >> solution is the same as found in the OMAP2 core code.
> >> >
> >> > We assume sched_clock() to return time in ns (e-9) resolution.
> >>
> >> Yep okay and in this case:
> >>
> >> >> +       ret = (unsigned long long) u300_get_cycles();
> >> >> +       ret = (ret * clocksource_u300_1mhz.mult_orig) >>
> >> >> +               clocksource_u300_1mhz.shift;
> >> >> +       return ret;
> >>
> >> (mult_orig >> shift) == 1000
> >
> > Ah, ok -- missed that little detail ;-)
> >
> >> So for each cycle in cyclecount register we return 1000 * cycles
> >> i.e 1000ns.
> >>
> >> If it looks nicer we can of course simply:
> >> return (unsigned long long) u300_get_cycles * 1000;
> >>
> >> But the question here is whether this resolution is enough for
> >> sched_clock() or if it is irrelevant to override sched_clock()
> >> if it cannot schedule with better precision than 1000 ns.
> >
> > No anything better than jiffies is good, 1us certainly is worth the
> > trouble.
> 
> Can I interpret this as Acked-by: Peter Zijlstra <peterz@infradead.org> ?

I think its best if we continue with the patch Paul Mundt has been
proposing.

Re: [PATCH] U300 sched_clock implementation

[Linus Walleij]

2009/5/25 Peter Zijlstra <peterz@infradead.org>:
> On Mon, 2009-05-25 at 14:13 +0200, Linus Walleij wrote:
>> 2009/5/24 Peter Zijlstra <peterz@infradead.org>:
>>
>> > On Sat, 2009-05-23 at 23:46 +0200, Linus Walleij wrote:
>> >
>> >> This overrides the global sched_clock() symbol in the Linux
>> >> scheduler with a local implementation which takes advantage of
>> >> the timesource in U300 giving a scheduling resolution of 1us. The
>> >> solution is the same as found in the OMAP2 core code.
>> >
>> > We assume sched_clock() to return time in ns (e-9) resolution.
>>
>> Yep okay and in this case:
>>
>> >> +       ret = (unsigned long long) u300_get_cycles();
>> >> +       ret = (ret * clocksource_u300_1mhz.mult_orig) >>
>> >> +               clocksource_u300_1mhz.shift;
>> >> +       return ret;
>>
>> (mult_orig >> shift) == 1000
>
> Ah, ok -- missed that little detail ;-)
>
>> So for each cycle in cyclecount register we return 1000 * cycles
>> i.e 1000ns.
>>
>> If it looks nicer we can of course simply:
>> return (unsigned long long) u300_get_cycles * 1000;
>>
>> But the question here is whether this resolution is enough for
>> sched_clock() or if it is irrelevant to override sched_clock()
>> if it cannot schedule with better precision than 1000 ns.
>
> No anything better than jiffies is good, 1us certainly is worth the
> trouble.

Can I interpret this as Acked-by: Peter Zijlstra <peterz@infradead.org> ?

Russell wanted an indication from the scheduler people that it looked sane...

Yours,
Linus Walleij

Re: [PATCH] U300 sched_clock implementation

[Peter Zijlstra]

On Mon, 2009-05-25 at 15:01 +0200, Peter Zijlstra wrote:
> On Mon, 2009-05-25 at 14:13 +0200, Linus Walleij wrote:
> > 2009/5/24 Peter Zijlstra <peterz@infradead.org>:
> > 
> > > On Sat, 2009-05-23 at 23:46 +0200, Linus Walleij wrote:
> > >
> > >> This overrides the global sched_clock() symbol in the Linux
> > >> scheduler with a local implementation which takes advantage of
> > >> the timesource in U300 giving a scheduling resolution of 1us. The
> > >> solution is the same as found in the OMAP2 core code.
> > >
> > > We assume sched_clock() to return time in ns (e-9) resolution.
> > 
> > Yep okay and in this case:
> > 
> > >> +       ret = (unsigned long long) u300_get_cycles();
> > >> +       ret = (ret * clocksource_u300_1mhz.mult_orig) >>
> > >> +               clocksource_u300_1mhz.shift;
> > >> +       return ret;
> > 
> > (mult_orig >> shift) == 1000
> 
> Ah, ok -- missed that little detail ;-)
> 
> > So for each cycle in cyclecount register we return 1000 * cycles
> > i.e 1000ns.
> > 
> > If it looks nicer we can of course simply:
> > return (unsigned long long) u300_get_cycles * 1000;
> > 
> > But the question here is whether this resolution is enough for
> > sched_clock() or if it is irrelevant to override sched_clock()
> > if it cannot schedule with better precision than 1000 ns.
> 
> No anything better than jiffies is good, 1us certainly is worth the
> trouble.

One note, sched_clock() is assumed to be _cheap_. Now I assume you knew
that and chose a suitable clocksource.

But that is the reason this isn't generic, non of the 'stable'
clocksources on x86 are fast enough to use as sched_clock.

Of course, x86 isn't the only arch and if enough architectures do show
this pattern, we could indeed think about doing this in generic code.

Re: [PATCH] U300 sched_clock implementation

[Peter Zijlstra]

On Mon, 2009-05-25 at 14:13 +0200, Linus Walleij wrote:
> 2009/5/24 Peter Zijlstra <peterz@infradead.org>:
> 
> > On Sat, 2009-05-23 at 23:46 +0200, Linus Walleij wrote:
> >
> >> This overrides the global sched_clock() symbol in the Linux
> >> scheduler with a local implementation which takes advantage of
> >> the timesource in U300 giving a scheduling resolution of 1us. The
> >> solution is the same as found in the OMAP2 core code.
> >
> > We assume sched_clock() to return time in ns (e-9) resolution.
> 
> Yep okay and in this case:
> 
> >> +       ret = (unsigned long long) u300_get_cycles();
> >> +       ret = (ret * clocksource_u300_1mhz.mult_orig) >>
> >> +               clocksource_u300_1mhz.shift;
> >> +       return ret;
> 
> (mult_orig >> shift) == 1000

Ah, ok -- missed that little detail ;-)

> So for each cycle in cyclecount register we return 1000 * cycles
> i.e 1000ns.
> 
> If it looks nicer we can of course simply:
> return (unsigned long long) u300_get_cycles * 1000;
> 
> But the question here is whether this resolution is enough for
> sched_clock() or if it is irrelevant to override sched_clock()
> if it cannot schedule with better precision than 1000 ns.

No anything better than jiffies is good, 1us certainly is worth the
trouble.

Re: [PATCH] U300 sched_clock implementation

[Linus Walleij]

2009/5/24 Peter Zijlstra <peterz@infradead.org>:

> On Sat, 2009-05-23 at 23:46 +0200, Linus Walleij wrote:
>
>> This overrides the global sched_clock() symbol in the Linux
>> scheduler with a local implementation which takes advantage of
>> the timesource in U300 giving a scheduling resolution of 1us. The
>> solution is the same as found in the OMAP2 core code.
>
> We assume sched_clock() to return time in ns (e-9) resolution.

Yep okay and in this case:

>> +       ret = (unsigned long long) u300_get_cycles();
>> +       ret = (ret * clocksource_u300_1mhz.mult_orig) >>
>> +               clocksource_u300_1mhz.shift;
>> +       return ret;

(mult_orig >> shift) == 1000

So for each cycle in cyclecount register we return 1000 * cycles
i.e 1000ns.

If it looks nicer we can of course simply:
return (unsigned long long) u300_get_cycles * 1000;

But the question here is whether this resolution is enough for
sched_clock() or if it is irrelevant to override sched_clock()
if it cannot schedule with better precision than 1000 ns.

Yours,
Linus Walleij

Re: [PATCH] U300 sched_clock implementation

[Peter Zijlstra]

On Sat, 2009-05-23 at 23:46 +0200, Linus Walleij wrote:

> This overrides the global sched_clock() symbol in the Linux
> scheduler with a local implementation which takes advantage of
> the timesource in U300 giving a scheduling resolution of 1us. The
> solution is the same as found in the OMAP2 core code.

We assume sched_clock() to return time in ns (e-9) resolution.

> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
> ---
>  arch/arm/mach-u300/timer.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-u300/timer.c b/arch/arm/mach-u300/timer.c
> index 4bf82f8..222de18 100644
> --- a/arch/arm/mach-u300/timer.c
> +++ b/arch/arm/mach-u300/timer.c
> @@ -346,6 +346,22 @@ static struct clocksource clocksource_u300_1mhz = {
>        .flags          = CLOCK_SOURCE_IS_CONTINUOUS,
>  };
> 
> +/*
> + * Override the global weak sched_clock symbol with this
> + * local implementation which uses the clocksource to get some
> + * better resolution when scheduling the kernel. We accept that
> + * this wraps around for now, since it is just a relative time
> + * stamp. (Inspired by OMAP implementation.)
> + */
> +unsigned long long sched_clock(void)
> +{
> +       unsigned long long ret;
> +
> +       ret = (unsigned long long) u300_get_cycles();
> +       ret = (ret * clocksource_u300_1mhz.mult_orig) >>
> +               clocksource_u300_1mhz.shift;
> +       return ret;
> +}
> 
>  /*
>  * This sets up the system timers, clock source and clock event.
> --
> 1.6.2.1

[PATCH] U300 sched_clock implementation

[Linus Walleij]

This patch was submitted to the ARM Linux maillist, but we need
some scheduler person to have a look at this sched_clock() thing.

The U300 timekeeping code is in the -next tree at:
arch/arm/mach-u300/time.c

The OMAP2 solution is in:
arch/arm/plat-omap/common.c

When I look at it I get the feeling, that if this works at all times, then
anything that has a continous clock source should actually
implement it this way if they have nothing else so schedule with.
I wonder how hard that would be to do in a generic way.


PATCH FOLLOWS
This overrides the global sched_clock() symbol in the Linux
scheduler with a local implementation which takes advantage of
the timesource in U300 giving a scheduling resolution of 1us. The
solution is the same as found in the OMAP2 core code.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 arch/arm/mach-u300/timer.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-u300/timer.c b/arch/arm/mach-u300/timer.c
index 4bf82f8..222de18 100644
--- a/arch/arm/mach-u300/timer.c
+++ b/arch/arm/mach-u300/timer.c
@@ -346,6 +346,22 @@ static struct clocksource clocksource_u300_1mhz = {
       .flags          = CLOCK_SOURCE_IS_CONTINUOUS,
 };

+/*
+ * Override the global weak sched_clock symbol with this
+ * local implementation which uses the clocksource to get some
+ * better resolution when scheduling the kernel. We accept that
+ * this wraps around for now, since it is just a relative time
+ * stamp. (Inspired by OMAP implementation.)
+ */
+unsigned long long sched_clock(void)
+{
+       unsigned long long ret;
+
+       ret = (unsigned long long) u300_get_cycles();
+       ret = (ret * clocksource_u300_1mhz.mult_orig) >>
+               clocksource_u300_1mhz.shift;
+       return ret;
+}

 /*
 * This sets up the system timers, clock source and clock event.
--
1.6.2.1