Re: [PATCH 4/8] PM / Sleep: Use wait queue to signal "no wakeup events in progress"

Rafael J. Wysocki
2012-02-09T00:02:05Z

On Thursday, February 09, 2012, NeilBrown wrote:
> On Tue, 7 Feb 2012 02:04:19 +0100 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > The current wakeup source deactivation code doesn't do anything when
> > the counter of wakeup events in progress goes down to zero, which
> > requires pm_get_wakeup_count() to poll that counter periodically.
> > Although this reduces the average time it takes to deactivate a
> > wakeup source, it also may lead to a substantial amount of unnecessary
> > polling if there are extended periods of wakeup activity. Thus it
> > seems reasonable to use a wait queue for signaling the "no wakeup
> > events in progress" condition and remove the polling.
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> > drivers/base/power/wakeup.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > Index: linux/drivers/base/power/wakeup.c
> > ===================================================================
> > --- linux.orig/drivers/base/power/wakeup.c
> > +++ linux/drivers/base/power/wakeup.c
> > @@ -17,8 +17,6 @@
> >
> > #include "power.h"
> >
> > -#define TIMEOUT 100
> > -
> > /*
> > * If set, the suspend/hibernate code will abort transitions to a sleep state
> > * if wakeup events are registered during or immediately before the transition.
> > @@ -52,6 +50,8 @@ static void pm_wakeup_timer_fn(unsigned
> >
> > static LIST_HEAD(wakeup_sources);
> >
> > +static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
> > +
> > /**
> > * wakeup_source_create - Create a struct wakeup_source object.
> > * @name: Name of the new wakeup source.
> > @@ -84,7 +84,7 @@ void wakeup_source_destroy(struct wakeup
> > while (ws->active) {
> > spin_unlock_irq(&ws->lock);
> >
> > - schedule_timeout_interruptible(msecs_to_jiffies(TIMEOUT));
> > + schedule_timeout_interruptible(msecs_to_jiffies(100));
> >
> > spin_lock_irq(&ws->lock);
> > }
> > @@ -411,6 +411,7 @@ EXPORT_SYMBOL_GPL(pm_stay_awake);
> > */
> > static void wakeup_source_deactivate(struct wakeup_source *ws)
> > {
> > + unsigned int cnt, inpr;
> > ktime_t duration;
> > ktime_t now;
> >
> > @@ -444,6 +445,10 @@ static void wakeup_source_deactivate(str
> > * couter of wakeup events in progress simultaneously.
> > */
> > atomic_add(MAX_IN_PROGRESS, &combined_event_count);
> > +
> > + split_counters(&cnt, &inpr);
> > + if (!inpr)
> > + wake_up_all(&wakeup_count_wait_queue);
> > }
>
> Would it be worth making this:
>
> if (!inpr && waitqueue_active(&wakeup_count_wait_queue))
> wake_up_all(&wakeup_count_wait_queue);
>
> ??
> It would often save a spinlock.

Yes, good point. :-)

> Also was there a reason you used wake_up_all(). That is only really needed
> were EXCLUSIVE waits are happening, and there aren't any of those.

Right, I think wake_up() should be fine too.

Thanks,
Rafael