Reentrant clock sources

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.

---

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

@@ -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);
 }
 
 /**

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.

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

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

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.

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

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

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.

yeah, that would be nice.

Ingo

Hi guys,

Thanks for all the feedback so far.

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

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.

Exactly.

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.

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

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.

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.

Yeah, sorry about that. =)

Thanks for your comments.

/ magnus