linux-kernel - Re: [RFC v6] wait: prevent exclusive waiter starvation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090127223116.GA21484@cmpxchg.org>
Date:	Tue, 27 Jan 2009 23:31:16 +0100
From:	Johannes Weiner <hannes@...xchg.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Chris Mason <chris.mason@...cle.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Matthew Wilcox <matthew@....cx>,
	Chuck Lever <cel@...i.umich.edu>,
	Nick Piggin <nickpiggin@...oo.com.au>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [RFC v6] wait: prevent exclusive waiter starvation

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/