Re: [RFC v6] wait: prevent exclusive waiter starvation

[Johannes Weiner <hannes@cmpxchg.org>]

On Tue, Jan 27, 2009 at 09:05:44PM +0100, Oleg Nesterov wrote:
> On 01/27, Johannes Weiner wrote:
> >
> > +void abort_exclusive_wait(wait_queue_head_t *q, wait_queue_t *wait)
> > +{
> > +	unsigned long flags;
> > +
> > +	__set_current_state(TASK_RUNNING);
> > +	spin_lock_irqsave(&q->lock, flags);
> > +	if (list_empty(&wait->task_list))
> 
> Hmm... it should be !list_empty() ?

Yes.

> 
> > +		list_del_init(&wait->task_list);
> > +	/*
> > +	 * If we were woken through the waitqueue (waker removed
> > +	 * us from the list) we must ensure the next waiter down
> > +	 * the line is woken up.  The callsite will not do it as
> > +	 * it didn't finish waiting successfully.
> > +	 */
> > +	else if (waitqueue_active(q))
> > +		__wake_up_locked(q, TASK_INTERRUPTIBLE);
> > +	spin_unlock_irqrestore(&q->lock, flags);
> > +}
> 
> Well, personally I don't care, but this is against CodingStyle rules ;)

I removed it from there and added a note to the kerneldoc.

> >  int autoremove_wake_function(wait_queue_t *wait, unsigned mode, int sync, void *key)
> >  {
> >  	int ret = default_wake_function(wait, mode, sync, key);
> > @@ -177,17 +218,19 @@ int __sched
> >  __wait_on_bit_lock(wait_queue_head_t *wq, struct wait_bit_queue *q,
> >  			int (*action)(void *), unsigned mode)
> >  {
> > -	int ret = 0;
> > -
> >  	do {
> > +		int ret;
> > +
> >  		prepare_to_wait_exclusive(wq, &q->wait, mode);
> > -		if (test_bit(q->key.bit_nr, q->key.flags)) {
> > -			if ((ret = (*action)(q->key.flags)))
> > -				break;
> > -		}
> > +		if (!test_bit(q->key.bit_nr, q->key.flags))
> > +			continue;
> > +		if (!(ret = action(q->key.flags)))
> > +			continue;
> > +		abort_exclusive_wait(wq, &q->wait);
> 
> No, no. We should use the same key in abort_exclusive_wait().
> Otherwise, how can we wakeup the next waiter which needs this
> bit in the same page->flags?
> 
> That is why I suggested finish_wait_exclusive(..., void *key)
> which should we passed to __wake_up_common().

Okay, I am obviously wasting our time now.  And I definitely stared so
long at the same three lines that I send randomly broken patches, so
v7 coming after some delay including sleep.

Thanks for your patience,

	hannes