2009-06-01 07:46:19

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH] U300 sched_clock implementation

2009/5/25 Peter Zijlstra <[email protected]>:
> On Mon, 2009-05-25 at 14:13 +0200, Linus Walleij wrote:
>> 2009/5/24 Peter Zijlstra <[email protected]>:
>>
>> > 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 <[email protected]> ?

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

Yours,
Linus Walleij


2009-06-02 09:00:24

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] U300 sched_clock implementation

On Mon, 2009-06-01 at 09:46 +0200, Linus Walleij wrote:
> 2009/5/25 Peter Zijlstra <[email protected]>:
> > On Mon, 2009-05-25 at 14:13 +0200, Linus Walleij wrote:
> >> 2009/5/24 Peter Zijlstra <[email protected]>:
> >>
> >> > 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 <[email protected]> ?

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

2009-07-07 07:43:13

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH] U300 sched_clock implementation

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 <[email protected]>:
> > > On Mon, 2009-05-25 at 14:13 +0200, Linus Walleij wrote:
> > >> 2009/5/24 Peter Zijlstra <[email protected]>:
> > >>
> > >> > 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 <[email protected]> ?
>
> 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.

2009-07-07 08:02:18

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH] U300 sched_clock implementation

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 <[email protected]>:
> 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 <[email protected]>:
>> > > On Mon, 2009-05-25 at 14:13 +0200, Linus Walleij wrote:
>> > >> 2009/5/24 Peter Zijlstra <[email protected]>:
>> > >>
>> > >> > 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 <[email protected]> ?
>>
>> 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

2009-07-08 09:35:34

by Paul Mundt

[permalink] [raw]
Subject: Re: [PATCH] U300 sched_clock implementation

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 <[email protected]>:
> > 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/.