Re: [PATCH v2 10/13] rwsem: simplify __rwsem_do_wake

[Peter Hurley <peter@hurleysoftware.com>]

On Fri, 2013-03-15 at 03:54 -0700, Michel Lespinasse wrote:
> This is mostly for cleanup value:
> 
> - We don't need several gotos to handle the case where the first
>   waiter is a writer. Two simple tests will do (and generate very
>   similar code).
> 
> - In the remainder of the function, we know the first waiter is a reader,
>   so we don't have to double check that. We can use do..while loops
>   to iterate over the readers to wake (generates slightly better code).
> 
> Signed-off-by: Michel Lespinasse <walken@google.com>
> 
> ---
>  lib/rwsem-spinlock.c | 25 ++++++++-----------------
>  lib/rwsem.c          | 26 ++++++++++++--------------
>  2 files changed, 20 insertions(+), 31 deletions(-)
> 
> diff --git a/lib/rwsem-spinlock.c b/lib/rwsem-spinlock.c
> index 5f117f37ac0a..20207a6e8ac3 100644
> --- a/lib/rwsem-spinlock.c
> +++ b/lib/rwsem-spinlock.c
> @@ -70,26 +70,17 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wakewrite)
>  
>  	waiter = list_entry(sem->wait_list.next, struct rwsem_waiter, list);
>  
> -	if (!wakewrite) {
> -		if (waiter->type == RWSEM_WAITING_FOR_WRITE)
> -			goto out;
> -		goto dont_wake_writers;
> -	}
> -
> -	/*
> -	 * as we support write lock stealing, we can't set sem->activity
> -	 * to -1 here to indicate we get the lock. Instead, we wake it up
> -	 * to let it go get it again.
> -	 */
>  	if (waiter->type == RWSEM_WAITING_FOR_WRITE) {
> -		wake_up_process(waiter->task);
> +		if (wakewrite)
> +			/* Wake up a writer. Note that we do not grant it the
> +			 * lock - it will have to acquire it when it runs. */
> +			wake_up_process(waiter->task);
>  		goto out;
>  	}
>  
> -	/* grant an infinite number of read locks to the front of the queue */
> - dont_wake_writers:
> +	/* grant read locks to all queued readers. */

I think the original comment still applies here now?

>  	woken = 0;
> -	while (waiter->type == RWSEM_WAITING_FOR_READ) {
> +	do {
>  		struct list_head *next = waiter->list.next;
>  
>  		list_del(&waiter->list);
> @@ -99,10 +90,10 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wakewrite)
>  		wake_up_process(tsk);
>  		put_task_struct(tsk);
>  		woken++;
> -		if (list_empty(&sem->wait_list))
> +		if (next == &sem->wait_list)
>  			break;
>  		waiter = list_entry(next, struct rwsem_waiter, list);
> -	}
> +	} while (waiter->type != RWSEM_WAITING_FOR_WRITE);

Maybe convert this to list_for_each_entry_safe() since you're cleaning
it up?

>  
>  	sem->activity += woken;
>  
> diff --git a/lib/rwsem.c b/lib/rwsem.c
> index 0d50e46d5b0c..9a675fa9d78e 100644
> --- a/lib/rwsem.c
> +++ b/lib/rwsem.c
> @@ -68,20 +68,17 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wake_type)
>  	signed long woken, loop, adjustment;
>  
>  	waiter = list_entry(sem->wait_list.next, struct rwsem_waiter, list);
> -	if (waiter->type != RWSEM_WAITING_FOR_WRITE)
> -		goto readers_only;
> -
> -	if (wake_type == RWSEM_WAKE_READ_OWNED)
> -		/* Another active reader was observed, so wakeup is not
> -		 * likely to succeed. Save the atomic op.
> -		 */
> +	if (waiter->type == RWSEM_WAITING_FOR_WRITE) {
> +		if (wake_type != RWSEM_WAKE_READ_OWNED)
> +			/* 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.
> +			 */
> +			wake_up_process(waiter->task);
>  		goto out;
> +	}
>  
> -	/* Wake up the writing waiter and let the task grab the sem: */
> -	wake_up_process(waiter->task);
> -	goto out;
> -
> - readers_only:
>  	/* If we come here from up_xxxx(), another thread might have reached
>  	 * rwsem_down_failed_common() before we acquired the spinlock and
>  	 * woken up a waiter, making it now active.  We prefer to check for
> @@ -125,7 +122,8 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wake_type)
>  	rwsem_atomic_add(adjustment, sem);
>  
>  	next = sem->wait_list.next;
> -	for (loop = woken; loop > 0; loop--) {
> +	loop = woken;
> +	do {
>  		waiter = list_entry(next, struct rwsem_waiter, list);
>  		next = waiter->list.next;
>  		tsk = waiter->task;
> @@ -133,7 +131,7 @@ __rwsem_do_wake(struct rw_semaphore *sem, int wake_type)
>  		waiter->task = NULL;
>  		wake_up_process(tsk);
>  		put_task_struct(tsk);
> -	}
> +	} while (--loop);
>  
>  	sem->wait_list.next = next;
>  	next->prev = &sem->wait_list;