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: <20090127200544.GA28843@redhat.com>
Date:	Tue, 27 Jan 2009 21:05:44 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Johannes Weiner <hannes@...xchg.org>
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 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() ?

> +		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 ;)

>  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().

Oleg.

--
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/