[PATCH] Prevent clockevent event_handler ending up handler_noop

[PATCH] Prevent clockevent event_handler ending up handler_noop

[Venki Pallipadi]

There is a ordering related problem with clockevents code, due to which
clockevents_register_device() called after tickless/highres switch
will not work. The new clockevent ends up with clockevents_handle_noop as
event handler, resulting in no timer activity.

The problematic path seems to be

* old device already has hrtimer_interrupt as the event_handler
* new clockevent device registers with a higher rating
* tick_check_new_device() is called
  * clockevents_exchange_device() gets called
    * old->event_handler is set to clockevents_handle_noop
  * tick_setup_device() is called for the new device
    * which sets new->event_handler using the old->event_handler which is noop.

Change the ordering so that new device inherits the proper handler.

This does not have any issue in normal case as most likely all the clockevent
devices are setup before the highres switch. But, can potentially be affecting
some corner case where HPET force detect happens after the highres switch.
This was a problem with HPET in MSI mode code that we have been experimenting
with.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Signed-off-by: Shaohua Li <shaohua.li@intel.com>

---
 include/linux/clockchips.h |    2 ++
 kernel/time/clockevents.c  |    3 +--
 kernel/time/tick-common.c  |    1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

Index: tip/kernel/time/clockevents.c
===================================================================
--- tip.orig/kernel/time/clockevents.c	2008-09-02 13:26:35.000000000 -0700
+++ tip/kernel/time/clockevents.c	2008-09-02 15:24:13.000000000 -0700
@@ -177,7 +177,7 @@ void clockevents_register_device(struct 
 /*
  * Noop handler when we shut down an event device
  */
-static void clockevents_handle_noop(struct clock_event_device *dev)
+void clockevents_handle_noop(struct clock_event_device *dev)
 {
 }
 
@@ -199,7 +199,6 @@ void clockevents_exchange_device(struct 
 	 * released list and do a notify add later.
 	 */
 	if (old) {
-		old->event_handler = clockevents_handle_noop;
 		clockevents_set_mode(old, CLOCK_EVT_MODE_UNUSED);
 		list_del(&old->list);
 		list_add(&old->list, &clockevents_released);
Index: tip/kernel/time/tick-common.c
===================================================================
--- tip.orig/kernel/time/tick-common.c	2008-09-02 13:26:35.000000000 -0700
+++ tip/kernel/time/tick-common.c	2008-09-02 15:24:13.000000000 -0700
@@ -161,6 +161,7 @@ static void tick_setup_device(struct tic
 	} else {
 		handler = td->evtdev->event_handler;
 		next_event = td->evtdev->next_event;
+		td->evtdev->event_handler = clockevents_handle_noop;
 	}
 
 	td->evtdev = newdev;
Index: tip/include/linux/clockchips.h
===================================================================
--- tip.orig/include/linux/clockchips.h	2008-09-02 13:26:34.000000000 -0700
+++ tip/include/linux/clockchips.h	2008-09-02 15:24:13.000000000 -0700
@@ -127,6 +127,8 @@ extern int clockevents_register_notifier
 extern int clockevents_program_event(struct clock_event_device *dev,
 				     ktime_t expires, ktime_t now);
 
+extern void clockevents_handle_noop(struct clock_event_device *dev);
+
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 extern void clockevents_notify(unsigned long reason, void *arg);
 #else

Re: [PATCH] Prevent clockevent event_handler ending up handler_noop

[Thomas Gleixner]

On Tue, 2 Sep 2008, Venki Pallipadi wrote:
> There is a ordering related problem with clockevents code, due to which
> clockevents_register_device() called after tickless/highres switch
> will not work. The new clockevent ends up with clockevents_handle_noop as
> event handler, resulting in no timer activity.
> 
> The problematic path seems to be
> 
> * old device already has hrtimer_interrupt as the event_handler
> * new clockevent device registers with a higher rating
> * tick_check_new_device() is called
>   * clockevents_exchange_device() gets called
>     * old->event_handler is set to clockevents_handle_noop
>   * tick_setup_device() is called for the new device
>     * which sets new->event_handler using the old->event_handler which is noop.
> 
> Change the ordering so that new device inherits the proper handler.
> 
> This does not have any issue in normal case as most likely all the clockevent
> devices are setup before the highres switch. But, can potentially be affecting
> some corner case where HPET force detect happens after the highres switch.
> This was a problem with HPET in MSI mode code that we have been experimenting
> with.
> 
> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

Acked-by: Thomas Gleixner <tglx@linutronix.de>

Should go into stable as well.

/me looks for a brown paperbag

Thanks, 

	tglx

> ---
>  include/linux/clockchips.h |    2 ++
>  kernel/time/clockevents.c  |    3 +--
>  kernel/time/tick-common.c  |    1 +
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> Index: tip/kernel/time/clockevents.c
> ===================================================================
> --- tip.orig/kernel/time/clockevents.c	2008-09-02 13:26:35.000000000 -0700
> +++ tip/kernel/time/clockevents.c	2008-09-02 15:24:13.000000000 -0700
> @@ -177,7 +177,7 @@ void clockevents_register_device(struct 
>  /*
>   * Noop handler when we shut down an event device
>   */
> -static void clockevents_handle_noop(struct clock_event_device *dev)
> +void clockevents_handle_noop(struct clock_event_device *dev)
>  {
>  }
>  
> @@ -199,7 +199,6 @@ void clockevents_exchange_device(struct 
>  	 * released list and do a notify add later.
>  	 */
>  	if (old) {
> -		old->event_handler = clockevents_handle_noop;
>  		clockevents_set_mode(old, CLOCK_EVT_MODE_UNUSED);
>  		list_del(&old->list);
>  		list_add(&old->list, &clockevents_released);
> Index: tip/kernel/time/tick-common.c
> ===================================================================
> --- tip.orig/kernel/time/tick-common.c	2008-09-02 13:26:35.000000000 -0700
> +++ tip/kernel/time/tick-common.c	2008-09-02 15:24:13.000000000 -0700
> @@ -161,6 +161,7 @@ static void tick_setup_device(struct tic
>  	} else {
>  		handler = td->evtdev->event_handler;
>  		next_event = td->evtdev->next_event;
> +		td->evtdev->event_handler = clockevents_handle_noop;
>  	}
>  
>  	td->evtdev = newdev;
> Index: tip/include/linux/clockchips.h
> ===================================================================
> --- tip.orig/include/linux/clockchips.h	2008-09-02 13:26:34.000000000 -0700
> +++ tip/include/linux/clockchips.h	2008-09-02 15:24:13.000000000 -0700
> @@ -127,6 +127,8 @@ extern int clockevents_register_notifier
>  extern int clockevents_program_event(struct clock_event_device *dev,
>  				     ktime_t expires, ktime_t now);
>  
> +extern void clockevents_handle_noop(struct clock_event_device *dev);
> +
>  #ifdef CONFIG_GENERIC_CLOCKEVENTS
>  extern void clockevents_notify(unsigned long reason, void *arg);
>  #else
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] Prevent clockevent event_handler ending up handler_noop

[Andreas Mohr]

Hi,

> This does not have any issue in normal case as most likely all the clockevent
> devices are setup before the highres switch. But, can potentially be affecting
> some corner case where HPET force detect happens after the highres switch.

So... does that apply to a nice sound card with an even nicer
1 MHz "DirectX timer" as well
which I would soon force into registering a clock_event_device? ;)
(I might add that this sound driver is _modular_, BTW it's azt3328.c)

Or did I completely slide off the usual tracks of what is "politically
correct" to do with kernel source code here? ;)

Oh, and how would this integrate (or rather, "conflict"?)
with globally managed PIT / HPET event device handling?

I'm halfway through converting this driver, thus a not entirely negative
reply would be helpful...

See what such highres-deprived people as myself (PIT, acpi_pm,
_no_ HPET and thus no IRQed highres timer) are doing now,
PITy (sic!) my poor soul...

This timer could even be considered the predecessor of HPET,
since AFAIK several sound cards have such a feature
(maybe it's even semi-standardized?)
and thus it might be very worthwhile for tuning of rotten hardware.

And any hints on how to possibly (reliably!) provide both clock_event_device
(oneshot) _and_ clocksource for a _single_ IRQed countdown timer?

Thanks,

Andreas Mohr

Re: [PATCH] Prevent clockevent event_handler ending up handler_noop

[Thomas Gleixner]

On Wed, 3 Sep 2008, Andreas Mohr wrote:
> Hi,
> 
> > This does not have any issue in normal case as most likely all the clockevent
> > devices are setup before the highres switch. But, can potentially be affecting
> > some corner case where HPET force detect happens after the highres switch.
> 
> So... does that apply to a nice sound card with an even nicer
> 1 MHz "DirectX timer" as well
> which I would soon force into registering a clock_event_device? ;)
> (I might add that this sound driver is _modular_, BTW it's azt3328.c)

Yup, the switchover to any new device can cause this. Right now we
only have HPET replacing PIT, but ..
 
> Or did I completely slide off the usual tracks of what is "politically
> correct" to do with kernel source code here? ;)

-ENOPARSE

> Oh, and how would this integrate (or rather, "conflict"?)
> with globally managed PIT / HPET event device handling?

Pretty much not. We have this global management as HPET replaces the
PIT and occupies the PIT irq as well, so we need to know which one is
active.

> See what such highres-deprived people as myself (PIT, acpi_pm,
> _no_ HPET and thus no IRQed highres timer) are doing now,

Err. PIT + acpi_pm works with highres. acpi_pm is a stable clocksource
and PIT is not a good, but a usable oneshot timer.
 
> And any hints on how to possibly (reliably!) provide both clock_event_device
> (oneshot) _and_ clocksource for a _single_ IRQed countdown timer?

No. For a reliable clocksource you need an ever increasing /
decreasing counter, which just wraps around when the max. count is
reached.

If you use a single shot timer, which is reprogrammed after every
interrupt, then you will deviate from the time line as you have no
idea how long it took from the counter reaching zero to the point
where you reload the timer. Simply wont work.

Thanks,

	tglx

Re: [PATCH] Prevent clockevent event_handler ending up handler_noop

[Andreas Mohr]

Hi,

On Wed, Sep 03, 2008 at 07:15:16PM +0200, Thomas Gleixner wrote:
> On Wed, 3 Sep 2008, Andreas Mohr wrote:
> > See what such highres-deprived people as myself (PIT, acpi_pm,
> > _no_ HPET and thus no IRQed highres timer) are doing now,
> 
> Err. PIT + acpi_pm works with highres. acpi_pm is a stable clocksource
> and PIT is not a good, but a usable oneshot timer.

Hmm. What I'm interested in is longer wakeup timeouts (ACPI C2/C3
stuff), which would be a problem with PIT (~20 forced wakeups per second)
but with my new timer that would work.
Oh, side question: it _is_ easily possible to then disable PIT IRQs
once my event device is registered, right? Otherwise I could forget
about power management benefits...

And of course I'm interested in very short delays as well ("highres"),
which my timer could provide.

> > And any hints on how to possibly (reliably!) provide both clock_event_device
> > (oneshot) _and_ clocksource for a _single_ IRQed countdown timer?
> 
> No. For a reliable clocksource you need an ever increasing /
> decreasing counter, which just wraps around when the max. count is
> reached.
> 
> If you use a single shot timer, which is reprogrammed after every
> interrupt, then you will deviate from the time line as you have no
> idea how long it took from the counter reaching zero to the point
> where you reload the timer. Simply wont work.

Yup, that's exactly what I thought, slowly deviating from the timing
that it actually should have been, due to reprogramming...


So as I see it, the most beneficial thing I can do is to provide
a clock_event_device only.

Reasoning being that acpi_pm already is an easily accessible (fast)
reliable clocksource,
yet PIT is a _very_ slow (ISA timing) relatively coarse and
force-wakeupping event device.

Then additionally upgrade my driver to do mmapped access to this
PCI-MEM-capable sound card, and there we go with a fast and fine-grained
event device.

Out of interest: how big would you believe the benefit to be?
(overall system performance gains and ACPI PM benefits)

Thanks,

Andreas Mohr

Re: [PATCH] Prevent clockevent event_handler ending up handler_noop

[Thomas Gleixner]

On Wed, 3 Sep 2008, Andreas Mohr wrote:
> On Wed, Sep 03, 2008 at 07:15:16PM +0200, Thomas Gleixner wrote:
> > On Wed, 3 Sep 2008, Andreas Mohr wrote:
> > > See what such highres-deprived people as myself (PIT, acpi_pm,
> > > _no_ HPET and thus no IRQed highres timer) are doing now,
> > 
> > Err. PIT + acpi_pm works with highres. acpi_pm is a stable clocksource
> > and PIT is not a good, but a usable oneshot timer.
> 
> Hmm. What I'm interested in is longer wakeup timeouts (ACPI C2/C3
> stuff), which would be a problem with PIT (~20 forced wakeups per second)
> but with my new timer that would work.
> Oh, side question: it _is_ easily possible to then disable PIT IRQs
> once my event device is registered, right? Otherwise I could forget
> about power management benefits...

When you register something better than PIT, then the PIT is
disabled completely. No more interrupts from there.

> So as I see it, the most beneficial thing I can do is to provide
> a clock_event_device only.

Right.
 
> Out of interest: how big would you believe the benefit to be?
> (overall system performance gains and ACPI PM benefits)

Hard to tell, your milage can vary.

     tglx

Re: [PATCH] Prevent clockevent event_handler ending up handler_noop

[Andreas Mohr]

Hi,

On Wed, Sep 03, 2008 at 09:01:35PM +0200, Thomas Gleixner wrote:
> When you register something better than PIT, then the PIT is
> disabled completely. No more interrupts from there.

GOOD.

> > Out of interest: how big would you believe the benefit to be?
> > (overall system performance gains and ACPI PM benefits)
> 
> Hard to tell, your milage can vary.

ACPI PM benefits exclusively depend on whether I can make my system go
below 20 wakeups/s at all or not, otherwise there won't be any improvement
of course.

I/O overhead due to PIT ISA access _might_ be a couple % of overall
system performance, but as you say, this is hard to tell, admittedly.

We'll see rather soon, since integration is finished, however I get
MODPOST symbol link errors for clockevents_register_device() and another
clockevent function which I forgot.
Most certainly we should have had an EXPORT_SYMBOL_GPL() for
these? Or are you going to tell me that modular registration won't work
at all (yet?) for clock_event_device, as opposed to clocksource?

Anything else I need to take into account, other than applying the
fixup patch in the parent post and exporting these symbols?
And how to unregister the event device on module unload?

Oh, and another question:
Since this timer hardware was previously hooked up to an ALSA snd_timer
(struct snd_timer_hardware), do you think changing this into
clock_event_device provides almost equivalent service quality?
snd_timer is used for precise MIDI timestamping (and not much else,
thus this valuable timer was entirely unused/idle, read: wasted), I think,
so I wonder what kind of accuracy loss (jitter, additional timer/IRQ load)
I'd have for MIDI operation if I used this timer for the entire system's
highres timer wheel. If you believe this is even remotely comparably robust
as using a snd_timer-only construct, then I'm more than happy to rip out
snd_timer and _always_ go with clock_event_device instead.


Looking forward to getting my box crashed, hard ;)

Thanks!

Andreas

Re: [PATCH] Prevent clockevent event_handler ending up handler_noop

[Thomas Gleixner]

On Thu, 4 Sep 2008, Andreas Mohr wrote:
> We'll see rather soon, since integration is finished, however I get
> MODPOST symbol link errors for clockevents_register_device() and another
> clockevent function which I forgot.
> Most certainly we should have had an EXPORT_SYMBOL_GPL() for
> these? Or are you going to tell me that modular registration won't work
> at all (yet?) for clock_event_device, as opposed to clocksource?

Should work fine. We just did not have a user yet.
 
> Anything else I need to take into account, other than applying the
> fixup patch in the parent post and exporting these symbols?
> And how to unregister the event device on module unload?

Not implemented :) Shouldnt be that hard, but is it worth the trouble ?
 
> Oh, and another question:
> Since this timer hardware was previously hooked up to an ALSA snd_timer
> (struct snd_timer_hardware), do you think changing this into
> clock_event_device provides almost equivalent service quality?
> snd_timer is used for precise MIDI timestamping (and not much else,
> thus this valuable timer was entirely unused/idle, read: wasted), I think,
> so I wonder what kind of accuracy loss (jitter, additional timer/IRQ load)
> I'd have for MIDI operation if I used this timer for the entire system's
> highres timer wheel. If you believe this is even remotely comparably robust
> as using a snd_timer-only construct, then I'm more than happy to rip out
> snd_timer and _always_ go with clock_event_device instead.

I think it should be pretty robust.

Thanks,

	tglx