use cycle_t instead of u64 in struct time_interpolator

RFC PATCH In review Helge Deller 5 participants 15 messages Jan 2, 2007 – Jan 3, 2007 Timekeeping, clocksource core, NTP, alarmtimer

lore

Share & tools

lore

Helge Deller

author

2007-01-02 21:34 UTC #0 lore raw

RFC PATCH 1/1 use cycle_t instead of u64 in struct time_interpolator

The 32bit and 64bit PARISC Linux kernels suffers from the problem, that the gettimeofday() call sometimes returns non-monotonic times.
The easiest way to fix this, is to drop the PARISC-specific implementation and switch over to the generic TIME_INTERPOLATION framework.
But in order to make it even compile on 32bit PARISC, the patch below which touches the generic Linux code, is mandatory.
More information and the full patch with the parisc-specific changes is included in this thread: http://lists.parisc-linux.org/pipermail/parisc-linux/2006-December/031003.html

As far as I could see, this patch does not change anything for the existing architectures which use this framework (IA64 and SPARC64), since "cycles_t" is defined there as unsigned 64bit-integer anyway (which then makes this patch a no-change for them).

Ok, not Ok ?

Regards, Helge

Signed-off-by Helge Deller <[email protected]>

include/linux/timex.h +2 -2

index db501dc..9a24e50 100644
@@ -255,10 +255,10 @@ struct time_interpolator {
 	u8 jitter;			/* if set compensate for fluctuations */
 	u32 nsec_per_cyc;		/* set by register_time_interpolator() */
 	void *addr;			/* address of counter or function */
-	u64 mask;			/* mask the valid bits of the counter */
+	cycles_t mask;			/* mask the valid bits of the counter */
 	unsigned long offset;		/* nsec offset at last update of interpolator */
 	u64 last_counter;		/* counter value in units of the counter at last update */
-	u64 last_cycle;			/* Last timer value if TIME_SOURCE_JITTER is set */
+	cycles_t last_cycle;		/* Last timer value if TIME_SOURCE_JITTER is set */
 	u64 frequency;			/* frequency in counts/second */
 	long drift;			/* drift in parts-per-million (or -1) */
 	unsigned long skips;		/* skips forward */

kernel/timer.c +3 -3

index c2a8ccf..d38801a 100644
@@ -1624,7 +1624,7 @@ struct time_interpolator *time_interpola
 static struct time_interpolator *time_interpolator_list __read_mostly;
 static DEFINE_SPINLOCK(time_interpolator_lock);
 
-static inline u64 time_interpolator_get_cycles(unsigned int src)
+static inline cycles_t time_interpolator_get_cycles(unsigned int src)
 {
 	unsigned long (*x)(void);

@@ -1650,8 +1650,8 @@ static inline u64 time_interpolator_get_
 
 	if (time_interpolator->jitter)
 	{
-		u64 lcycle;
-		u64 now;
+		cycles_t lcycle;
+		cycles_t now;
 
 		do {
 			lcycle = time_interpolator->last_cycle;

Matthew Wilcox

2007-01-02 21:44 UTC #1 lore raw

Quoted 1 line · Helge Deller

Ok, not Ok ?

Um, this is still doing cmpxchg() with insufficient locking. So, not OK.

Matthew Wilcox

2007-01-02 21:44 UTC #2 lore raw

In reply to Helge Deller

Quoted 1 line · Helge Deller

Ok, not Ok ?

Um, this is still doing cmpxchg() with insufficient locking. So, not OK.

John David Anglin

2007-01-03 00:02 UTC #3 lore raw

Re: [parisc-linux] [RFC][PATCH] use cycle_t instead of u64 in struct

In reply to Helge Deller

Quoted 1 line

The 32bit and 64bit PARISC Linux kernels suffers from the problem, that the gettimeofday() call sometimes returns non-monotonic times.

This certainly needs to be fixed. I see stuff like this from ping:

64 bytes from 132.246.100.193: icmp_seq=19 ttl=255 time=0.4 ms 64 bytes from 132.246.100.193: icmp_seq=20 ttl=255 time=429496729.5 ms

tar also occasionally prints warning about times. This is with a 32bit kernel.

Dave

Signature · 2 lines

J. David Anglin                                  [email protected]
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

John David Anglin

2007-01-03 00:24 UTC #4 lore raw

Re: [parisc-linux] [RFC][PATCH] use cycle_t instead of u64 in struct

In reply to Helge Deller

Quoted 1 line

The 32bit and 64bit PARISC Linux kernels suffers from the problem, that the gettimeofday() call sometimes returns non-monotonic times.

This certainly needs to be fixed. I see stuff like this from ping:

64 bytes from 132.246.100.193: icmp_seq=19 ttl=255 time=0.4 ms 64 bytes from 132.246.100.193: icmp_seq=20 ttl=255 time=429496729.5 ms

tar also occasionally prints warning about times. This is with a 32bit kernel.

Dave

Signature · 2 lines

J. David Anglin                                  [email protected]
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Christoph Lameter

2007-01-03 17:46 UTC #5 lore raw

In reply to Helge Deller

Quoted 4 lines · Helge Deller

As far as I could see, this patch does not change anything for the existing architectures which use this framework (IA64 and SPARC64), since "cycles_t" is defined there as unsigned 64bit-integer anyway (which then makes this patch a no-change for them).

The 64bit nature of some entities was so far necessary to get the proper accuracy of interpolation. Maybe it can be made to work with 32 bit entities. The macro GET_TI_SECS must work correctly and the less bits are specified in shift the less self-tuning accuracy you will get.

Christoph Lameter

2007-01-03 17:46 UTC #6 lore raw

In reply to Helge Deller

Quoted 4 lines · Helge Deller

As far as I could see, this patch does not change anything for the existing architectures which use this framework (IA64 and SPARC64), since "cycles_t" is defined there as unsigned 64bit-integer anyway (which then makes this patch a no-change for them).

Helge Deller

author

2007-01-03 18:36 UTC #7 lore raw

Quoted 11 lines · Christoph Lameter

Quoted 4 lines · Helge Deller
As far as I could see, this patch does not change anything for the existing architectures which use this framework (IA64 and SPARC64), since "cycles_t" is defined there as unsigned 64bit-integer anyway (which then makes this patch a no-change for them).
The 64bit nature of some entities was so far necessary to get the proper accuracy of interpolation. Maybe it can be made to work with 32 bit entities. The macro GET_TI_SECS must work correctly and the less bits are specified in shift the less self-tuning accuracy you will get.

Yes, it was easily possible to make it 32bit-ready without loosing the accuracy.

Nevertheless, in the meantime John Stultz pointed me to the CONFIG_GENERIC_TIME framework, and I implemented it that way:
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=b6de83b58b8b07f057deacdef8a95b6c32d1c4e6
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=f70a979c843e4610edfb2a316648fe8ae8718f69

Thus please ignore my original patch proposal. It's not needed any more...

Thanks,
Helge

_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Helge Deller

author

2007-01-03 18:36 UTC #8 lore raw

In reply to Christoph Lameter

Quoted 11 lines · Christoph Lameter

Quoted 4 lines · Helge Deller
As far as I could see, this patch does not change anything for the existing architectures which use this framework (IA64 and SPARC64), since "cycles_t" is defined there as unsigned 64bit-integer anyway (which then makes this patch a no-change for them).
The 64bit nature of some entities was so far necessary to get the proper accuracy of interpolation. Maybe it can be made to work with 32 bit entities. The macro GET_TI_SECS must work correctly and the less bits are specified in shift the less self-tuning accuracy you will get.

Yes, it was easily possible to make it 32bit-ready without loosing the accuracy.

Thus please ignore my original patch proposal. It's not needed any more...

Thanks,
Helge

john stultz

2007-01-03 19:17 UTC #9 lore raw

Quoted 13 lines · Helge Deller

Quoted 11 lines · Christoph Lameter
Quoted 4 lines · Helge Deller
As far as I could see, this patch does not change anything for the existing architectures which use this framework (IA64 and SPARC64), since "cycles_t" is defined there as unsigned 64bit-integer anyway (which then makes this patch a no-change for them).
The 64bit nature of some entities was so far necessary to get the proper accuracy of interpolation. Maybe it can be made to work with 32 bit entities. The macro GET_TI_SECS must work correctly and the less bits are specified in shift the less self-tuning accuracy you will get.

Quoted 4 lines

Yes, it was easily possible to make it 32bit-ready without loosing the accuracy.
Nevertheless, in the meantime John Stultz pointed me to the CONFIG_GENERIC_TIME framework, and I implemented it that way:
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=b6de83b58b8b07f057deacdef8a95b6c32d1c4e6

This looks pretty good, although setting the rating to 200 for a clocksource you don't want to use seems a bit high (there's a rough rating scale in clocksource.h). Zero is probably what you want to use there.

Quoted 1 line

http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=f70a979c843e4610edfb2a316648fe8ae8718f69

This looks to be correct, although as the clocksource infrastructure evolves it looks like we'll be removing the update_callback code in the future. So this is fine for now, but will probably need a reevaluation at some point.

Also to avoid jumping between clocksources, I'd keep the initial disqualification that occurs before you register the clocksource (otherwise it will be used for one tick, then be disqualified and you're back to jiffies).

thanks
-john

john stultz

2007-01-03 19:18 UTC #10 lore raw

In reply to Helge Deller

Quoted 13 lines · Helge Deller

Quoted 11 lines · Christoph Lameter
Quoted 4 lines · Helge Deller
As far as I could see, this patch does not change anything for the existing architectures which use this framework (IA64 and SPARC64), since "cycles_t" is defined there as unsigned 64bit-integer anyway (which then makes this patch a no-change for them).
The 64bit nature of some entities was so far necessary to get the proper accuracy of interpolation. Maybe it can be made to work with 32 bit entities. The macro GET_TI_SECS must work correctly and the less bits are specified in shift the less self-tuning accuracy you will get.

Quoted 4 lines

Yes, it was easily possible to make it 32bit-ready without loosing the accuracy.
Nevertheless, in the meantime John Stultz pointed me to the CONFIG_GENERIC_TIME framework, and I implemented it that way:
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=b6de83b58b8b07f057deacdef8a95b6c32d1c4e6

Quoted 1 line

http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=f70a979c843e4610edfb2a316648fe8ae8718f69

thanks
-john

Helge Deller

author

2007-01-03 20:23 UTC #11 lore raw

Quoted 31 lines · john stultz

Quoted 13 lines · Helge Deller
Quoted 11 lines · Christoph Lameter
Quoted 4 lines · Helge Deller
As far as I could see, this patch does not change anything for the existing architectures which use this framework (IA64 and SPARC64), since "cycles_t" is defined there as unsigned 64bit-integer anyway (which then makes this patch a no-change for them).
The 64bit nature of some entities was so far necessary to get the proper accuracy of interpolation. Maybe it can be made to work with 32 bit entities. The macro GET_TI_SECS must work correctly and the less bits are specified in shift the less self-tuning accuracy you will get.
Quoted 4 lines
Yes, it was easily possible to make it 32bit-ready without loosing the accuracy.
Nevertheless, in the meantime John Stultz pointed me to the CONFIG_GENERIC_TIME framework, and I implemented it that way:
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=b6de83b58b8b07f057deacdef8a95b6c32d1c4e6
This looks pretty good, although setting the rating to 200 for a clocksource you don't want to use seems a bit high (there's a rough rating scale in clocksource.h). Zero is probably what you want to use there.
Quoted 1 line
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=f70a979c843e4610edfb2a316648fe8ae8718f69
This looks to be correct, although as the clocksource infrastructure evolves it looks like we'll be removing the update_callback code in the future. So this is fine for now, but will probably need a reevaluation at some point.

Quoted 4 lines

Also to avoid jumping between clocksources, I'd keep the initial disqualification that occurs before you register the clocksource (otherwise it will be used for one tick, then be disqualified and you're back to jiffies).

That's true, but James Bottomley noticed, that time_init() is called before we've done system inventory (which detects if we have a SMP box with multiple CPUs), so num_online_cpus() would always be one. The update_callback function enables us to switch back to jiffies if we actually run on a SMP box.

That said, it would be nice to keep the update_callback() functionality or provide another nice solution around that problem...

Helge
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Helge Deller

author

2007-01-03 20:23 UTC #12 lore raw

In reply to john stultz

Quoted 31 lines · john stultz

Quoted 13 lines · Helge Deller
Quoted 11 lines · Christoph Lameter
Quoted 4 lines · Helge Deller
As far as I could see, this patch does not change anything for the existing architectures which use this framework (IA64 and SPARC64), since "cycles_t" is defined there as unsigned 64bit-integer anyway (which then makes this patch a no-change for them).
The 64bit nature of some entities was so far necessary to get the proper accuracy of interpolation. Maybe it can be made to work with 32 bit entities. The macro GET_TI_SECS must work correctly and the less bits are specified in shift the less self-tuning accuracy you will get.
Quoted 4 lines
Yes, it was easily possible to make it 32bit-ready without loosing the accuracy.
Nevertheless, in the meantime John Stultz pointed me to the CONFIG_GENERIC_TIME framework, and I implemented it that way:
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=b6de83b58b8b07f057deacdef8a95b6c32d1c4e6
This looks pretty good, although setting the rating to 200 for a clocksource you don't want to use seems a bit high (there's a rough rating scale in clocksource.h). Zero is probably what you want to use there.
Quoted 1 line
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=f70a979c843e4610edfb2a316648fe8ae8718f69
This looks to be correct, although as the clocksource infrastructure evolves it looks like we'll be removing the update_callback code in the future. So this is fine for now, but will probably need a reevaluation at some point.

Quoted 4 lines

Also to avoid jumping between clocksources, I'd keep the initial disqualification that occurs before you register the clocksource (otherwise it will be used for one tick, then be disqualified and you're back to jiffies).

That said, it would be nice to keep the update_callback() functionality or provide another nice solution around that problem...

Helge

john stultz

2007-01-03 20:42 UTC #13 lore raw

Quoted 37 lines · Helge Deller

Quoted 31 lines · john stultz
Quoted 13 lines · Helge Deller
Quoted 11 lines · Christoph Lameter
Quoted 4 lines · Helge Deller
As far as I could see, this patch does not change anything for the existing architectures which use this framework (IA64 and SPARC64), since "cycles_t" is defined there as unsigned 64bit-integer anyway (which then makes this patch a no-change for them).
The 64bit nature of some entities was so far necessary to get the proper accuracy of interpolation. Maybe it can be made to work with 32 bit entities. The macro GET_TI_SECS must work correctly and the less bits are specified in shift the less self-tuning accuracy you will get.
Quoted 4 lines
Yes, it was easily possible to make it 32bit-ready without loosing the accuracy.
Nevertheless, in the meantime John Stultz pointed me to the CONFIG_GENERIC_TIME framework, and I implemented it that way:
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=b6de83b58b8b07f057deacdef8a95b6c32d1c4e6
This looks pretty good, although setting the rating to 200 for a clocksource you don't want to use seems a bit high (there's a rough rating scale in clocksource.h). Zero is probably what you want to use there.
Quoted 1 line
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=f70a979c843e4610edfb2a316648fe8ae8718f69
This looks to be correct, although as the clocksource infrastructure evolves it looks like we'll be removing the update_callback code in the future. So this is fine for now, but will probably need a reevaluation at some point.
Quoted 4 lines
Also to avoid jumping between clocksources, I'd keep the initial disqualification that occurs before you register the clocksource (otherwise it will be used for one tick, then be disqualified and you're back to jiffies).

Quoted 4 lines

That's true, but James Bottomley noticed, that time_init() is called before we've done system inventory (which detects if we have a SMP box with multiple CPUs), so num_online_cpus() would always be one. The update_callback function enables us to switch back to jiffies if we actually run on a SMP box.

Ah, yes, right now most clocksources currently register themselves at module_init time to avoid this sort of thing. Although we might move that to be a bit earlier, since there is some need for high precision timeofday earlier at bootup.

Quoted 2 lines

That said, it would be nice to keep the update_callback() functionality or provide another nice solution around that problem...

Yea. There will be an alternate solution, hopefully something more direct that just removes the clocksource and moves on, rather then polling each tick for change.

thanks
-john

john stultz

2007-01-03 20:42 UTC #14 lore raw

In reply to Helge Deller

Quoted 37 lines · Helge Deller

Quoted 31 lines · john stultz
Quoted 13 lines · Helge Deller
Quoted 11 lines · Christoph Lameter
Quoted 4 lines · Helge Deller
As far as I could see, this patch does not change anything for the existing architectures which use this framework (IA64 and SPARC64), since "cycles_t" is defined there as unsigned 64bit-integer anyway (which then makes this patch a no-change for them).
The 64bit nature of some entities was so far necessary to get the proper accuracy of interpolation. Maybe it can be made to work with 32 bit entities. The macro GET_TI_SECS must work correctly and the less bits are specified in shift the less self-tuning accuracy you will get.
Quoted 4 lines
Yes, it was easily possible to make it 32bit-ready without loosing the accuracy.
Nevertheless, in the meantime John Stultz pointed me to the CONFIG_GENERIC_TIME framework, and I implemented it that way:
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=b6de83b58b8b07f057deacdef8a95b6c32d1c4e6
This looks pretty good, although setting the rating to 200 for a clocksource you don't want to use seems a bit high (there's a rough rating scale in clocksource.h). Zero is probably what you want to use there.
Quoted 1 line
http://git.parisc-linux.org/?p=linux-2.6.git;a=commit;h=f70a979c843e4610edfb2a316648fe8ae8718f69
This looks to be correct, although as the clocksource infrastructure evolves it looks like we'll be removing the update_callback code in the future. So this is fine for now, but will probably need a reevaluation at some point.
Quoted 4 lines
Also to avoid jumping between clocksources, I'd keep the initial disqualification that occurs before you register the clocksource (otherwise it will be used for one tick, then be disqualified and you're back to jiffies).

Quoted 4 lines

That's true, but James Bottomley noticed, that time_init() is called before we've done system inventory (which detects if we have a SMP box with multiple CPUs), so num_online_cpus() would always be one. The update_callback function enables us to switch back to jiffies if we actually run on a SMP box.

Quoted 2 lines

That said, it would be nice to keep the update_callback() functionality or provide another nice solution around that problem...

Yea. There will be an alternate solution, hopefully something more direct that just removes the clocksource and moves on, rather then polling each tick for change.

thanks
-john