[PATCH 1/5] generic clocksource updates

[PATCH 1/5] generic clocksource updates

[Roman Zippel]

A number of changes to the clocksource infrastructur:
- use xtime_lock instead of clocksource_lock, so it's easier to
  synchronize the clocksource switch for gettimeofday.
- add a few clock state variables (instead of making them global).
- clocksource_get_nsec_offset(): this should become clocksource specific
  later.
- select_clocks: immediately switch clocksource.
- cleanup naming to put the subsystem name in front

Signed-off-by: Roman Zippel <zippel@linux-m68k.org>

---

 include/linux/clocksource.h |   33 +++++++-------
 kernel/Makefile             |    1 
 kernel/time/clocksource.c   |   99 +++++++++++++++++---------------------------
 kernel/time/jiffies.c       |   16 ++++---
 4 files changed, 67 insertions(+), 82 deletions(-)

Index: linux-2.6-mm/include/linux/clocksource.h
===================================================================
--- linux-2.6-mm.orig/include/linux/clocksource.h	2006-04-02 06:53:29.000000000 +0200
+++ linux-2.6-mm/include/linux/clocksource.h	2006-04-02 16:12:28.000000000 +0200
@@ -45,9 +45,8 @@ typedef u64 cycle_t;
  * @mult:		cycle to nanosecond multiplier
  * @shift:		cycle to nanosecond divisor (power of two)
  * @update_callback:	called when safe to alter clocksource values
- * @is_continuous:	defines if clocksource is free-running.
- * @interval_cycles:	Used internally by timekeeping core, please ignore.
- * @interval_snsecs:	Used internally by timekeeping core, please ignore.
+ * @cycle_update:	Used internally by timekeeping core, please ignore.
+ * @xtime_update:	Used internally by timekeeping core, please ignore.
  */
 struct clocksource {
 	char *name;
@@ -58,11 +57,11 @@ struct clocksource {
 	u32 mult;
 	u32 shift;
 	int (*update_callback)(void);
-	int is_continuous;
 
 	/* timekeeping specific data, ignore */
-	cycle_t interval_cycles;
-	u64 interval_snsecs;
+	u64 cycles_last, cycle_update;
+	u64 xtime_nsec, xtime_update;
+	s64 ntp_error;
 };
 
 
@@ -145,7 +144,7 @@ static inline s64 cyc2ns(struct clocksou
 }
 
 /**
- * calculate_clocksource_interval - Calculates a clocksource interval struct
+ * clocksource_calculate_interval - Calculates a clocksource interval struct
  *
  * @c:		Pointer to clocksource.
  * @length_nsec: Desired interval length in nanoseconds.
@@ -155,8 +154,8 @@ static inline s64 cyc2ns(struct clocksou
  *
  * Unless you're the timekeeping code, you should not be using this!
  */
-static inline void calculate_clocksource_interval(struct clocksource *c,
-						unsigned long length_nsec)
+static inline void clocksource_calculate_interval(struct clocksource *c,
+						  unsigned long length_nsec)
 {
 	u64 tmp;
 
@@ -166,16 +165,18 @@ static inline void calculate_clocksource
 	tmp += c->mult/2;
 	do_div(tmp, c->mult);
 
-	c->interval_cycles = (cycle_t)tmp;
-	if(c->interval_cycles == 0)
-		c->interval_cycles = 1;
+	c->cycle_update = (cycle_t)tmp;
+	if (c->cycle_update == 0)
+		c->cycle_update = 1;
 
-	c->interval_snsecs = (u64)c->interval_cycles * c->mult;
+	c->xtime_update = (u64)c->cycle_update * c->mult;
 }
 
 /* used to install a new clocksource */
-int register_clocksource(struct clocksource*);
-void reselect_clocksource(void);
-struct clocksource* get_next_clocksource(void);
+int clocksource_register(struct clocksource *);
+void clocksource_reselect(void);
+unsigned long clocksource_get_nsec_offset(struct clocksource *cs);
+
+extern struct clocksource *curr_clocksource;
 
 #endif /* _LINUX_CLOCKSOURCE_H */
Index: linux-2.6-mm/kernel/Makefile
===================================================================
--- linux-2.6-mm.orig/kernel/Makefile	2006-04-02 06:52:06.000000000 +0200
+++ linux-2.6-mm/kernel/Makefile	2006-04-02 16:11:03.000000000 +0200
@@ -10,6 +10,7 @@ obj-y     = sched.o fork.o exec_domain.o
 	    kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \
 	    hrtimer.o
 
+obj-y += time/
 obj-$(CONFIG_DEBUG_MUTEXES) += mutex-debug.o
 obj-$(CONFIG_FUTEX) += futex.o
 ifeq ($(CONFIG_COMPAT),y)
Index: linux-2.6-mm/kernel/time/clocksource.c
===================================================================
--- linux-2.6-mm.orig/kernel/time/clocksource.c	2006-04-02 06:53:29.000000000 +0200
+++ linux-2.6-mm/kernel/time/clocksource.c	2006-04-02 16:11:28.000000000 +0200
@@ -35,57 +35,28 @@ extern struct clocksource clocksource_ji
 /*[Clocksource internal variables]---------
  * curr_clocksource:
  *	currently selected clocksource. Initialized to clocksource_jiffies.
- * next_clocksource:
- *	pending next selected clocksource.
  * clocksource_list:
  *	linked list with the registered clocksources
- * clocksource_lock:
- *	protects manipulations to curr_clocksource and next_clocksource
- *	and the clocksource_list
  * override_name:
  *	Name of the user-specified clocksource.
  */
-static struct clocksource *curr_clocksource = &clocksource_jiffies;
-static struct clocksource *next_clocksource;
+struct clocksource *curr_clocksource = &clocksource_jiffies;
 static LIST_HEAD(clocksource_list);
-static DEFINE_SPINLOCK(clocksource_lock);
 static char override_name[32];
-static int finished_booting;
 
-/* clocksource_done_booting - Called near the end of bootup
- *
- * Hack to avoid lots of clocksource churn at boot time
- */
-static int clocksource_done_booting(void)
+unsigned long clocksource_get_nsec_offset(struct clocksource *cs)
 {
-	finished_booting = 1;
-	return 0;
-}
+	cycle_t cycle_delta;
 
-late_initcall(clocksource_done_booting);
+	cycle_delta = (cs->read() - cs->cycles_last) & cs->mask;
 
-/**
- * get_next_clocksource - Returns the selected clocksource
- *
- */
-struct clocksource *get_next_clocksource(void)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&clocksource_lock, flags);
-	if (next_clocksource && finished_booting) {
-		curr_clocksource = next_clocksource;
-		next_clocksource = NULL;
-	}
-	spin_unlock_irqrestore(&clocksource_lock, flags);
-
-	return curr_clocksource;
+	return (cs->xtime_nsec + cycle_delta * cs->mult) >> cs->shift;
 }
 
 /**
  * select_clocksource - Finds the best registered clocksource.
  *
- * Private function. Must hold clocksource_lock when called.
+ * Private function. Must hold xtime_lock when called.
  *
  * Looks through the list of registered clocksources, returning
  * the one with the highest rating value. If there is a clocksource
@@ -114,6 +85,17 @@ static struct clocksource *select_clocks
 		 	best = src;
 	}
 
+	if (curr_clocksource != best) {
+		printk("switching to clocksource %s\n", best->name);
+		if (curr_clocksource)
+			xtime.tv_nsec = clocksource_get_nsec_offset(curr_clocksource);
+		best->ntp_error = 0;
+		best->cycles_last = best->read();
+		best->xtime_nsec = (u64)xtime.tv_nsec << best->shift;
+		clocksource_calculate_interval(best, tick_nsec);
+		curr_clocksource = best;
+	}
+
 	return best;
 }
 
@@ -121,7 +103,7 @@ static struct clocksource *select_clocks
  * is_registered_source - Checks if clocksource is registered
  * @c:		pointer to a clocksource
  *
- * Private helper function. Must hold clocksource_lock when called.
+ * Private helper function. Must hold xtime_lock when called.
  *
  * Returns one if the clocksource is already registered, zero otherwise.
  */
@@ -142,48 +124,45 @@ static int is_registered_source(struct c
 }
 
 /**
- * register_clocksource - Used to install new clocksources
+ * clocksource_register - Used to install new clocksources
  * @t:		clocksource to be registered
  *
  * Returns -EBUSY if registration fails, zero otherwise.
  */
-int register_clocksource(struct clocksource *c)
+int clocksource_register(struct clocksource *c)
 {
 	int ret = 0;
-	unsigned long flags;
 
-	spin_lock_irqsave(&clocksource_lock, flags);
+	write_seqlock_irq(&xtime_lock);
 	/* check if clocksource is already registered */
 	if (is_registered_source(c)) {
-		printk("register_clocksource: Cannot register %s. "
+		printk("clocksource_register: Cannot register %s. "
 			"Already registered!", c->name);
 		ret = -EBUSY;
 	} else {
 		/* register it */
  		list_add(&c->list, &clocksource_list);
 		/* scan the registered clocksources, and pick the best one */
-		next_clocksource = select_clocksource();
+		select_clocksource();
 	}
-	spin_unlock_irqrestore(&clocksource_lock, flags);
+	write_sequnlock_irq(&xtime_lock);
 	return ret;
 }
 
-EXPORT_SYMBOL(register_clocksource);
+EXPORT_SYMBOL(clocksource_register);
 
 /**
- * reselect_clocksource - Rescan list for next clocksource
+ * clocksource_reselect - Rescan list for next clocksource
  *
  * A quick helper function to be used if a clocksource changes its
  * rating. Forces the clocksource list to be re-scaned for the best
  * clocksource.
  */
-void reselect_clocksource(void)
+void clocksource_reselect(void)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&clocksource_lock, flags);
-	next_clocksource = select_clocksource();
-	spin_unlock_irqrestore(&clocksource_lock, flags);
+	write_seqlock_irq(&xtime_lock);
+	select_clocksource();
+	write_sequnlock_irq(&xtime_lock);
 }
 
 /**
@@ -198,9 +177,9 @@ sysfs_show_current_clocksources(struct s
 {
 	char *curr = buf;
 
-	spin_lock_irq(&clocksource_lock);
+	write_seqlock_irq(&xtime_lock);
 	curr += sprintf(curr, "%s ", curr_clocksource->name);
-	spin_unlock_irq(&clocksource_lock);
+	write_sequnlock_irq(&xtime_lock);
 
 	curr += sprintf(curr, "\n");
 
@@ -230,16 +209,16 @@ static ssize_t sysfs_override_clocksourc
 	if (count < 1)
 		return -EINVAL;
 
-	spin_lock_irq(&clocksource_lock);
+	write_seqlock_irq(&xtime_lock);
 
 	/* copy the name given: */
 	memcpy(override_name, buf, count);
 	override_name[count] = 0;
 
 	/* try to select it: */
-	next_clocksource = select_clocksource();
+	select_clocksource();
 
-	spin_unlock_irq(&clocksource_lock);
+	write_sequnlock_irq(&xtime_lock);
 
 	return ret;
 }
@@ -257,14 +236,14 @@ sysfs_show_available_clocksources(struct
 	struct list_head *tmp;
 	char *curr = buf;
 
-	spin_lock_irq(&clocksource_lock);
+	write_seqlock_irq(&xtime_lock);
 	list_for_each(tmp, &clocksource_list) {
 		struct clocksource *src;
 
 		src = list_entry(tmp, struct clocksource, list);
 		curr += sprintf(curr, "%s ", src->name);
 	}
-	spin_unlock_irq(&clocksource_lock);
+	write_sequnlock_irq(&xtime_lock);
 
 	curr += sprintf(curr, "\n");
 
@@ -318,10 +297,10 @@ device_initcall(init_clocksource_sysfs);
 static int __init boot_override_clocksource(char* str)
 {
 	unsigned long flags;
-	spin_lock_irqsave(&clocksource_lock, flags);
+	write_seqlock_irqsave(&xtime_lock, flags);
 	if (str)
 		strlcpy(override_name, str, sizeof(override_name));
-	spin_unlock_irqrestore(&clocksource_lock, flags);
+	write_sequnlock_irqrestore(&xtime_lock, flags);
 	return 1;
 }
 
Index: linux-2.6-mm/kernel/time/jiffies.c
===================================================================
--- linux-2.6-mm.orig/kernel/time/jiffies.c	2006-04-02 06:53:29.000000000 +0200
+++ linux-2.6-mm/kernel/time/jiffies.c	2006-04-02 06:53:29.000000000 +0200
@@ -50,10 +50,7 @@
  */
 #define JIFFIES_SHIFT	8
 
-static cycle_t jiffies_read(void)
-{
-	return (cycle_t) jiffies;
-}
+static cycle_t jiffies_read(void);
 
 struct clocksource clocksource_jiffies = {
 	.name		= "jiffies",
@@ -62,12 +59,19 @@ struct clocksource clocksource_jiffies =
 	.mask		= 0xffffffff, /*32bits*/
 	.mult		= NSEC_PER_JIFFY << JIFFIES_SHIFT, /* details above */
 	.shift		= JIFFIES_SHIFT,
-	.is_continuous	= 0, /* tick based, not free running */
+
+	.cycle_update	= 1,
+	.xtime_update	= NSEC_PER_JIFFY << JIFFIES_SHIFT,
 };
 
+static cycle_t jiffies_read(void)
+{
+	return clocksource_jiffies.cycles_last + 1;
+}
+
 static int __init init_jiffies_clocksource(void)
 {
-	return register_clocksource(&clocksource_jiffies);
+	return clocksource_register(&clocksource_jiffies);
 }
 
 module_init(init_jiffies_clocksource);

Re: [PATCH 1/5] generic clocksource updates

[Thomas Gleixner]

On Mon, 2006-04-03 at 21:55 +0200, Roman Zippel wrote:
>  struct clocksource {
>  	char *name;
> @@ -58,11 +57,11 @@ struct clocksource {
>  	u32 mult;
>  	u32 shift;
>  	int (*update_callback)(void);
> -	int is_continuous;

This field was introduced to have a clear property description. The
rating field might be used for this, but from a given rating on a
particular CPU architecture it might be hard to deduce whether this
clock source is good enough so we can switch to high resolution timer
mode.

is_continous is maybe not the best choice, but a flag field to retrieve
certain properties would be a good thing to have.

	tglx

Re: [PATCH 1/5] generic clocksource updates

[Roman Zippel]

Hi,

On Thu, 6 Apr 2006, Thomas Gleixner wrote:

> On Mon, 2006-04-03 at 21:55 +0200, Roman Zippel wrote:
> >  struct clocksource {
> >  	char *name;
> > @@ -58,11 +57,11 @@ struct clocksource {
> >  	u32 mult;
> >  	u32 shift;
> >  	int (*update_callback)(void);
> > -	int is_continuous;
> 
> This field was introduced to have a clear property description. The
> rating field might be used for this, but from a given rating on a
> particular CPU architecture it might be hard to deduce whether this
> clock source is good enough so we can switch to high resolution timer
> mode.

Currently this field isn't needed and as soon we have a need for it, we 
can add proper capability information.

bye, Roman

Re: [PATCH 1/5] generic clocksource updates

[Thomas Gleixner]

On Thu, 2006-04-06 at 21:00 +0200, Roman Zippel wrote:
> > > -	int is_continuous;
> > 
> > This field was introduced to have a clear property description. The
> > rating field might be used for this, but from a given rating on a
> > particular CPU architecture it might be hard to deduce whether this
> > clock source is good enough so we can switch to high resolution timer
> > mode.
> 
> Currently this field isn't needed and as soon we have a need for it, we 
> can add proper capability information.

Is there a reason, why requirements which are known from existing
experience must be discarded to be reintroduced later ?

	tglx

Re: [PATCH 1/5] generic clocksource updates

[john stultz]

On Mon, 2006-04-03 at 21:55 +0200, Roman Zippel wrote:
> A number of changes to the clocksource infrastructur:
> - use xtime_lock instead of clocksource_lock, so it's easier to
>   synchronize the clocksource switch for gettimeofday.

I'm not sure I understand the benefit of this. Could you explain?

> - add a few clock state variables (instead of making them global).

This goes against some of my goals for keeping the clocksource stupid
and simple (just a method for hardware access), so I don't know if I
view this as a good change. Most of the global variables it replaces
were static to a single function. Could you explain the rational a bit?

More on this below.

> - clocksource_get_nsec_offset(): this should become clocksource specific
>   later.
> - select_clocks: immediately switch clocksource.

Again, why is this desired?

> - cleanup naming to put the subsystem name in front

Most of these I can agree with.  :)


>  include/linux/clocksource.h |   33 +++++++-------
>  kernel/Makefile             |    1 
>  kernel/time/clocksource.c   |   99 +++++++++++++++++---------------------------
>  kernel/time/jiffies.c       |   16 ++++---
>  4 files changed, 67 insertions(+), 82 deletions(-)
> 
> Index: linux-2.6-mm/include/linux/clocksource.h
> ===================================================================
> --- linux-2.6-mm.orig/include/linux/clocksource.h	2006-04-02 06:53:29.000000000 +0200
> +++ linux-2.6-mm/include/linux/clocksource.h	2006-04-02 16:12:28.000000000 +0200
> @@ -45,9 +45,8 @@ typedef u64 cycle_t;
>   * @mult:		cycle to nanosecond multiplier
>   * @shift:		cycle to nanosecond divisor (power of two)
>   * @update_callback:	called when safe to alter clocksource values
> - * @is_continuous:	defines if clocksource is free-running.
> - * @interval_cycles:	Used internally by timekeeping core, please ignore.
> - * @interval_snsecs:	Used internally by timekeeping core, please ignore.
> + * @cycle_update:	Used internally by timekeeping core, please ignore.
> + * @xtime_update:	Used internally by timekeeping core, please ignore.

I don't think cycle_update/xtime_update are very clear variable names.
The interval_cycles/snsecs are the per-interval cycle and shifted
nanosecond components. Could you explain what you feel makes yours more
clear?


>  struct clocksource {
>  	char *name;
> @@ -58,11 +57,11 @@ struct clocksource {
>  	u32 mult;
>  	u32 shift;
>  	int (*update_callback)(void);
> -	int is_continuous;
>  
>  	/* timekeeping specific data, ignore */
> -	cycle_t interval_cycles;
> -	u64 interval_snsecs;
> +	u64 cycles_last, cycle_update;
> +	u64 xtime_nsec, xtime_update;
> +	s64 ntp_error;

My opinion: These add very timekeeping specific state values to the
clocksource structure. In my initial design, the clocksource structures
have no idea what they might be used for, so they only provide
information about the hardware. In my mind, this avoids mixing
functionality between files and makes the code easier to read. So I'd
prefer to keep it more limited (even going as far as removing the
interval_xxx units) to keep the clocksource structure from having any
timekeeping data in it.

Could you maybe provide more info why this information should be stored
in the clocksource structure instead of keeping it in the timekeeping
code?

Also again, the names are not clear and there isn't much documentation
as to what these values are. For example, xtime_nsec: it isn't even a
nsec value (its a shifted nanosecond value), so it doesn't make much
sense at all.

> Index: linux-2.6-mm/kernel/time/clocksource.c
> ===================================================================
> --- linux-2.6-mm.orig/kernel/time/clocksource.c	2006-04-02 06:53:29.000000000 +0200
> +++ linux-2.6-mm/kernel/time/clocksource.c	2006-04-02 16:11:28.000000000 +0200
> @@ -35,57 +35,28 @@ extern struct clocksource clocksource_ji
>  /*[Clocksource internal variables]---------
>   * curr_clocksource:
>   *	currently selected clocksource. Initialized to clocksource_jiffies.
> - * next_clocksource:
> - *	pending next selected clocksource.
>   * clocksource_list:
>   *	linked list with the registered clocksources
> - * clocksource_lock:
> - *	protects manipulations to curr_clocksource and next_clocksource
> - *	and the clocksource_list
>   * override_name:
>   *	Name of the user-specified clocksource.
>   */
> -static struct clocksource *curr_clocksource = &clocksource_jiffies;
> -static struct clocksource *next_clocksource;
> +struct clocksource *curr_clocksource = &clocksource_jiffies;
>  static LIST_HEAD(clocksource_list);
> -static DEFINE_SPINLOCK(clocksource_lock);
>  static char override_name[32];
> -static int finished_booting;

Again, why is it necessary to switch the clocksources here? It is what
forces all of the timekeeping code to be mingled in the clocksource
management. I think the way I did it was pretty clean, limiting
clocksource.c to only registration and selection of a clocksource. Now
we have timekeeping code in clocksource.c too? What is the gain here?

Also, why do you want to protect the list of clocksources w/
xtime_lock? 

thanks
-john

Re: [PATCH 1/5] generic clocksource updates

[Roman Zippel]

Hi,

On Thu, 6 Apr 2006, Thomas Gleixner wrote:

> > Currently this field isn't needed and as soon we have a need for it, we 
> > can add proper capability information.
> 
> Is there a reason, why requirements which are known from existing
> experience must be discarded to be reintroduced later ?

Then please explain these requirements.
This field shouldn't have been added in first place, I guess I managed to 
confuse John when I talked about handling of continuous vs. tick based 
clocks. Currently no user should even care about this, it's an 
implementation detail of the clock.

bye, Roman

Re: [PATCH 1/5] generic clocksource updates

[john stultz]

On Fri, 2006-04-07 at 22:43 +0200, Roman Zippel wrote:
> Hi,
> 
> On Thu, 6 Apr 2006, Thomas Gleixner wrote:
> 
> > > Currently this field isn't needed and as soon we have a need for it, we 
> > > can add proper capability information.
> > 
> > Is there a reason, why requirements which are known from existing
> > experience must be discarded to be reintroduced later ?
> 
> Then please explain these requirements.
> This field shouldn't have been added in first place, I guess I managed to 
> confuse John when I talked about handling of continuous vs. tick based 
> clocks. Currently no user should even care about this, it's an 
> implementation detail of the clock.

I don't think you confused me on this issue (although, I admit I'm prone
to confusion). 

The is_continuous flag on the clocksource is used so other systems can
query if timekeeping is able to function without regular timer ticks.
This would be necessary for the HRT patchset, as well as the dynamic
tick patches, as they both reprograms the tick frequency, and need to
know if that will affect time.

I can reasonably drop this bit from the current patches, but it is very
small and and will be needed shortly, so I'm not sure its that big of a
deal.

thanks
-john

Re: [PATCH 1/5] generic clocksource updates

[Thomas Gleixner]

Roman,

On Fri, 2006-04-07 at 22:43 +0200, Roman Zippel wrote:
> > > Currently this field isn't needed and as soon we have a need for it, we 
> > > can add proper capability information.
> > 
> > Is there a reason, why requirements which are known from existing
> > experience must be discarded to be reintroduced later ?
> 
> Then please explain these requirements.

I explained them very clear in my original post:

http://marc.theaimsgroup.com/?l=linux-kernel&m=114431804702870&w=2

> This field shouldn't have been added in first place, I guess I managed to 
> confuse John when I talked about handling of continuous vs. tick based 
> clocks.

I appreciate your responsibility, but the is_continous field was
introduced on my request because I did not want to rely on (rating > X)
to decide whether I can safely switch to high resolution timer mode or
not.

I really do not understand, why you claim to be the ultimate authority
to decide whats right and wrong in this area. Can you please shed some
light on my agnostic mind with some _real_ explanation why you can claim
to have the authority to decide whats needed and whats not needed ?

"Currently this field isn't needed and as soon we have a need ....".
----------------------------------------------^^^^

I guess http://en.wikipedia.org/wiki/Pluralis_Majestatis is the right
place for me until you start to explain.

> Currently no user should even care about this, it's an 
> implementation detail of the clock.

Right. Even if no user cares about this right now, nevertheless
forseeing the requirements of the near future is the finer art of
engineering. Especially if there is existing experience, which shows
that it is necessary.

	tglx

Re: [PATCH 1/5] generic clocksource updates

[Roman Zippel]

Hi,

On Sat, 8 Apr 2006, Thomas Gleixner wrote:

> > Then please explain these requirements.
> 
> I explained them very clear in my original post:
> 
> http://marc.theaimsgroup.com/?l=linux-kernel&m=114431804702870&w=2
> 
> > This field shouldn't have been added in first place, I guess I managed to 
> > confuse John when I talked about handling of continuous vs. tick based 
> > clocks.
> 
> I appreciate your responsibility, but the is_continous field was
> introduced on my request because I did not want to rely on (rating > X)
> to decide whether I can safely switch to high resolution timer mode or
> not.

What you didn't explain is why exactly makes this field a difference. 
Right now one can only read the clocksource, so why should it make a 
difference how the time is generated?

> I really do not understand, why you claim to be the ultimate authority
> to decide whats right and wrong in this area. Can you please shed some
> light on my agnostic mind with some _real_ explanation why you can claim
> to have the authority to decide whats needed and whats not needed ?
> 
> "Currently this field isn't needed and as soon we have a need ....".
> ----------------------------------------------^^^^
> 
> I guess http://en.wikipedia.org/wiki/Pluralis_Majestatis is the right
> place for me until you start to explain.

Out of curiosity: what kind of response do you expect to such crap?

(To understand how ridiculous this is, one only has to know whose time(r) 
patches get dropped by Andrew like hot potatoes.)

> > Currently no user should even care about this, it's an 
> > implementation detail of the clock.
> 
> Right. Even if no user cares about this right now, nevertheless
> forseeing the requirements of the near future is the finer art of
> engineering. Especially if there is existing experience, which shows
> that it is necessary.

I do respect your experience, but you also have to show it.

bye, Roman

Re: [PATCH 1/5] generic clocksource updates

[Roman Zippel]

Hi,

On Fri, 7 Apr 2006, john stultz wrote:

> On Mon, 2006-04-03 at 21:55 +0200, Roman Zippel wrote:
> > A number of changes to the clocksource infrastructur:
> > - use xtime_lock instead of clocksource_lock, so it's easier to
> >   synchronize the clocksource switch for gettimeofday.
> 
> I'm not sure I understand the benefit of this. Could you explain?

Switching the system clock requires synchronizing using xtime_lock, since 
the other uses of clocksource_lock were not critical, it's easier to use 
there xtime_lock as well. This makes the clock switch code simpler.

> > - add a few clock state variables (instead of making them global).
> 
> This goes against some of my goals for keeping the clocksource stupid
> and simple (just a method for hardware access), so I don't know if I
> view this as a good change. Most of the global variables it replaces
> were static to a single function. Could you explain the rational a bit?

There is one practical reason: Accessing the variables via a pointer 
allows the compiler to produce smaller code.
>From a design point of view there is no practical reason to not have 
multiple clocks. These variables belong to the clock and they can be used 
for more than just synchronizing with the NTP system, e.g. the same basic 
idea can be used to lock a fast but unstable TSC clock to a slower but 
more stable clock.

> > - * @is_continuous:	defines if clocksource is free-running.
> > - * @interval_cycles:	Used internally by timekeeping core, please ignore.
> > - * @interval_snsecs:	Used internally by timekeeping core, please ignore.
> > + * @cycle_update:	Used internally by timekeeping core, please ignore.
> > + * @xtime_update:	Used internally by timekeeping core, please ignore.
> 
> I don't think cycle_update/xtime_update are very clear variable names.
> The interval_cycles/snsecs are the per-interval cycle and shifted
> nanosecond components. Could you explain what you feel makes yours more
> clear?

The names should be more clear in the context of the design document I've 
sent. As there are derived from it, I would prefer comments regarding to 
this in the context of this document.
Anyway, using interval instead of update sounds good, but many variables 
in this area are fixed point values, so I don't see much value in 
explicitely stating that there are "shifted" (or do you do the same with 
floating point values?). 

> >  struct clocksource {
> >  	char *name;
> > @@ -58,11 +57,11 @@ struct clocksource {
> >  	u32 mult;
> >  	u32 shift;
> >  	int (*update_callback)(void);
> > -	int is_continuous;
> >  
> >  	/* timekeeping specific data, ignore */
> > -	cycle_t interval_cycles;
> > -	u64 interval_snsecs;
> > +	u64 cycles_last, cycle_update;
> > +	u64 xtime_nsec, xtime_update;
> > +	s64 ntp_error;
> 
> My opinion: These add very timekeeping specific state values to the
> clocksource structure. In my initial design, the clocksource structures
> have no idea what they might be used for, so they only provide
> information about the hardware. In my mind, this avoids mixing
> functionality between files and makes the code easier to read. So I'd
> prefer to keep it more limited (even going as far as removing the
> interval_xxx units) to keep the clocksource structure from having any
> timekeeping data in it.
> 
> Could you maybe provide more info why this information should be stored
> in the clocksource structure instead of keeping it in the timekeeping
> code?

Well, in other programming languages there other ways to keep this 
separate, but this is C and these variables belong to the clock if it's to 
be used as system clock.

> Also again, the names are not clear and there isn't much documentation
> as to what these values are.

You did get the separate document, which describes the clock updates?

> For example, xtime_nsec: it isn't even a
> nsec value (its a shifted nanosecond value), so it doesn't make much
> sense at all.

Actually your sentence doesn't make much sense. :)
A shifted nsec value is also a nsec value, i.e the shifted value is just a 
special case (aka a fixed pointed value).

> Again, why is it necessary to switch the clocksources here? It is what
> forces all of the timekeeping code to be mingled in the clocksource
> management. I think the way I did it was pretty clean, limiting
> clocksource.c to only registration and selection of a clocksource. Now
> we have timekeeping code in clocksource.c too? What is the gain here?

It's IMO better than the alternative to make the code clock switching 
in timer.c even more complex. A clock switch is a rare event, I don't see 
a good reason to check for it every single timer interrupt.
We can try to clean this up later, but in this case I prefer the 
simplicity.

bye, Roman

Re: [PATCH 1/5] generic clocksource updates

[john stultz]

On Thu, 2006-04-27 at 22:41 +0200, Roman Zippel wrote:
> Hi,
> 
> On Fri, 7 Apr 2006, john stultz wrote:
> 
> > On Mon, 2006-04-03 at 21:55 +0200, Roman Zippel wrote:
> > > A number of changes to the clocksource infrastructur:
> > > - use xtime_lock instead of clocksource_lock, so it's easier to
> > >   synchronize the clocksource switch for gettimeofday.
> > 
> > I'm not sure I understand the benefit of this. Could you explain?
> 
> Switching the system clock requires synchronizing using xtime_lock, since 
> the other uses of clocksource_lock were not critical, it's easier to use 
> there xtime_lock as well. This makes the clock switch code simpler.

Here I disagree, as it makes the code less understandable. But this is a
minor issue.

> > > - add a few clock state variables (instead of making them global).
> > 
> > This goes against some of my goals for keeping the clocksource stupid
> > and simple (just a method for hardware access), so I don't know if I
> > view this as a good change. Most of the global variables it replaces
> > were static to a single function. Could you explain the rational a bit?
> 
> There is one practical reason: Accessing the variables via a pointer 
> allows the compiler to produce smaller code.

Ok, that's a good reason, however can we keep the clocksource
abstraction clean by adding a middle layer for the timekeeping values?

ie:
	struct timekeeping_data {
		struct clocksource clock;
		cycle_t last_cycle
		cycle_t interval_cycle
		s64 interval_snsec
		s64 snsec_remainder
	} time_data;

That way the abstraction is cleaner. It would pull the
interval_snsecs/interval_cycles values out and clocksources would simply
export counter/frequency data. Then the timkeeping core would
clocksources as well as its own data to export time. 


> From a design point of view there is no practical reason to not have 
> multiple clocks. These variables belong to the clock and they can be used 
> for more than just synchronizing with the NTP system, e.g. the same basic 
> idea can be used to lock a fast but unstable TSC clock to a slower but 
> more stable clock.

Well, TSCs are mostly junk, so I don't know if that is really doable.
I'm also not so sure of the utility of multiple clocks, but I'm
interested in hearing more.  However I disagree those variables belong
to the clocksource driver. Since clocksources just export a cycle
counter and frequency data, they can be used for more then just
timekeeping. Delay loops come to mind as one possibility.


> > > - * @is_continuous:	defines if clocksource is free-running.
> > > - * @interval_cycles:	Used internally by timekeeping core, please ignore.
> > > - * @interval_snsecs:	Used internally by timekeeping core, please ignore.
> > > + * @cycle_update:	Used internally by timekeeping core, please ignore.
> > > + * @xtime_update:	Used internally by timekeeping core, please ignore.
> > 
> > I don't think cycle_update/xtime_update are very clear variable names.
> > The interval_cycles/snsecs are the per-interval cycle and shifted
> > nanosecond components. Could you explain what you feel makes yours more
> > clear?
> 
> The names should be more clear in the context of the design document I've 
> sent. As there are derived from it, I would prefer comments regarding to 
> this in the context of this document.
> Anyway, using interval instead of update sounds good, but many variables 
> in this area are fixed point values, so I don't see much value in 
> explicitely stating that there are "shifted" (or do you do the same with 
> floating point values?). 


Well, I don't call floating point values integers. Just because we're
dealing with time doesn't mean the unit has no meaning. 

The unit described in the name is what people expect to get when they
print the value. This is important for people to be able to read the
code and quickly understand it without reading pages of design
documentation.

> > Again, why is it necessary to switch the clocksources here? It is what
> > forces all of the timekeeping code to be mingled in the clocksource
> > management. I think the way I did it was pretty clean, limiting
> > clocksource.c to only registration and selection of a clocksource. Now
> > we have timekeeping code in clocksource.c too? What is the gain here?
> 
> It's IMO better than the alternative to make the code clock switching 
> in timer.c even more complex. A clock switch is a rare event, I don't see 
> a good reason to check for it every single timer interrupt.
> We can try to clean this up later, but in this case I prefer the 
> simplicity.

Ok, I think I can agree w/ this if we use something like the struct
timekeeping_data layer above.

Thanks for the feedback.
-john