[PATCH] time : SMP friendly alignment of struct clocksource

[PATCH] time : SMP friendly alignment of struct clocksource

[Eric Dumazet]

struct clocksource is a critical data structure.

Most of its fields are read only, some of them are heavily modified at each timer interrupt.

It makes sense to separate those fields and make sure they all share one cache line, or at least the minimum for machines with small cache lines.

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

--- linux-2.6.21-rc4-mm1/include/linux/clocksource.h
+++ linux-2.6.21-rc4-mm1-ed/include/linux/clocksource.h
@@ -49,25 +49,35 @@ struct clocksource;
  * @flags:		flags describing special properties
  * @vread:		vsyscall based read
  * @cycle_interval:	Used internally by timekeeping core, please ignore.
  * @xtime_interval:	Used internally by timekeeping core, please ignore.
  */
 struct clocksource {
+	/*
+	 * First part of structure is read mostly
+	 */
 	char *name;
 	struct list_head list;
 	int rating;
 	cycle_t (*read)(void);
 	cycle_t mask;
 	u32 mult;
 	u32 shift;
 	unsigned long flags;
 	cycle_t (*vread)(void);
 
 	/* timekeeping specific data, ignore */
-	cycle_t cycle_last, cycle_interval;
-	u64 xtime_nsec, xtime_interval;
+	cycle_t cycle_interval;
+	u64	xtime_interval;
+	/*
+	 * Second part is written at each timer interrupt
+	 * Keep it in a different cache line to dirty no
+	 * more than one cache line.
+	 */
+	cycle_t cycle_last ____cacheline_aligned_in_smp;
+	u64 xtime_nsec;
 	s64 error;
 
 #ifdef CONFIG_CLOCKSOURCE_WATCHDOG
 	/* Watchdog related data, used by the framework */
 	struct list_head wd_list;
 	cycle_t wd_last;

Re: [PATCH] time : SMP friendly alignment of struct clocksource

[john stultz]

On Tue, 2007-03-20 at 11:09 +0100, Eric Dumazet wrote:
> struct clocksource is a critical data structure.
> 
> Most of its fields are read only, some of them are heavily modified at each timer interrupt.
> 
> It makes sense to separate those fields and make sure they all share one cache line, or at least the minimum for machines with small cache lines.
> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>


Sounds fine to me. Can you actually observe a difference?

Acked-by: John Stultz <johnstul@us.ibm.com>



> --- linux-2.6.21-rc4-mm1/include/linux/clocksource.h
> +++ linux-2.6.21-rc4-mm1-ed/include/linux/clocksource.h
> @@ -49,25 +49,35 @@ struct clocksource;
>   * @flags:		flags describing special properties
>   * @vread:		vsyscall based read
>   * @cycle_interval:	Used internally by timekeeping core, please ignore.
>   * @xtime_interval:	Used internally by timekeeping core, please ignore.
>   */
>  struct clocksource {
> +	/*
> +	 * First part of structure is read mostly
> +	 */
>  	char *name;
>  	struct list_head list;
>  	int rating;
>  	cycle_t (*read)(void);
>  	cycle_t mask;
>  	u32 mult;
>  	u32 shift;
>  	unsigned long flags;
>  	cycle_t (*vread)(void);
> 
>  	/* timekeeping specific data, ignore */
> -	cycle_t cycle_last, cycle_interval;
> -	u64 xtime_nsec, xtime_interval;
> +	cycle_t cycle_interval;
> +	u64	xtime_interval;
> +	/*
> +	 * Second part is written at each timer interrupt
> +	 * Keep it in a different cache line to dirty no
> +	 * more than one cache line.
> +	 */
> +	cycle_t cycle_last ____cacheline_aligned_in_smp;
> +	u64 xtime_nsec;
>  	s64 error;
> 
>  #ifdef CONFIG_CLOCKSOURCE_WATCHDOG
>  	/* Watchdog related data, used by the framework */
>  	struct list_head wd_list;
>  	cycle_t wd_last;

Re: [PATCH] time : SMP friendly alignment of struct clocksource

[Daniel Walker]

On Tue, 2007-03-20 at 10:58 -0700, john stultz wrote:

> >  	/* timekeeping specific data, ignore */
> > -	cycle_t cycle_last, cycle_interval;
> > -	u64 xtime_nsec, xtime_interval;
> > +	cycle_t cycle_interval;
> > +	u64	xtime_interval;
> > +	/*
> > +	 * Second part is written at each timer interrupt
> > +	 * Keep it in a different cache line to dirty no
> > +	 * more than one cache line.
> > +	 */
> > +	cycle_t cycle_last ____cacheline_aligned_in_smp;
> > +	u64 xtime_nsec;
> >  	s64 error;

What was the motivation for having these (cycle_last for example) in the
clocksource structure. I recall them being added in there at one point,
but I'm not sure why.. Specifically the ones that are updated often.

Daniel

Re: [PATCH] time : SMP friendly alignment of struct clocksource

[john stultz]

On Tue, 2007-03-20 at 11:04 -0700, Daniel Walker wrote:
> On Tue, 2007-03-20 at 10:58 -0700, john stultz wrote:
> 
> > >  	/* timekeeping specific data, ignore */
> > > -	cycle_t cycle_last, cycle_interval;
> > > -	u64 xtime_nsec, xtime_interval;
> > > +	cycle_t cycle_interval;
> > > +	u64	xtime_interval;
> > > +	/*
> > > +	 * Second part is written at each timer interrupt
> > > +	 * Keep it in a different cache line to dirty no
> > > +	 * more than one cache line.
> > > +	 */
> > > +	cycle_t cycle_last ____cacheline_aligned_in_smp;
> > > +	u64 xtime_nsec;
> > >  	s64 error;
> 
> What was the motivation for having these (cycle_last for example) in the
> clocksource structure. I recall them being added in there at one point,
> but I'm not sure why.. Specifically the ones that are updated often.

Roman claimed it produced better code, and the values are functionally
clocksource specific (meaning if the clocksource changed, and they were
global, you would have to reset them anyway).

I think logically those values are more timekeeping oriented then
clocksource oriented, but one could argue that since we manipulate
clocksource.mult in the timekeeping's NTP code, mult is timekeeping
specific as well.

If you really wanted a clean design, you'd probably have to keep all of
those values as a timekeeping_clocksource struct, and then have smaller
clocksource structs (name, mult, shift, read) that is filled in by the
clocksource driver and copied over on clocksource changes. That way the
timekeeping code wouldn't manipulate the clocksource structure directly.

But really, it seems like work for little to no reward (plus opens the
issue of "but i want a ntp frequency adjusted sched_clock!" which would
not be possible).

-john