[RFC] Reentrant clock sources

[RFC] Reentrant clock sources

[Magnus Damm]

Hi everyone,

Is there any special reason behind the non-reentrant clock source
code? I'm writing some timer help code and getting the struct
clocksource as argument to the callbacks would make the code much
cleaner and better.

Extending the callbacks to be able to start and stop clock sources
for improved power management would be good too in my opinion.
Any thoughts?

I realize that there are quite a few clock source drivers
that need to be modified, any recommendation on which tree
to do it against and how to split up the patch? Thanks.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 include/linux/clocksource.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- 0001/include/linux/clocksource.h
+++ work/include/linux/clocksource.h	2008-11-25 21:07:13.000000000 +0900
@@ -61,14 +61,14 @@ struct clocksource {
 	char *name;
 	struct list_head list;
 	int rating;
-	cycle_t (*read)(void);
+	cycle_t (*read)(struct clocksource *cs);
 	cycle_t mask;
 	u32 mult;
 	u32 mult_orig;
 	u32 shift;
 	unsigned long flags;
-	cycle_t (*vread)(void);
-	void (*resume)(void);
+	cycle_t (*vread)(struct clocksource *cs);
+	void (*resume)(struct clocksource *cs);
 #ifdef CONFIG_IA64
 	void *fsys_mmio;        /* used by fsyscall asm code */
 #define CLKSRC_FSYS_MMIO_SET(mmio, addr)      ((mmio) = (addr))
@@ -170,7 +170,7 @@ static inline u32 clocksource_hz2mult(u3
  */
 static inline cycle_t clocksource_read(struct clocksource *cs)
 {
-	return cs->read();
+	return cs->read(cs);
 }
 
 /**

Re: [RFC] Reentrant clock sources

[Ingo Molnar]

* Magnus Damm <magnus.damm@gmail.com> wrote:

> Hi everyone,
> 
> Is there any special reason behind the non-reentrant clock source 
> code? I'm writing some timer help code and getting the struct 
> clocksource as argument to the callbacks would make the code much 
> cleaner and better.

No, there's no real reason, and cleaner code is welcome :-)

Historically most clocksource drivers started out as static and 
standalone entities and hence they always knew how to access their own 
data structures. Passing in cs and generalizing it along that line is 
certainly cleaner.

I'd suggest to not do this by wide-scale breakage of the ->read() API 
though: please introduce a ->read2() method instead, and extend GTOD 
to make use of it if it's present. This way we can gradually migrate 
to ->read2() and then remove the ->read() method atomically once the 
conversion has been finished.

> Extending the callbacks to be able to start and stop clock sources 
> for improved power management would be good too in my opinion. Any 
> thoughts?
> 
> I realize that there are quite a few clock source drivers that need 
> to be modified, any recommendation on which tree to do it against 
> and how to split up the patch? Thanks.

the timer tree is in tip/auto-timers-next, integrated into tip/master:

    http://people.redhat.com/mingo/tip.git/README

and also propagated towards linux-next. Not much is pending at the 
moment so you can work against -git too if you prefer that.

	Ingo

Re: [RFC] Reentrant clock sources

[Thomas Gleixner]

On Tue, 25 Nov 2008, Magnus Damm wrote:
> Hi everyone,
> 
> Is there any special reason behind the non-reentrant clock source
> code? I'm writing some timer help code and getting the struct
> clocksource as argument to the callbacks would make the code much
> cleaner and better.

Why do you want that ? And what has reentrancy to do with the
clocksource argument to read() ?

> Extending the callbacks to be able to start and stop clock sources
> for improved power management would be good too in my opinion.
> Any thoughts?

What have you in mind there ? Starting / stopping a clocksource when
what happens ? You can't stop them randomly except you want to screw
timekeeping.

> +	cycle_t (*vread)(struct clocksource *cs);

This is crap. vread can not access the clocksource.

Thanks,

	tglx

Re: [RFC] Reentrant clock sources

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> > +	cycle_t (*vread)(struct clocksource *cs);
> 
> This is crap. vread can not access the clocksource.

i think 'reentrant' in the sense of creating self-sufficient driver 
entities. vread wont (and shouldnt) call ->vread() recursively - but 
it might want to access fields on the clocksource.

this way more dynamic clocksource drivers could be written. Am i 
missing something?

	Ingo

Re: [RFC] Reentrant clock sources

[john stultz]

On Wed, 2008-11-26 at 04:07 +0100, Ingo Molnar wrote:
> * Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > > +	cycle_t (*vread)(struct clocksource *cs);
> > 
> > This is crap. vread can not access the clocksource.
> 
> i think 'reentrant' in the sense of creating self-sufficient driver 
> entities. vread wont (and shouldnt) call ->vread() recursively - but 
> it might want to access fields on the clocksource.

I think Thomas' issue is that vread() *cannot* access fields on the
clocksource (since vread has to be careful not to access any
non-vsyscall mapped memory).

However not all clocksources use vread(), but really I'm not quite clear
on what one would want to access in the clocksource structure when
making a ->read() call.

So maybe a further description of what specific need motivates this
change would be helpful? The brief description of power management
doesn't quite click in my head yet.

thanks
-john

Re: [RFC] Reentrant clock sources

[Ingo Molnar]

* john stultz <johnstul@us.ibm.com> wrote:

> On Wed, 2008-11-26 at 04:07 +0100, Ingo Molnar wrote:
> > * Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > > +	cycle_t (*vread)(struct clocksource *cs);
> > > 
> > > This is crap. vread can not access the clocksource.
> > 
> > i think 'reentrant' in the sense of creating self-sufficient driver 
> > entities. vread wont (and shouldnt) call ->vread() recursively - but 
> > it might want to access fields on the clocksource.
> 
> I think Thomas' issue is that vread() *cannot* access fields on the 
> clocksource (since vread has to be careful not to access any 
> non-vsyscall mapped memory).

ah, yeah - i was thinking about ->read().

in a more dynamic driver model the clocksource driver could store 
dynamic data like target port/memory address next to the clocksource 
driver, and access that - if the driver pointer is passed in.

> However not all clocksources use vread(), but really I'm not quite 
> clear on what one would want to access in the clocksource structure 
> when making a ->read() call.
> 
> So maybe a further description of what specific need motivates this 
> change would be helpful? The brief description of power management 
> doesn't quite click in my head yet.

yeah, that would be nice.

	Ingo

Re: [RFC] Reentrant clock sources

[Magnus Damm]

Hi guys,

Thanks for all the feedback so far.

On Wed, Nov 26, 2008 at 12:52 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * john stultz <johnstul@us.ibm.com> wrote:
>
>> On Wed, 2008-11-26 at 04:07 +0100, Ingo Molnar wrote:
>> > * Thomas Gleixner <tglx@linutronix.de> wrote:
>> >
>> > > > +       cycle_t (*vread)(struct clocksource *cs);
>> > >
>> > > This is crap. vread can not access the clocksource.
>> >
>> > i think 'reentrant' in the sense of creating self-sufficient driver
>> > entities. vread wont (and shouldnt) call ->vread() recursively - but
>> > it might want to access fields on the clocksource.

Uhm, I should have used that wording instead. =)

>> I think Thomas' issue is that vread() *cannot* access fields on the
>> clocksource (since vread has to be careful not to access any
>> non-vsyscall mapped memory).
>
> ah, yeah - i was thinking about ->read().

My main concern is ->read(). Initially I thought that I could pass the
clock source as argument to all callbacks, but I now understand that
passing cs to ->vread() is difficult.

> in a more dynamic driver model the clocksource driver could store
> dynamic data like target port/memory address next to the clocksource
> driver, and access that - if the driver pointer is passed in.

Exactly.

>> However not all clocksources use vread(), but really I'm not quite
>> clear on what one would want to access in the clocksource structure
>> when making a ->read() call.

Below is a link to some helper functions to manage incrementing
timers. The code is a bit rough but the idea is simple - create one
simple interface for timers that can be used as clock source and/or
clock event.

http://comments.gmane.org/gmane.linux.ports.sh.devel/4850

If you like this idea then similar helper code for decrementing timers
may be useful as well.

>> So maybe a further description of what specific need motivates this
>> change would be helpful? The brief description of power management
>> doesn't quite click in my head yet.
>
> yeah, that would be nice.

The reason for passing the clock source as argument to the read() and
resume() callbacks is that I want to use the same function for
multiple timer instances and use container_of() to get access to
per-instance private data. Without any argument I have to rely on
global data or compile time constants which makes it difficult to
reuse functions for multiple instances. Right now the timer_inc helper
code only exports a single clock source because of this limitation.

Regarding power management, the clock event code allows us to put the
hardware block for unused clock events in some kind of sleep mode when
CLOCK_EVT_MODE_SHUTDOWN or CLOCK_EVT_MODE_UNUSED is passed to
->set_mode(). I'd like to have something similar for clock sources
where unused clock sources can be powered down. For instance, we may
have two clock sources, one high resolution and one with not so good
accuracy. If the system is running with performance in mind then the
high resolution clock source should be used, but when we need to save
energy we want to switch over to the more primitive clock source and
disable the hight resolution clock source to be able to enter deeper
sleep states.

It is unfortunately a bit difficult to fully tie the clock framework
to the clock event drivers today. The timer hardware that generates
clock events for us is driven by some clock managed by the clock
framework. We use clk_enable() and clk_disable() to dynamically gate
off the clock for the timer hardware. This gives us basic run time
power management - disabling the clock when the device is unused is a
must to be able to enter deep sleep states.

Anyway, the current clock event interface wants details such as clock
rate and delta values at registration time. The problem is that the
clock framework doesn't guarantee the clock rate to be stable until
the clock is enabled, and at the time the clock is registered the
clock is disabled. So we don't have that information at clock event
registration time. And even if we did - after resuming from SHUTDOWN
or UNUSED the clock rate and deltas may have changed so we need some
way to update these parameters if we want to integrate well with the
clock framework.

I hope this makes things a bit more clear. =)

/ magnus

Re: [RFC] Reentrant clock sources

[Magnus Damm]

On Wed, Nov 26, 2008 at 6:22 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 25 Nov 2008, Magnus Damm wrote:
>> Hi everyone,
>>
>> Is there any special reason behind the non-reentrant clock source
>> code? I'm writing some timer help code and getting the struct
>> clocksource as argument to the callbacks would make the code much
>> cleaner and better.
>
> Why do you want that ? And what has reentrancy to do with the
> clocksource argument to read() ?

I should have picked my words more carefully, sorry about that. I
simply want to get some context so my callbacks can access
per-instance private data such as ioport address or irq number. Right
now I have to get the context from somewhere else.

>> Extending the callbacks to be able to start and stop clock sources
>> for improved power management would be good too in my opinion.
>> Any thoughts?
>
> What have you in mind there ? Starting / stopping a clocksource when
> what happens ? You can't stop them randomly except you want to screw
> timekeeping.

Is changing clock source using sysfs known to screw up the time keeping?

# echo jiffies >
/sys/devices/system/clocksource/clocksource0/current_clocksource

The line above seems to work well with dynamic ticks disabled, but the
current code doesn't seem to handle the nohz -> periodic transition
very well.

>> +     cycle_t (*vread)(struct clocksource *cs);
>
> This is crap. vread can not access the clocksource.

Yeah, sorry about that. =)

Thanks for your comments.

/ magnus