linux-kernel - Re: [PATCH 3/4] locking/rwsem: Enable lockless waiter wakeup(s)

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: <20160509074830.GD3430@twins.programming.kicks-ass.net>
Date:	Mon, 9 May 2016 09:48:30 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Davidlohr Bueso <dave@...olabs.net>
Cc:	mingo@...nel.org, tglx@...utronix.de, Waiman.Long@....com,
	jason.low2@...com, linux-kernel@...r.kernel.org,
	Davidlohr Bueso <dbueso@...e.de>
Subject: Re: [PATCH 3/4] locking/rwsem: Enable lockless waiter wakeup(s)

On Sun, May 08, 2016 at 09:56:09PM -0700, Davidlohr Bueso wrote:
> @@ -129,12 +133,14 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type)
>  	waiter = list_entry(sem->wait_list.next, struct rwsem_waiter, list);
>  	if (waiter->type == RWSEM_WAITING_FOR_WRITE) {
>  		if (wake_type == RWSEM_WAKE_ANY)
> -			/* Wake writer at the front of the queue, but do not
> -			 * grant it the lock yet as we want other writers
> -			 * to be able to steal it.  Readers, on the other hand,
> -			 * will block as they will notice the queued writer.
> +			/*
> +			 * Mark writer at the front of the queue for wakeup.
> +			 * Until the task is actually later awoken later by
> +			 * the caller, other writers are able to steal it the
> +			 * lock to be able to steal it.  Readers, on the other,
> +			 * hand, will block as they will notice the queued writer.
>  			 */
> -			wake_up_process(waiter->task);
> +			wake_q_add(wake_q, waiter->task);

Thanks for fixing that comment; that bugged the hell out of me ;-)

>  		goto out;
>  	}
>  
> @@ -196,12 +202,11 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum rwsem_wake_type wake_type)
>  		 */
>  		smp_mb();
>  		waiter->task = NULL;
> -		wake_up_process(tsk);
> +		wake_q_add(wake_q, tsk);


However, note that per the race in the previous email; this is too late
to acquire the tsk refcount. I think it'll work if you do wake_q_add()
_before_ the waiter->task = NULL.

On that same note; I think that you can replace:

	smp_mb();
	waiter->task = NULL;

with:

	smp_store_release(&waiter->task, NULL);

>  	} while (--loop);
>  
>  	sem->wait_list.next = next;
>  	next->prev = &sem->wait_list;