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

[Rafael J. Wysocki]

On Thursday, February 09, 2012, NeilBrown wrote:
> On Tue, 7 Feb 2012 02:04:19 +0100 "Rafael J. Wysocki" <rjw@xxxxxxx> wrote:
> 
> > From: Rafael J. Wysocki <rjw@xxxxxxx>
> > 
> > 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@xxxxxxx>
> > ---
> >  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/