Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Mikael Pettersson]

On Sun, 09 Jul 2006 14:20:56 -0700, john stultz wrote:
>> I've traced the cause of this problem to the i386 time-keeping
>> changes in kernel 2.6.17-git11. What happens is that:
>> - The kernel autoselects TSC as my clocksource, which is
>>   reasonable since it's a PentiumII. 2.6.17 also chose the TSC.
>> - Immediately after APM resumes (arch/i386/kernel/apm.c line
>>   1231 in 2.6.18-rc1) there is an interrupt from the PIT,
>>   which takes us to kernel/timer.c:update_wall_time().
>> - update_wall_time() does a clocksource_read() and computes
>>   the offset from the previous read. However, the TSC was
>>   reset by HW or BIOS during the APM suspend/resume cycle and
>>   is now smaller than it was at the prevous read. On my machine,
>>   the offset is 0xffffffd598e0a566 at this point, which appears
>>   to throw update_wall_time() into a very very long loop.
>
>Huh. It seems you're getting an interrupt before timekeeping_resume()
>runs (which resets cycle_last). I'll look over the code and see if I can
>sort out why it works w/ ACPI suspend, but not APM, or if the
>resume/interrupt-enablement bit is just racy in general.

I forgot to mention this, but I had a debug printk() in apm.c
which showed that irqs_disabled() == 0 at the point when APM
resumes the kernel.

/Mikael

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Pavel Machek]

On Mon 2006-07-10 01:52:35, Mikael Pettersson wrote:
> On Sun, 09 Jul 2006 14:20:56 -0700, john stultz wrote:
> >> I've traced the cause of this problem to the i386 time-keeping
> >> changes in kernel 2.6.17-git11. What happens is that:
> >> - The kernel autoselects TSC as my clocksource, which is
> >>   reasonable since it's a PentiumII. 2.6.17 also chose the TSC.
> >> - Immediately after APM resumes (arch/i386/kernel/apm.c line
> >>   1231 in 2.6.18-rc1) there is an interrupt from the PIT,
> >>   which takes us to kernel/timer.c:update_wall_time().
> >> - update_wall_time() does a clocksource_read() and computes
> >>   the offset from the previous read. However, the TSC was
> >>   reset by HW or BIOS during the APM suspend/resume cycle and
> >>   is now smaller than it was at the prevous read. On my machine,
> >>   the offset is 0xffffffd598e0a566 at this point, which appears
> >>   to throw update_wall_time() into a very very long loop.
> >
> >Huh. It seems you're getting an interrupt before timekeeping_resume()
> >runs (which resets cycle_last). I'll look over the code and see if I can
> >sort out why it works w/ ACPI suspend, but not APM, or if the
> >resume/interrupt-enablement bit is just racy in general.
> 
> I forgot to mention this, but I had a debug printk() in apm.c
> which showed that irqs_disabled() == 0 at the point when APM
> resumes the kernel.

/*
 * These are the actual BIOS calls.  Depending on APM_ZERO_SEGS and
 * apm_info.allow_ints, we are being really paranoid here!  Not only
 * are interrupts disabled, but all the segment registers (except SS)
 * are saved and zeroed this means that if the BIOS tries to reference
 * any data without explicitly loading the segment registers, the
kernel
 * will fault immediately rather than have some unforeseen
circumstances
 * for the rest of the kernel.  And it will be very obvious!  :-)
Doing
 * this depends on CS referring to the same physical memory as DS so
that
 * DS can be zeroed before the call. Unfortunately, we can't do
anything
 * about the stack segment/pointer.  Also, we tell the compiler that
 * everything could change.
 *
 * Also, we KNOW that for the non error case of apm_bios_call, there
 * is no useful data returned in the low order 8 bits of eax.
 */
#define APM_DO_CLI      \
        if (apm_info.allow_ints) \
                local_irq_enable(); \
        else \
                local_irq_disable();

...and I think allow_ints was invented because of thinkpads.. So
AFAICT you can't rely on interrupts being off and you can't rely on
interrupts being on.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[john stultz]

On Mon, 2006-07-10 at 01:52 +0200, Mikael Pettersson wrote:
> On Sun, 09 Jul 2006 14:20:56 -0700, john stultz wrote:
> >> I've traced the cause of this problem to the i386 time-keeping
> >> changes in kernel 2.6.17-git11. What happens is that:
> >> - The kernel autoselects TSC as my clocksource, which is
> >>   reasonable since it's a PentiumII. 2.6.17 also chose the TSC.
> >> - Immediately after APM resumes (arch/i386/kernel/apm.c line
> >>   1231 in 2.6.18-rc1) there is an interrupt from the PIT,
> >>   which takes us to kernel/timer.c:update_wall_time().
> >> - update_wall_time() does a clocksource_read() and computes
> >>   the offset from the previous read. However, the TSC was
> >>   reset by HW or BIOS during the APM suspend/resume cycle and
> >>   is now smaller than it was at the prevous read. On my machine,
> >>   the offset is 0xffffffd598e0a566 at this point, which appears
> >>   to throw update_wall_time() into a very very long loop.
> >
> >Huh. It seems you're getting an interrupt before timekeeping_resume()
> >runs (which resets cycle_last). I'll look over the code and see if I can
> >sort out why it works w/ ACPI suspend, but not APM, or if the
> >resume/interrupt-enablement bit is just racy in general.
> 
> I forgot to mention this, but I had a debug printk() in apm.c
> which showed that irqs_disabled() == 0 at the point when APM
> resumes the kernel.

So it seems possible that the timer tick will be enabled before the
timekeeping resume code runs. I'm not sure why this isn't seen w/ ACPI
suspend/resume, as I think they're using the same
sysdev_class .suspend/.resume bits. 

Anyway, I think this patch should fix it (I've only compile tested it,
as I don't have my laptop on me right now). Would you mind giving it a
try? 

thanks
-john

diff --git a/kernel/timer.c b/kernel/timer.c
index 396a3c0..afaa594 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -966,7 +966,7 @@ void __init timekeeping_init(void)
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 }
 
-
+static int timekeeping_suspended;
 /*
  * timekeeping_resume - Resumes the generic timekeeping subsystem.
  * @dev:	unused
@@ -982,6 +982,18 @@ static int timekeeping_resume(struct sys
 	write_seqlock_irqsave(&xtime_lock, flags);
 	/* restart the last cycle value */
 	clock->cycle_last = clocksource_read(clock);
+	clock->error = 0;
+	timekeeping_suspended = 0;
+	write_sequnlock_irqrestore(&xtime_lock, flags);
+	return 0;
+}
+
+static int timekeeping_suspend(struct sys_device *dev, pm_message_t state)
+{
+	unsigned long flags;
+
+	write_seqlock_irqsave(&xtime_lock, flags);
+	timekeeping_suspended = 1;
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 	return 0;
 }
@@ -989,6 +1001,7 @@ static int timekeeping_resume(struct sys
 /* sysfs resume/suspend bits for timekeeping */
 static struct sysdev_class timekeeping_sysclass = {
 	.resume		= timekeeping_resume,
+	.suspend	= timekeeping_suspend,
 	set_kset_name("timekeeping"),
 };
 
@@ -1090,14 +1103,17 @@ static void clocksource_adjust(struct cl
 static void update_wall_time(void)
 {
 	cycle_t offset;
-
-	clock->xtime_nsec += (s64)xtime.tv_nsec << clock->shift;
+	
+	/* avoid timekeeping before we're fully resumed */
+	if (unlikely(timekeeping_suspended))
+		return;
 
 #ifdef CONFIG_GENERIC_TIME
 	offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
 #else
 	offset = clock->cycle_interval;
 #endif
+	clock->xtime_nsec += (s64)xtime.tv_nsec << clock->shift;
 
 	/* normally this loop will run just once, however in the
 	 * case of lost or late ticks, it will accumulate correctly.

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Pavel Machek]

Hi!

> > >> I've traced the cause of this problem to the i386 time-keeping
> > >> changes in kernel 2.6.17-git11. What happens is that:
> > >> - The kernel autoselects TSC as my clocksource, which is
> > >>   reasonable since it's a PentiumII. 2.6.17 also chose the TSC.
> > >> - Immediately after APM resumes (arch/i386/kernel/apm.c line
> > >>   1231 in 2.6.18-rc1) there is an interrupt from the PIT,
> > >>   which takes us to kernel/timer.c:update_wall_time().
> > >> - update_wall_time() does a clocksource_read() and computes
> > >>   the offset from the previous read. However, the TSC was
> > >>   reset by HW or BIOS during the APM suspend/resume cycle and
> > >>   is now smaller than it was at the prevous read. On my machine,
> > >>   the offset is 0xffffffd598e0a566 at this point, which appears
> > >>   to throw update_wall_time() into a very very long loop.
> > >
> > >Huh. It seems you're getting an interrupt before timekeeping_resume()
> > >runs (which resets cycle_last). I'll look over the code and see if I can
> > >sort out why it works w/ ACPI suspend, but not APM, or if the
> > >resume/interrupt-enablement bit is just racy in general.
> > 
> > I forgot to mention this, but I had a debug printk() in apm.c
> > which showed that irqs_disabled() == 0 at the point when APM
> > resumes the kernel.
> 
> So it seems possible that the timer tick will be enabled before the
> timekeeping resume code runs. I'm not sure why this isn't seen w/ ACPI
> suspend/resume, as I think they're using the same
> sysdev_class .suspend/.resume bits. 

ACPI actually keeps interrupts disabled, always.

APM can only keep interrupts disabled on non-IBM machines, presumably
due to BIOS problems.

Could we get some sanity check into looping function? If timesource
goes backwards, at least somehow reporting it would be nice...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[john stultz]

On Mon, 2006-07-10 at 20:08 +0200, Pavel Machek wrote:
> Hi!
> 
> > > >> I've traced the cause of this problem to the i386 time-keeping
> > > >> changes in kernel 2.6.17-git11. What happens is that:
> > > >> - The kernel autoselects TSC as my clocksource, which is
> > > >>   reasonable since it's a PentiumII. 2.6.17 also chose the TSC.
> > > >> - Immediately after APM resumes (arch/i386/kernel/apm.c line
> > > >>   1231 in 2.6.18-rc1) there is an interrupt from the PIT,
> > > >>   which takes us to kernel/timer.c:update_wall_time().
> > > >> - update_wall_time() does a clocksource_read() and computes
> > > >>   the offset from the previous read. However, the TSC was
> > > >>   reset by HW or BIOS during the APM suspend/resume cycle and
> > > >>   is now smaller than it was at the prevous read. On my machine,
> > > >>   the offset is 0xffffffd598e0a566 at this point, which appears
> > > >>   to throw update_wall_time() into a very very long loop.
> > > >
> > > >Huh. It seems you're getting an interrupt before timekeeping_resume()
> > > >runs (which resets cycle_last). I'll look over the code and see if I can
> > > >sort out why it works w/ ACPI suspend, but not APM, or if the
> > > >resume/interrupt-enablement bit is just racy in general.
> > > 
> > > I forgot to mention this, but I had a debug printk() in apm.c
> > > which showed that irqs_disabled() == 0 at the point when APM
> > > resumes the kernel.
> > 
> > So it seems possible that the timer tick will be enabled before the
> > timekeeping resume code runs. I'm not sure why this isn't seen w/ ACPI
> > suspend/resume, as I think they're using the same
> > sysdev_class .suspend/.resume bits. 
> 
> ACPI actually keeps interrupts disabled, always.
> 
> APM can only keep interrupts disabled on non-IBM machines, presumably
> due to BIOS problems.
> 
> Could we get some sanity check into looping function? If timesource
> goes backwards, at least somehow reporting it would be nice...

Yep, I'm working on a debug patch (similar to the paranoid timekeeping
debugging option in earlier versions of the TOD patch) that will spit
out warnings when we see unusual behavior: large numbers of lost ticks,
timer ticks arriving too early, settimeofday being call, etc.

thanks
-john

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Roman Zippel]

Hi,

On Mon, 10 Jul 2006, Pavel Machek wrote:

> APM can only keep interrupts disabled on non-IBM machines, presumably
> due to BIOS problems.

Is it possible to disable the timer interrupt before suspend and just 
reinit the timer afterwards?

bye, Roman

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[john stultz]

On Tue, 2006-07-11 at 00:37 +0200, Roman Zippel wrote:
> Hi,
> 
> On Mon, 10 Jul 2006, Pavel Machek wrote:
> 
> > APM can only keep interrupts disabled on non-IBM machines, presumably
> > due to BIOS problems.
> 
> Is it possible to disable the timer interrupt before suspend and just 
> reinit the timer afterwards?

The timer interrupt is re-enabled, via the timer_sysclass::resume hook,
while the timekeeping code is re-enabled via the
timekeeping_sysclass::resume hook. The issue being that I'm not sure
there's a defined way to specify the .resume calling order.

The timekeeping_suspended flag is a bit heavy handed, but I think it
might be the safest bet (assuming Mikael finds it works for him).

thanks
-john

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Roman Zippel]

Hi,

On Mon, 10 Jul 2006, john stultz wrote:

> > > APM can only keep interrupts disabled on non-IBM machines, presumably
> > > due to BIOS problems.
> > 
> > Is it possible to disable the timer interrupt before suspend and just 
> > reinit the timer afterwards?
> 
> The timer interrupt is re-enabled, via the timer_sysclass::resume hook,
> while the timekeeping code is re-enabled via the
> timekeeping_sysclass::resume hook. The issue being that I'm not sure
> there's a defined way to specify the .resume calling order.
> 
> The timekeeping_suspended flag is a bit heavy handed, but I think it
> might be the safest bet (assuming Mikael finds it works for him).

As temporary measure it's ok, but please add a comment, that it's there 
because of broken suspend/resume ordering.
That's another reason why I think that keeping interrupt handling and 
timekeeping separate is illusionary.

bye, Roman

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Thomas Gleixner]

Roman,

On Tue, 2006-07-11 at 00:59 +0200, Roman Zippel wrote:
> > The timer interrupt is re-enabled, via the timer_sysclass::resume hook,
> > while the timekeeping code is re-enabled via the
> > timekeeping_sysclass::resume hook. The issue being that I'm not sure
> > there's a defined way to specify the .resume calling order.
> > 
> > The timekeeping_suspended flag is a bit heavy handed, but I think it
> > might be the safest bet (assuming Mikael finds it works for him).
> 
> As temporary measure it's ok, but please add a comment, that it's there 
> because of broken suspend/resume ordering.
> That's another reason why I think that keeping interrupt handling and 
> timekeeping separate is illusionary.

It is not illusionary at all and we have to find a way to handle this.

Forcing time keeping to be bound on some interrupt handling is the wrong
design and in the way of tickless systems.

When there is a system where the time source is bound to an interrupt
event then the handling code for that time source has to cope with the
problem instead of enforcing this not generally valid scenario on
everything. We can of course add helpers into the generic part of the
time keeping code to make this easier.

The majority of machines has stand alone time sources and there is no
need to enforce artificial interrupt relations on them.

Please accept that the "tick" is not the holy grail of Linux. We have
already way too much historic tick ballast all over the place, so we
really want to avoid that when we design replacement functionality.

	tglx

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Roman Zippel]

Hi,

On Tue, 11 Jul 2006, Thomas Gleixner wrote:

> > That's another reason why I think that keeping interrupt handling and 
> > timekeeping separate is illusionary.
> 
> It is not illusionary at all and we have to find a way to handle this.
> 
> Forcing time keeping to be bound on some interrupt handling is the wrong
> design and in the way of tickless systems.

So what is the correct design?
Especially for tickless system it's vital for precise timekeeping that the 
timekeeping code knows what the timer interrupt code does.

> When there is a system where the time source is bound to an interrupt
> event then the handling code for that time source has to cope with the
> problem instead of enforcing this not generally valid scenario on
> everything. We can of course add helpers into the generic part of the
> time keeping code to make this easier.

I'm not sure I'm following, could you please illustrate with an example?

> The majority of machines has stand alone time sources and there is no
> need to enforce artificial interrupt relations on them.

What do you mean with "artificial interrupt relations"?

> Please accept that the "tick" is not the holy grail of Linux. We have
> already way too much historic tick ballast all over the place, so we
> really want to avoid that when we design replacement functionality.

What do you mean with the "holy grail" of "tick"?

bye, Roman

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Thomas Gleixner]

Roman,

On Tue, 2006-07-11 at 11:29 +0200, Roman Zippel wrote:
> > It is not illusionary at all and we have to find a way to handle this.
> > 
> > Forcing time keeping to be bound on some interrupt handling is the wrong
> > design and in the way of tickless systems.
> 
> So what is the correct design?
> Especially for tickless system it's vital for precise timekeeping that the 
> timekeeping code knows what the timer interrupt code does.

Err. Why needs the time keeping code to know what the next timer event
will be ? The time keeping code must not care at all about it. And there
is nothing vital at all.

Time keeping code reads from a given source and does the necessary
adjustments when NTP is active. There is no relation to an interrupt
event at all. At the very end it boils down to a linear equation which
is recalculated at the synchronization points.

The timer interrupt itself is not a synchronization point.

At any synchronization point you store the current time as seen by the
time keeping code for reference and calculate the deviation from the
reference time line. Until the next synchronization point you apply the
recalculated correction to the readout and it does not matter at all
whether there are 0, 10 or 1000 timer interrupts between those points
and whether the delta between those interrupts is constant or not.

> > When there is a system where the time source is bound to an interrupt
> > event then the handling code for that time source has to cope with the
> > problem instead of enforcing this not generally valid scenario on
> > everything. We can of course add helpers into the generic part of the
> > time keeping code to make this easier.
> 
> I'm not sure I'm following, could you please illustrate with an example?

An example is time keeping via a periodic interrupt which does not allow
to readout intermediate values. Thats the only case where you want that
the increment of the internal time is as close as possible to the point
where the event happens. This is a property of this particular time
source not of the general time keeping code. If its necessary to have
some infrastructure for such cases in the generic code, I'm not
opposing. 

I'm just opposing the general tight coupling of the time keeping to
timer interrupts.

Vs. the original problem of resume ordering. It simply has to be
structured in a way that the essential fixups are done _before_ anything
uses the values. If this is not the case then we have to fix this
problem instead of proposing that time keeping has to be coupled to
timer interrupts.

> > The majority of machines has stand alone time sources and there is no
> > need to enforce artificial interrupt relations on them.
> 
> What do you mean with "artificial interrupt relations"?

Binding the time source to an interrupt event. There is no need to do so
at all when you have a continous clock source like TSC, pm_timer, PPC
incrementer, ... 

Why should the time keeping code care when the next interrupt goes off ?

> > Please accept that the "tick" is not the holy grail of Linux. We have
> > already way too much historic tick ballast all over the place, so we
> > really want to avoid that when we design replacement functionality.
> 
> What do you mean with the "holy grail" of "tick"?

Well, I have the feeling - maybe I misunderstand you - that your idea of
time keeping is affixed around the tight coupling of time keeping and
timer interrupt which is the tick in the current implementation.

	tglx

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Roman Zippel]

Hi,

On Tue, 11 Jul 2006, Thomas Gleixner wrote:

> > > It is not illusionary at all and we have to find a way to handle this.
> > > 
> > > Forcing time keeping to be bound on some interrupt handling is the wrong
> > > design and in the way of tickless systems.
> > 
> > So what is the correct design?
> > Especially for tickless system it's vital for precise timekeeping that the 
> > timekeeping code knows what the timer interrupt code does.
> 
> Err. Why needs the time keeping code to know what the next timer event
> will be ? The time keeping code must not care at all about it. And there
> is nothing vital at all.
> 
> Time keeping code reads from a given source and does the necessary
> adjustments when NTP is active. There is no relation to an interrupt
> event at all. At the very end it boils down to a linear equation which
> is recalculated at the synchronization points.
> 
> The timer interrupt itself is not a synchronization point.

If the timer interrupt is not a synchronization point, what is?

> At any synchronization point you store the current time as seen by the
> time keeping code for reference and calculate the deviation from the
> reference time line. Until the next synchronization point you apply the
> recalculated correction to the readout and it does not matter at all
> whether there are 0, 10 or 1000 timer interrupts between those points
> and whether the delta between those interrupts is constant or not.

Well, it does matter, otherwise the recent fix woudn't have been needed.
The timekeeping doesn't care much where synchronizations point comes from, 
but it can do a much better job, if it knows when they come.
Without knowing this the timekeeping code has to do extra work and has to 
be either optimistic or assume the worst case, neither is good for precise 
timekeeping, the first causes extra jitter, the latter very slow 
adjustments.

For example what we can do now is to provide per cpu clock sources for 
asynchronous multiprocessing systems, so they can make use of the fast tsc 
access. The problem here is one should target for small jitter and quick 
adjustments to have some margin for the worst cases, but this also means 
they at least need to know about the synchronization points and it would 
be even better to be able insert a few.

> > > When there is a system where the time source is bound to an interrupt
> > > event then the handling code for that time source has to cope with the
> > > problem instead of enforcing this not generally valid scenario on
> > > everything. We can of course add helpers into the generic part of the
> > > time keeping code to make this easier.
> > 
> > I'm not sure I'm following, could you please illustrate with an example?
> 
> An example is time keeping via a periodic interrupt which does not allow
> to readout intermediate values. Thats the only case where you want that
> the increment of the internal time is as close as possible to the point
> where the event happens. This is a property of this particular time
> source not of the general time keeping code. If its necessary to have
> some infrastructure for such cases in the generic code, I'm not
> opposing. 
> 
> I'm just opposing the general tight coupling of the time keeping to
> timer interrupts.

Timekeeping needs control of something, interrupts are the general 
mechanism for asynchronous events. (Unless I'm missing something) your 
clockevent concept is far too complex, simple clocksources only need to 
export a shareable interrupt source, which the timekeeping code can use to 
synchronize.
If by "tight coupling" you mean the current hardcoded timer interrupt I 
agree, but I still don't see that the clockevent stuff is really a better 
solution.

> Vs. the original problem of resume ordering. It simply has to be
> structured in a way that the essential fixups are done _before_ anything
> uses the values. If this is not the case then we have to fix this
> problem instead of proposing that time keeping has to be coupled to
> timer interrupts.
> 
> > > The majority of machines has stand alone time sources and there is no
> > > need to enforce artificial interrupt relations on them.
> > 
> > What do you mean with "artificial interrupt relations"?
> 
> Binding the time source to an interrupt event. There is no need to do so
> at all when you have a continous clock source like TSC, pm_timer, PPC
> incrementer, ... 
> 
> Why should the time keeping code care when the next interrupt goes off ?

_Precise_ timekeeping.

> > > Please accept that the "tick" is not the holy grail of Linux. We have
> > > already way too much historic tick ballast all over the place, so we
> > > really want to avoid that when we design replacement functionality.
> > 
> > What do you mean with the "holy grail" of "tick"?
> 
> Well, I have the feeling - maybe I misunderstand you - that your idea of
> time keeping is affixed around the tight coupling of time keeping and
> timer interrupt which is the tick in the current implementation.

I don't care how coupling is done (well, I do like a good compromise 
between simplicity and performace), but the timekeeping needs to 
synchronize to something.

bye, Roman

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[john stultz]

On Wed, 2006-07-12 at 01:31 +0200, Roman Zippel wrote:
> On Tue, 11 Jul 2006, Thomas Gleixner wrote:
> 
> > > > It is not illusionary at all and we have to find a way to handle this.
> > > > 
> > > > Forcing time keeping to be bound on some interrupt handling is the wrong
> > > > design and in the way of tickless systems.
> > > 
> > > So what is the correct design?
> > > Especially for tickless system it's vital for precise timekeeping that the 
> > > timekeeping code knows what the timer interrupt code does.
> > 
> > Err. Why needs the time keeping code to know what the next timer event
> > will be ? The time keeping code must not care at all about it. And there
> > is nothing vital at all.
> > 
> > Time keeping code reads from a given source and does the necessary
> > adjustments when NTP is active. There is no relation to an interrupt
> > event at all. At the very end it boils down to a linear equation which
> > is recalculated at the synchronization points.
> > 
> > The timer interrupt itself is not a synchronization point.
> 
> If the timer interrupt is not a synchronization point, what is?

Synchronization point seems like a bad term in my mind. I prefer to
think of it as an accumulation point (to avoid clocksource overflows) as
well as a calculation point where we can make careful adjustments to the
clocksource frequency. Please note that these two actions are not
logically linked and could be done separately (although there really
isn't a need to do so).

> > At any synchronization point you store the current time as seen by the
> > time keeping code for reference and calculate the deviation from the
> > reference time line. Until the next synchronization point you apply the
> > recalculated correction to the readout and it does not matter at all
> > whether there are 0, 10 or 1000 timer interrupts between those points
> > and whether the delta between those interrupts is constant or not.
> 
> Well, it does matter, otherwise the recent fix woudn't have been needed.

Assuming you're talking about the suspend fix, I'm not sure I agree.
Making sure the timekeeping code is initialized before we use it does
not, to me at least, illustrate how interrupt frequency and timekeeping
are entwined. That's just an initialization ordering problem, as Thomas
already said.

> The timekeeping doesn't care much where synchronizations point comes from, 
> but it can do a much better job, if it knows when they come.
> Without knowing this the timekeeping code has to do extra work and has to 
> be either optimistic or assume the worst case, neither is good for precise 
> timekeeping, the first causes extra jitter, the latter very slow 
> adjustments.

This I agree with. But first lets bound the issue to make it more clear
for everyone: The conflict lies only with the high-precision clocksource
adjustment code and not so much with the rest of the timekeeping code.

The issue being that the high-precision clocksource adjustment code is
needed because when NTP makes an adjustment, that very precise
adjustment might be finer then the smallest multiplier unit adjustment
that could be made to the clocksource (1/(2^clock->shift)).

To compensate for that, at interrupt time we accumulate the
high-precision error between what NTP told us to use and what we're able
to use and store it in the error value. Then we apply extra short-term
correction when the error grows large enough to keep our long term
accuracy finer then the (1/2^shift) clocksource resolution.

Without this high-precision adjustment, you would have to rely on NTPd
to detect and adjust for this granularity difference, which would cause
a slower, but larger jitter.

Now the problem is, that if interrupts are delayed, this extra short
term correction might be applied for too long, causing the overshoot
issue we've seen (which I believe is the "extra jitter" issue mentioned
above).

However, if we greatly dampen our adjustments, so we are less likely to
overshoot, this means it will take longer for us to converge (ie: the
"slow adjustment" issue above).

My only problem here is this: I don't think the slow adjustment issue is
as severe as claimed. NTPd itself limits its adjustment speed to 500ppm
and the frequency of its adjustment changes are in the minutes range. So
I'm not sure I see why damping the error correction so we converge a bit
more slowly over a period of a second or two is such an issue. 

I think this would give a bit of independence between the clocksource
adjustment code and the interrupt frequency (and likely improve
robustness as well).

> > I'm just opposing the general tight coupling of the time keeping to
> > timer interrupts.
> 
> Timekeeping needs control of something, interrupts are the general 
> mechanism for asynchronous events. (Unless I'm missing something) your 
> clockevent concept is far too complex, simple clocksources only need to 
> export a shareable interrupt source, which the timekeeping code can use to 
> synchronize.
> If by "tight coupling" you mean the current hardcoded timer interrupt I 
> agree, but I still don't see that the clockevent stuff is really a better 
> solution.

Both hardware and software caused lost ticks are a reality and for the
sake of power-saving and virtualization the dynamic ticks feature is
very much desired, so I do see the clockevent stuff as being a good step
in the right direction.

The timekeeping code must be robust enough to handle unexpected lost
ticks, and entwining interrupt frequency and the timekeeping makes
high-res and dynamic ticks more difficult to implement and debug.

I would be open to some sort of frequency-hint hook that would give the
timekeeping code a better idea of what the dyntick interrupt frequency
will be, but since terrible long running BIOS SMIs won't call this, I
think the adjustment code will have to just be robust enough to handle
semi-random tick frequencies.

thanks
-john

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Thomas Gleixner]

John,

On Tue, 2006-07-11 at 17:42 -0700, john stultz wrote: 
> > > 
> > > Time keeping code reads from a given source and does the necessary
> > > adjustments when NTP is active. There is no relation to an interrupt
> > > event at all. At the very end it boils down to a linear equation which
> > > is recalculated at the synchronization points.
> > > 
> > > The timer interrupt itself is not a synchronization point.
> > 
> > If the timer interrupt is not a synchronization point, what is?
> 
> Synchronization point seems like a bad term in my mind. I prefer to
> think of it as an accumulation point (to avoid clocksource overflows) as
> well as a calculation point where we can make careful adjustments to the
> clocksource frequency. Please note that these two actions are not
> logically linked and could be done separately (although there really
> isn't a need to do so).

I used the term synchronization point for the points on the timeline,
where the information of an external time reference is available, e.g.
NTP data, a PPS interrupt ....

At those points we calculate the deviation from the external time
reference and refine the conversion and adjustment factors. In the
simplest form this boils down to a linear equation where we interpolate
between the points which are defined by the external time reference.

> > The timekeeping doesn't care much where synchronizations point comes from, 
> > but it can do a much better job, if it knows when they come.
> > Without knowing this the timekeeping code has to do extra work and has to 
> > be either optimistic or assume the worst case, neither is good for precise 
> > timekeeping, the first causes extra jitter, the latter very slow 
> > adjustments.
> 
> This I agree with. But first lets bound the issue to make it more clear
> for everyone: The conflict lies only with the high-precision clocksource
> adjustment code and not so much with the rest of the timekeeping code.
> 
> The issue being that the high-precision clocksource adjustment code is
> needed because when NTP makes an adjustment, that very precise
> adjustment might be finer then the smallest multiplier unit adjustment
> that could be made to the clocksource (1/(2^clock->shift)).
> 
> To compensate for that, at interrupt time we accumulate the
> high-precision error between what NTP told us to use and what we're able
> to use and store it in the error value. Then we apply extra short-term
> correction when the error grows large enough to keep our long term
> accuracy finer then the (1/2^shift) clocksource resolution.
> 
> Without this high-precision adjustment, you would have to rely on NTPd
> to detect and adjust for this granularity difference, which would cause
> a slower, but larger jitter.
> 
> Now the problem is, that if interrupts are delayed, this extra short
> term correction might be applied for too long, causing the overshoot
> issue we've seen (which I believe is the "extra jitter" issue mentioned
> above).
> 
> However, if we greatly dampen our adjustments, so we are less likely to
> overshoot, this means it will take longer for us to converge (ie: the
> "slow adjustment" issue above).
> 
> My only problem here is this: I don't think the slow adjustment issue is
> as severe as claimed. NTPd itself limits its adjustment speed to 500ppm
> and the frequency of its adjustment changes are in the minutes range. So
> I'm not sure I see why damping the error correction so we converge a bit
> more slowly over a period of a second or two is such an issue. 
> 
> I think this would give a bit of independence between the clocksource
> adjustment code and the interrupt frequency (and likely improve
> robustness as well).

John, that's exactly the central point. In an environment where we have
non periodic interrupts (high resolution timers, dynamic ticks) this
adjustment mechanism which relies on the periodic precise event to do
the accumulation does not work any more. I do not like the idea of
modifying this in a way that the timekeeping code does this fine grained
adjustment on the non periodic timer events. This will be a nightmare of
math and decrease robustness a lot.

The whole concept of doing the fine grained adjustment in order to
compensate for the inaccuracy of the scaled math factors on a _periodic_
event is flawed by design. The latency of interrupts in the kernel and
the fact that the periodic interrupt might be driven by a different
hardware clock, which has a different drift behaviour than the clock
which drives the time source, are reason enough to think about a
solution which makes this interdependency go away completely. In a high
resolution timer / dyntick system and also on virtualized environments
we need to get this dependency removed anyway.

The adjustment code is simply interpolation between points and boils
down to linear equations. Due to the fact that the conversion factor is
not accurate enough we need some mechanism to compensate this. I accept
that the current design has its charm, but I'm quite sure that we can do
a equally precise calculation without the interaction with the timer
interrupt code.

I know you prefer shopping over math, but it is a solvable problem.

	tglx

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[john stultz]

On Thu, 2006-07-13 at 22:27 +0200, Thomas Gleixner wrote:
> On Tue, 2006-07-11 at 17:42 -0700, john stultz wrote: 
> > However, if we greatly dampen our adjustments, so we are less likely to
> > overshoot, this means it will take longer for us to converge (ie: the
> > "slow adjustment" issue above).
> > 
> > My only problem here is this: I don't think the slow adjustment issue is
> > as severe as claimed. NTPd itself limits its adjustment speed to 500ppm
> > and the frequency of its adjustment changes are in the minutes range. So
> > I'm not sure I see why damping the error correction so we converge a bit
> > more slowly over a period of a second or two is such an issue. 
> > 
> > I think this would give a bit of independence between the clocksource
> > adjustment code and the interrupt frequency (and likely improve
> > robustness as well).
> 
> John, that's exactly the central point. In an environment where we have
> non periodic interrupts (high resolution timers, dynamic ticks) this
> adjustment mechanism which relies on the periodic precise event to do
> the accumulation does not work any more. 

Agreed. I don't want to embed any dependency on a *precise* event in the
timekeeping code. I know of too much hardware that doesn't allow that to
occur, so I really want this code to be robust in the case of lost
ticks.

However, we do want precise timekeeping. Since there is a resolution
difference between NTP's precision and the clocksources, keeping track
of that error and adjusting for it is a desirable thing to do. 

> I do not like the idea of
> modifying this in a way that the timekeeping code does this fine grained
> adjustment on the non periodic timer events. This will be a nightmare of
> math and decrease robustness a lot.
> 
> The whole concept of doing the fine grained adjustment in order to
> compensate for the inaccuracy of the scaled math factors on a _periodic_
> event is flawed by design. The latency of interrupts in the kernel and
> the fact that the periodic interrupt might be driven by a different
> hardware clock, which has a different drift behaviour than the clock
> which drives the time source, are reason enough to think about a
> solution which makes this interdependency go away completely. In a high
> resolution timer / dyntick system and also on virtualized environments
> we need to get this dependency removed anyway.

Again, agreed.  I think the conflict is that since we want to keep track
of and adjust for this fine error caused by the resolution differences,
one can keep "closer" time if those synchronization points where the
error is used to make an adjustment are close together and evenly
spaced.

Now, we *know* that isn't going to be the case. And we don't want to
make that a requirement. However, just to function, we do need some
calculation point every once in awhile, just so the clocksource doesn't
overflow, and we have a point where we can make adjustments if NTPd
tells us to adjust our clocksource frequency. These don't have to be
precise events, they just need to happen every once in awhile. Lets say
~once a second just to have a number to work with.

Again, the closer they are, the faster we can take the NTP changes and
start applying them, so they have an effect on timekeeping, but my point
is "how much?". 

Notice, for the high-precsion clocksource adjustments remember the
granularity of the error we're keeping track of is nanoseconds shifted
up by 32. That's way small.

Also note: Assuming no NTP changes during a period, the maximum error in
nanosecond accumulated over a period is the number of clocksource cycles
in that period shifted down by the clocksource shift value. (This is
since the smallest adjustment is 1 mult unit, and mult/2^shift is
1/freq)

So for a counter w/ a 4Ghz frequency and a shift value of 22 (similar to
a TSC). Over 1 second, if there was no high-precision adjustment, you
could get a *maximum* of error of ~1us (4b/2^22 = ~1024ns), which is a
1ppm drift, if left uncorrected.

Now, if we have high-precision adjustments, the question is "how fast
should we fix that 1us error?". If the code assumes we're going to have
a tick every 1 ms, it can make a stronger adjustment (of 10 mult units)
which it will remove after the next tick. This however could cause
problems if we lost ticks and that interrupt was delayed as we would
overshoot.

Thus, if we assume that ticks will show up worse case, about once a
second or two, we can make an adjustment of a single mult unit and
assume that we'll correct it over the next second. This is what Roman
was saying would be "slow adjustments", but I don't see it as too
unacceptable. 

My P-D code did just this with its two part (freq/offset) adjustment,
and Roman's current code is more careful here as well, dampening the
adjustment when the error grows.

> The adjustment code is simply interpolation between points and boils
> down to linear equations. Due to the fact that the conversion factor is
> not accurate enough we need some mechanism to compensate this. I accept
> that the current design has its charm, but I'm quite sure that we can do
> a equally precise calculation without the interaction with the timer
> interrupt code.

I'd be open to other implementations as well (I'm no control theorist,
so I've been learning as I go). But while I 110% agree that the
timekeeping code should be robust in the face of lost ticks and function
w/ dynamic ticks, I think it will be hard to escape the fact that the
higher the frequency at which we evaluate and adjust the clock against
the reference clock, the closer we can keep to it.

But since i386 *just* got sub-microsecond timekeeping resolution w/ this
patch set, I think the term "close" does not need to mean "within a
nanosecond at all times". 

And really, after dealing for so long w/ issues like "Clock can't keep
within 100ms of NTP", I find this discussion refreshing :)

thanks
-john

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Thomas Gleixner]

On Thu, 2006-07-13 at 15:05 -0700, john stultz wrote:
> Also note: Assuming no NTP changes during a period, the maximum error in
> nanosecond accumulated over a period is the number of clocksource cycles
> in that period shifted down by the clocksource shift value. (This is
> since the smallest adjustment is 1 mult unit, and mult/2^shift is
> 1/freq)
> 
> So for a counter w/ a 4Ghz frequency and a shift value of 22 (similar to
> a TSC). Over 1 second, if there was no high-precision adjustment, you
> could get a *maximum* of error of ~1us (4b/2^22 = ~1024ns), which is a
> 1ppm drift, if left uncorrected.
> 
> Now, if we have high-precision adjustments, the question is "how fast
> should we fix that 1us error?". If the code assumes we're going to have
> a tick every 1 ms, it can make a stronger adjustment (of 10 mult units)
> which it will remove after the next tick. This however could cause
> problems if we lost ticks and that interrupt was delayed as we would
> overshoot.
> 
> Thus, if we assume that ticks will show up worse case, about once a
> second or two, we can make an adjustment of a single mult unit and
> assume that we'll correct it over the next second. This is what Roman
> was saying would be "slow adjustments", but I don't see it as too
> unacceptable. 

There is really no need to worry about sub micro second accuracy, when
you look at the latencies in todays machines. We need a robust design
and no ivory tower experiments with precisions which are not useful for
anything.

	tglx

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Roman Zippel]

Hi,

On Thu, 13 Jul 2006, john stultz wrote:

> However, we do want precise timekeeping. Since there is a resolution
> difference between NTP's precision and the clocksources, keeping track
> of that error and adjusting for it is a desirable thing to do. 

I think it makes sense to focus not just on NTP, that's why I gave the 
example of per cpu clocks. Synchronizing time over the internet doesn't 
require that much precision, but it might also be used in a local network 
to keep machines as much as possible in sync.
Per cpu clocks would really push the limits. We could now chain clocks 
together where one central clock is controlled via NTP and attached to it 
run per cpu clocks, which could even run at different frequencies. Between 
these clocks one really wants to keep the error as small as technically 
possible, so that there is enough margin so that even in the worst cases 
it's rather unlikely that two threads on different cpus see widely 
different time values. The question would be now what is technically 
possible and what is needed to achieve it.

> Notice, for the high-precsion clocksource adjustments remember the
> granularity of the error we're keeping track of is nanoseconds shifted
> up by 32. That's way small.

BTW I chose that 32 mostly for convience, as it makes it easy to extract 
the nanosecond part. The current NTP code shoehorns various values into 
32bit using different scale values. Soonish this will be cleaned up to 
use 64bit values using a single scale value.

> So for a counter w/ a 4Ghz frequency and a shift value of 22 (similar to
> a TSC). Over 1 second, if there was no high-precision adjustment, you
> could get a *maximum* of error of ~1us (4b/2^22 = ~1024ns), which is a
> 1ppm drift, if left uncorrected.
> 
> Now, if we have high-precision adjustments, the question is "how fast
> should we fix that 1us error?". If the code assumes we're going to have
> a tick every 1 ms, it can make a stronger adjustment (of 10 mult units)
> which it will remove after the next tick. This however could cause
> problems if we lost ticks and that interrupt was delayed as we would
> overshoot.
> 
> Thus, if we assume that ticks will show up worse case, about once a
> second or two, we can make an adjustment of a single mult unit and
> assume that we'll correct it over the next second. This is what Roman
> was saying would be "slow adjustments", but I don't see it as too
> unacceptable. 

The error value can be a lot worse than 1us. time_adjust has a limit of 
0.5ms and NTP adjustments can be even worse than this. If the clock is now 
updated very infrequently, the clock can accumulate quite a lot of error 
before it starts changing its frequency. So if you limit the error 
correction to 1us per second it might take a very long time to reduce the 
error. You really have to keep the worst cases a bit in mind, even if they 
are rather unlikely.
BTW if you already know there will be no adjustment within the next ticks, 
the current code is very easy to change to deal with it, simply add a line 
"look_ahead = max(look_ahead, clock->min_look_ahead);" and the error 
adjustment will be spread over a longer period.
Also if you know that the average update frequency is that low, it might 
make sense to increase the shift value to get more precision for the error 
correction.

bye, Roman

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Roman Zippel]

Hi,

On Thu, 13 Jul 2006, Thomas Gleixner wrote:

> > > > The timer interrupt itself is not a synchronization point.
> > > 
> > > If the timer interrupt is not a synchronization point, what is?
> 
> I used the term synchronization point for the points on the timeline,
> where the information of an external time reference is available, e.g.
> NTP data, a PPS interrupt ....

This would be even less usable, there is no requirement that NTP is used 
for time synchronization and the kernel has no idea when then the next 
NTP adjustment happens.

> At those points we calculate the deviation from the external time
> reference and refine the conversion and adjustment factors. In the
> simplest form this boils down to a linear equation where we interpolate
> between the points which are defined by the external time reference.

This is true for the clock source, but the adjustments that come in via 
NTP are not exactly linear, so unless you want to rewrite the whole NTP 
system, I don't think this will work.

> > I think this would give a bit of independence between the clocksource
> > adjustment code and the interrupt frequency (and likely improve
> > robustness as well).
> 
> John, that's exactly the central point. In an environment where we have
> non periodic interrupts (high resolution timers, dynamic ticks) this
> adjustment mechanism which relies on the periodic precise event to do
> the accumulation does not work any more. I do not like the idea of
> modifying this in a way that the timekeeping code does this fine grained
> adjustment on the non periodic timer events. This will be a nightmare of
> math and decrease robustness a lot.
> 
> The whole concept of doing the fine grained adjustment in order to
> compensate for the inaccuracy of the scaled math factors on a _periodic_
> event is flawed by design. The latency of interrupts in the kernel and
> the fact that the periodic interrupt might be driven by a different
> hardware clock, which has a different drift behaviour than the clock
> which drives the time source, are reason enough to think about a
> solution which makes this interdependency go away completely. In a high
> resolution timer / dyntick system and also on virtualized environments
> we need to get this dependency removed anyway.
> 
> The adjustment code is simply interpolation between points and boils
> down to linear equations. Due to the fact that the conversion factor is
> not accurate enough we need some mechanism to compensate this. I accept
> that the current design has its charm, but I'm quite sure that we can do
> a equally precise calculation without the interaction with the timer
> interrupt code.
> 
> I know you prefer shopping over math, but it is a solvable problem.

Thomas, do you have any idea how insulting this is?
First of all you are pretty much wrong with your analysis. It does not 
rely on "periodic precise event", it helps if they are precise, but it's 
not a requirement and it's practically a one line change so it can deal 
with nonperiodic updates (the rest is just a matter of correct tuning). 
The "fine grained adjustment" doesn't "compensate for the inaccuracy of 
the scaled math" at all, one has to know here that we only have to be 
accurate within limits (usually the resolution of the clock), so that the 
inaccuracy is not really a problem and it has the advantage that is 
generally cheap. What the adjustments actually compensate for is the 
difference in resolution and the update delays.
Since your initial analysis is pretty much wrong, your conclusions are 
pretty much useless as well. So far so bad, by itself this wouldn't be 
really a problem yet, everyone makes mistakes. If you would at least ask 
some questions about whatever problem you'd like to see solved or if you 
would make some constructive suggestion how to solve these problems, this 
would have shown respect and could have been a nice discussion. Instead 
you just piss on my work by asking John to throw away everything I did and 
to "solve" it somehow.
Did you even consider for a second that I might have already thought about 
these problems? Did it enter your mind that the hard problems might 
already be pretty much solved? Since you don't ask me at all, you don't 
really seem to care about what I think. Instead of playing Mr. 
know-it-all, I would really appreciate it if you showed some more respect 
for my work.

bye, Roman

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Thomas Gleixner]

Roman,

At Sun, 16 Jul 2006 17:50:34 +0200 (CEST), Roman Zippel wrote:
> > > 
> > I know you prefer shopping over math, but it is a solvable problem.
> 
> Thomas, do you have any idea how insulting this is?

Sigh, 

I was talking to John and referencing his own "Math is hard, lets go shopping" 
line.

	tglx

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Roman Zippel]

Hi,

On Sun, 16 Jul 2006, Thomas Gleixner wrote:

> At Sun, 16 Jul 2006 17:50:34 +0200 (CEST), Roman Zippel wrote:
> > > > 
> > > I know you prefer shopping over math, but it is a solvable problem.
> > 
> > Thomas, do you have any idea how insulting this is?
> 
> Sigh, 
> 
> I was talking to John and referencing his own "Math is hard, lets go shopping" 
> line.

And I was refering to the complete text I quoted, please don't take things 
out of context. :-(

bye, Roman

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Pavel Machek]

On Tue 2006-07-11 00:37:06, Roman Zippel wrote:
> Hi,
> 
> On Mon, 10 Jul 2006, Pavel Machek wrote:
> 
> > APM can only keep interrupts disabled on non-IBM machines, presumably
> > due to BIOS problems.
> 
> Is it possible to disable the timer interrupt before suspend and just 
> reinit the timer afterwards?

I know little about APM...

We know that cli breaks those thinkpads.

Maybe disabling timer on PIC would do the trick? Not sure...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Mikael Pettersson]

On Mon, 10 Jul 2006 10:58:47 -0700, john stultz wrote:
>So it seems possible that the timer tick will be enabled before the
>timekeeping resume code runs. I'm not sure why this isn't seen w/ ACPI
>suspend/resume, as I think they're using the same
>sysdev_class .suspend/.resume bits. 
>
>Anyway, I think this patch should fix it (I've only compile tested it,
>as I don't have my laptop on me right now). Would you mind giving it a
>try? 
>
>thanks
>-john

With this patch my Latitude resumes OK from APM suspend again.

Thanks John.

Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>

>diff --git a/kernel/timer.c b/kernel/timer.c
>index 396a3c0..afaa594 100644
>--- a/kernel/timer.c
>+++ b/kernel/timer.c
>@@ -966,7 +966,7 @@ void __init timekeeping_init(void)
> 	write_sequnlock_irqrestore(&xtime_lock, flags);
> }
> 
>-
>+static int timekeeping_suspended;
> /*
>  * timekeeping_resume - Resumes the generic timekeeping subsystem.
>  * @dev:	unused
>@@ -982,6 +982,18 @@ static int timekeeping_resume(struct sys
> 	write_seqlock_irqsave(&xtime_lock, flags);
> 	/* restart the last cycle value */
> 	clock->cycle_last = clocksource_read(clock);
>+	clock->error = 0;
>+	timekeeping_suspended = 0;
>+	write_sequnlock_irqrestore(&xtime_lock, flags);
>+	return 0;
>+}
>+
>+static int timekeeping_suspend(struct sys_device *dev, pm_message_t state)
>+{
>+	unsigned long flags;
>+
>+	write_seqlock_irqsave(&xtime_lock, flags);
>+	timekeeping_suspended = 1;
> 	write_sequnlock_irqrestore(&xtime_lock, flags);
> 	return 0;
> }
>@@ -989,6 +1001,7 @@ static int timekeeping_resume(struct sys
> /* sysfs resume/suspend bits for timekeeping */
> static struct sysdev_class timekeeping_sysclass = {
> 	.resume		= timekeeping_resume,
>+	.suspend	= timekeeping_suspend,
> 	set_kset_name("timekeeping"),
> };
> 
>@@ -1090,14 +1103,17 @@ static void clocksource_adjust(struct cl
> static void update_wall_time(void)
> {
> 	cycle_t offset;
>-
>-	clock->xtime_nsec += (s64)xtime.tv_nsec << clock->shift;
>+	
>+	/* avoid timekeeping before we're fully resumed */
>+	if (unlikely(timekeeping_suspended))
>+		return;
> 
> #ifdef CONFIG_GENERIC_TIME
> 	offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
> #else
> 	offset = clock->cycle_interval;
> #endif
>+	clock->xtime_nsec += (s64)xtime.tv_nsec << clock->shift;
> 
> 	/* normally this loop will run just once, however in the
> 	 * case of lost or late ticks, it will accumulate correctly.
>
>

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Mikael Pettersson]

On Sun, 09 Jul 2006 17:31:39 -0400, Valdis.Kletnieks wrote:
>On Sun, 09 Jul 2006 22:58:31 +0200, Mikael Pettersson said:
>
>> I've traced the cause of this problem to the i386 time-keeping
>> changes in kernel 2.6.17-git11. What happens is that:
>> - The kernel autoselects TSC as my clocksource, which is
>>   reasonable since it's a PentiumII. 2.6.17 also chose the TSC.
>> - Immediately after APM resumes (arch/i386/kernel/apm.c line
>>   1231 in 2.6.18-rc1) there is an interrupt from the PIT,
>>   which takes us to kernel/timer.c:update_wall_time().
>> - update_wall_time() does a clocksource_read() and computes
>>   the offset from the previous read. However, the TSC was
>>   reset by HW or BIOS during the APM suspend/resume cycle and
>>   is now smaller than it was at the prevous read. On my machine,
>>   the offset is 0xffffffd598e0a566 at this point, which appears
>>   to throw update_wall_time() into a very very long loop.
>
>Does applying this patch make it work?
>
>ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc1/2.6.18-rc1-mm1/broken-out/adjust-clock-for-lost-ticks.patch
>
>Or is this a different breakage?

I haven't tried it, but I'm 99% certain it's unrelated breakage.
In my case the TSC jumped backwards not forwards as would be the
case in a lost ticks scenario, and the patch above doesn't touch
the code that gets into an semi-infinite loop in my case.

/Mikael

[BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Mikael Pettersson]

On Fri, 7 Jul 2006 21:01:26 +0200 (MEST), I wrote:
>Kernel 2.6.18-rc1 broke resume from APM suspend (to RAM)
>on my old Dell Latitude CPi laptop. At resume the disk
>spins up and the screen gets lit, but there is no response
>to the keyboard, not even sysrq. All other system activity
>also appears to be halted.
>
>I did the obvious test of reverting apm.c to the 2.6.17
>version and fixing up the fallout from the TIF_POLLING_NRFLAG
>changes, but it made no difference. So the problem must be
>somewhere else.

I've traced the cause of this problem to the i386 time-keeping
changes in kernel 2.6.17-git11. What happens is that:
- The kernel autoselects TSC as my clocksource, which is
  reasonable since it's a PentiumII. 2.6.17 also chose the TSC.
- Immediately after APM resumes (arch/i386/kernel/apm.c line
  1231 in 2.6.18-rc1) there is an interrupt from the PIT,
  which takes us to kernel/timer.c:update_wall_time().
- update_wall_time() does a clocksource_read() and computes
  the offset from the previous read. However, the TSC was
  reset by HW or BIOS during the APM suspend/resume cycle and
  is now smaller than it was at the prevous read. On my machine,
  the offset is 0xffffffd598e0a566 at this point, which appears
  to throw update_wall_time() into a very very long loop.

Hacks around the problem:
- echo jiffies>/sys/devices/system/clocksource/clocksource0/current_clocksource
  before suspending eliminates the hang. (Note: this doesn't affect
  the i386 delay loop implementation. Is that a bug or a feature?)
- Hacking apm.c to set a flag just before suspending and clear
  it only after all resume actions are done, and to have update_wall_time()
  return immediately when this flag is set also eliminates the hang.

I guess the appropriate fix would be to augment either the
TSC clocksource driver or the clocksource API to deal with
pseudo-monotonic clocks that get reset at suspend.

/Mikael

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[john stultz]

On Sun, 2006-07-09 at 22:58 +0200, Mikael Pettersson wrote:
> On Fri, 7 Jul 2006 21:01:26 +0200 (MEST), I wrote:
> >Kernel 2.6.18-rc1 broke resume from APM suspend (to RAM)
> >on my old Dell Latitude CPi laptop. At resume the disk
> >spins up and the screen gets lit, but there is no response
> >to the keyboard, not even sysrq. All other system activity
> >also appears to be halted.
> >
> >I did the obvious test of reverting apm.c to the 2.6.17
> >version and fixing up the fallout from the TIF_POLLING_NRFLAG
> >changes, but it made no difference. So the problem must be
> >somewhere else.
> 
> I've traced the cause of this problem to the i386 time-keeping
> changes in kernel 2.6.17-git11. What happens is that:
> - The kernel autoselects TSC as my clocksource, which is
>   reasonable since it's a PentiumII. 2.6.17 also chose the TSC.
> - Immediately after APM resumes (arch/i386/kernel/apm.c line
>   1231 in 2.6.18-rc1) there is an interrupt from the PIT,
>   which takes us to kernel/timer.c:update_wall_time().
> - update_wall_time() does a clocksource_read() and computes
>   the offset from the previous read. However, the TSC was
>   reset by HW or BIOS during the APM suspend/resume cycle and
>   is now smaller than it was at the prevous read. On my machine,
>   the offset is 0xffffffd598e0a566 at this point, which appears
>   to throw update_wall_time() into a very very long loop.

Huh. It seems you're getting an interrupt before timekeeping_resume()
runs (which resets cycle_last). I'll look over the code and see if I can
sort out why it works w/ ACPI suspend, but not APM, or if the
resume/interrupt-enablement bit is just racy in general.

Thanks for the bug report!
-john

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

On Sun, 09 Jul 2006 22:58:31 +0200, Mikael Pettersson said:

> I've traced the cause of this problem to the i386 time-keeping
> changes in kernel 2.6.17-git11. What happens is that:
> - The kernel autoselects TSC as my clocksource, which is
>   reasonable since it's a PentiumII. 2.6.17 also chose the TSC.
> - Immediately after APM resumes (arch/i386/kernel/apm.c line
>   1231 in 2.6.18-rc1) there is an interrupt from the PIT,
>   which takes us to kernel/timer.c:update_wall_time().
> - update_wall_time() does a clocksource_read() and computes
>   the offset from the previous read. However, the TSC was
>   reset by HW or BIOS during the APM suspend/resume cycle and
>   is now smaller than it was at the prevous read. On my machine,
>   the offset is 0xffffffd598e0a566 at this point, which appears
>   to throw update_wall_time() into a very very long loop.

Does applying this patch make it work?

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc1/2.6.18-rc1-mm1/broken-out/adjust-clock-for-lost-ticks.patch

Or is this a different breakage?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Pavel Machek]

On Sun 2006-07-09 22:58:31, Mikael Pettersson wrote:
> On Fri, 7 Jul 2006 21:01:26 +0200 (MEST), I wrote:
> >Kernel 2.6.18-rc1 broke resume from APM suspend (to RAM)
> >on my old Dell Latitude CPi laptop. At resume the disk
> >spins up and the screen gets lit, but there is no response
> >to the keyboard, not even sysrq. All other system activity
> >also appears to be halted.
> >
> >I did the obvious test of reverting apm.c to the 2.6.17
> >version and fixing up the fallout from the TIF_POLLING_NRFLAG
> >changes, but it made no difference. So the problem must be
> >somewhere else.
> 
> I've traced the cause of this problem to the i386 time-keeping
> changes in kernel 2.6.17-git11. What happens is that:
> - The kernel autoselects TSC as my clocksource, which is
>   reasonable since it's a PentiumII. 2.6.17 also chose the TSC.
> - Immediately after APM resumes (arch/i386/kernel/apm.c line
>   1231 in 2.6.18-rc1) there is an interrupt from the PIT,
>   which takes us to kernel/timer.c:update_wall_time().
> - update_wall_time() does a clocksource_read() and computes
>   the offset from the previous read. However, the TSC was
>   reset by HW or BIOS during the APM suspend/resume cycle and
>   is now smaller than it was at the prevous read. On my machine,
>   the offset is 0xffffffd598e0a566 at this point, which appears
>   to throw update_wall_time() into a very very long loop.

Step 0: could we get some sanity checks into that loop? I'm pretty
sure we'll face some TSCs going backwards... panic-king the box at
that point is okay, but infinite loop is not...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [BUG] APM resume breakage from 2.6.18-rc1 clocksource changes

[Alan Cox]

Ar Sul, 2006-07-09 am 23:44 +0200, ysgrifennodd Pavel Machek:
> Step 0: could we get some sanity checks into that loop? I'm pretty
> sure we'll face some TSCs going backwards... panic-king the box at
> that point is okay, but infinite loop is not...

If it goes backwards print a warning and carry on, a panic is as useless
for most users as a hang