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]>
diff --git a/include/linux/timex.h b/include/linux/timex.h
index db501dc..9a24e50 100644
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -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 */
diff --git a/kernel/timer.c b/kernel/timer.c
index c2a8ccf..d38801a 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -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;
On Tue, Jan 02, 2007 at 10:33:25PM +0100, Helge Deller wrote:
> Ok, not Ok ?
Um, this is still doing cmpxchg() with insufficient locking. So, not
OK.
> 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
--
J. David Anglin [email protected]
National Research Council of Canada (613) 990-0752 (FAX: 952-6602)
On Tue, 2 Jan 2007, Helge Deller wrote:
> 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.
On Wed Jan 3 2007, Christoph Lameter wrote:
> On Tue, 2 Jan 2007, Helge Deller wrote:
>
> > 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
On Wed, 2007-01-03 at 19:36 +0100, Helge Deller wrote:
> On Wed Jan 3 2007, Christoph Lameter wrote:
> > On Tue, 2 Jan 2007, Helge Deller wrote:
> >
> > > 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
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.
> 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
On Wed Jan 3 2007, john stultz wrote:
> On Wed, 2007-01-03 at 19:36 +0100, Helge Deller wrote:
> > On Wed Jan 3 2007, Christoph Lameter wrote:
> > > On Tue, 2 Jan 2007, Helge Deller wrote:
> > >
> > > > 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
>
> 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.
>
> > 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).
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
On Wed, 2007-01-03 at 21:23 +0100, Helge Deller wrote:
> On Wed Jan 3 2007, john stultz wrote:
> > On Wed, 2007-01-03 at 19:36 +0100, Helge Deller wrote:
> > > On Wed Jan 3 2007, Christoph Lameter wrote:
> > > > On Tue, 2 Jan 2007, Helge Deller wrote:
> > > >
> > > > > 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
> >
> > 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.
> >
> > > 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).
>
> 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.
> 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