PM: Make it possible to avoid wakeup events from being lost
in [Kernel]
|
Prev: problem: Re: [PATCH] input: fixup X86_MRST selects
Next: PM: Make it possible to avoid wakeup events from being lost
From: Alan Stern on 28 Jun 2010 15:20 On Mon, 28 Jun 2010, Rafael J. Wysocki wrote: > Below is the patch I'd like to add to suspend-2.6/linux-next. > > Rafael > > --- > From: Rafael J. Wysocki <rjw(a)sisk.pl> > Subject: PM: Make it possible to avoid wakeup events from being lost > > One of the arguments during the suspend blockers discussion was that > the mainline kernel didn't contain any mechanisms making it possible > to avoid losing wakeup events during system suspend. I don't see anything more to complain about in the patch. :-) You probably should change the title and the first paragraph of the description to avoid saying that events can get lost. Like: PM: Make it possible to avoid races between wakeup and system sleep Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on 30 Jun 2010 14:10 On Mon, 28 Jun 2010, Rafael J. Wysocki wrote: > +/* > + * The functions below use the observation that each wakeup event starts a > + * period in which the system should not be suspended. The moment this period > + * will end depends on how the wakeup event is going to be processed after being > + * detected and all of the possible cases can be divided into two distinct > + * groups. > + * > + * First, a wakeup event may be detected by the same functional unit that will > + * carry out the entire processing of it and possibly will pass it to user space > + * for further processing. In that case the functional unit that has detected > + * the event may later "close" the "no suspend" period associated with it > + * directly as soon as it has been dealt with. The pair of pm_stay_awake() and > + * pm_relax(), balanced with each other, is supposed to be used in such > + * situations. > + * > + * Second, a wakeup event may be detected by one functional unit and processed > + * by another one. In that case the unit that has detected it cannot really > + * "close" the "no suspend" period associated with it, unless it knows in > + * advance what's going to happen to the event during processing. This > + * knowledge, however, may not be available to it, so it can simply specify time > + * to wait before the system can be suspended and pass it as the second > + * argument of pm_wakeup_event(). > + */ Since there's no longer any way to cancel a call to pm_wakeup_event() or close the "no suspend" period early, there is no need to use dynamically-allocated delayed_work structures. You can make do with a single static timer; always keep it set to expire at the latest time passed to pm_wakeup_event(). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on 30 Jun 2010 16:00 On Wed, 30 Jun 2010, Rafael J. Wysocki wrote: > > Since there's no longer any way to cancel a call to pm_wakeup_event() > > or close the "no suspend" period early, there is no need to use > > dynamically-allocated delayed_work structures. You can make do with a > > single static timer; always keep it set to expire at the latest time > > passed to pm_wakeup_event(). > > The decremenations of events_in_progress wouldn't be balanced with > incrementations this way. Or do you have any clever way of dealing with > that in mind? Keep track of the current expiration time in a static variable called wakeup_timeout, and use 0 to indicate there is no timeout. In pm_wakeup_event() (everything protected by the spinlock): unsigned long new_timeout = jiffies + msecs_to_jiffies(msecs); if (new_timeout == 0) new_timeout = 1; ++event_count; if (!wakeup_timeout || time_after(new_timeout, wakeup_timeout)) { if (!wakeup_timeout) ++events_in_progress; wakeup_timeout = new_timeout; mod_timer(&wakeup_timer, wakeup_timeout); } In the timer routine: if (wakeup_timeout && time_before_eq(wakeup_timeout, jiffies)) { --events_in_progres; wakeup_timeout = 0; } Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on 1 Jul 2010 10:00 On Thu, 1 Jul 2010, Rafael J. Wysocki wrote: > I invented a slightly different version in the meantime, which is appended > as a patch on top of the original one (I don't want to modify the original > patch, since it's been reviewed already by several people and went to my > linux-next branch). > /** > - * pm_wakeup_work_fn - Deferred closing of a wakeup event. > + * pm_wakeup_timer_fn - Deferred closing of a wakeup event. > * > * Execute pm_relax() for a wakeup event detected in the past and free the > * work item object used for queuing up the work. > */ > -static void pm_wakeup_work_fn(struct work_struct *work) > +static void pm_wakeup_timer_fn(unsigned long data) > { > - struct delayed_work *dwork = to_delayed_work(work); > + unsigned long flags; > > - pm_relax(); > - kfree(dwork); > + spin_lock_irqsave(&events_lock, flags); > + if (events_timer_expires && time_after(jiffies, events_timer_expires)) { Should be time_after_eq. > + events_in_progress -= delayed_count; > + event_count += delayed_count; > + delayed_count = 0; > + events_timer_expires = 0; > + } > + spin_unlock_irqrestore(&events_lock, flags); > } > > /** > @@ -132,19 +145,31 @@ static void pm_wakeup_work_fn(struct wor > void pm_wakeup_event(struct device *dev, unsigned int msec) > { > unsigned long flags; > - struct delayed_work *dwork; > - > - dwork = msec ? kzalloc(sizeof(*dwork), GFP_ATOMIC) : NULL; > > spin_lock_irqsave(&events_lock, flags); > if (dev) > dev->power.wakeup_count++; > > - if (dwork) { > - INIT_DELAYED_WORK(dwork, pm_wakeup_work_fn); > - schedule_delayed_work(dwork, msecs_to_jiffies(msec)); > + if (msec) { > + ktime_t kt; > + struct timespec ts; > + unsigned long expires; > + > + kt = ktime_get(); > + kt = ktime_add_ns(kt, msec * NSEC_PER_MSEC); > + ts = ktime_to_timespec(kt); > + expires = timespec_to_jiffies(&ts); Is this somehow better than jiffies + msecs_to_jiffies(msec)? > + if (!expires) > + expires = 1; > + > + if (!events_timer_expires > + || time_after(expires, events_timer_expires)) { > + mod_timer(&events_timer, expires); > + events_timer_expires = expires; > + } > > events_in_progress++; > + delayed_count++; > } else { > event_count++; > } Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on 1 Jul 2010 16:50
On Thu, 1 Jul 2010, Rafael J. Wysocki wrote: > > > + if (msec) { > > > + ktime_t kt; > > > + struct timespec ts; > > > + unsigned long expires; > > > + > > > + kt = ktime_get(); > > > + kt = ktime_add_ns(kt, msec * NSEC_PER_MSEC); > > > + ts = ktime_to_timespec(kt); > > > + expires = timespec_to_jiffies(&ts); > > > > Is this somehow better than jiffies + msecs_to_jiffies(msec)? > > I'm not sure about overflows. That said, the "+" version is used in many > places, so there's no problem I think. Hmm. NSEC_PER_MSEC must be one million, right? So if msec referred to anything above 4 seconds (which seems unlikely but not impossible), the multiplication would overflow on a 32-bit machine. Apart from that, the main difference between the two patches lies in when the events are counted, i.e., whether event_count gets incremented at the start or when the timer expires. I can't see that it matters much either way. > Index: linux-2.6/drivers/base/power/wakeup.c > =================================================================== > --- linux-2.6.orig/drivers/base/power/wakeup.c > +++ linux-2.6/drivers/base/power/wakeup.c > @@ -9,6 +9,7 @@ > #include <linux/device.h> > #include <linux/slab.h> > #include <linux/sched.h> > +#include <linux/ktime.h> This isn't needed any more. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |