Re: [PATCH 5/5] locking/percpu-rwsem: Remove the embedded rwsem

[Davidlohr Bueso]

On Wed, 13 Nov 2019, Peter Zijlstra wrote:
> @@ -54,23 +52,23 @@ static bool __percpu_down_read_trylock(s
> 	 * the same CPU as the increment, avoiding the
> 	 * increment-on-one-CPU-and-decrement-on-another problem.

Nit: Now that you've made read_count more symmetric, maybe this first
paragraph can be moved down to __percpu_rwsem_trylock() reader side,
as such:

	/*
	 * Due to having preemption disabled the decrement happens on
	 * the same CPU as the increment, avoiding the
	 * increment-on-one-CPU-and-decrement-on-another problem.
	 */
	preempt_disable();
	ret = __percpu_down_read_trylock(sem);
	preempt_enable();

> 	 *
> -	 * If the reader misses the writer's assignment of readers_block, then
> -	 * the writer is guaranteed to see the reader's increment.
> +	 * If the reader misses the writer's assignment of sem->block, then the
> +	 * writer is guaranteed to see the reader's increment.

...

> bool __percpu_down_read(struct percpu_rw_semaphore *sem, bool try)
> {
> 	if (__percpu_down_read_trylock(sem))
> @@ -89,20 +156,10 @@ bool __percpu_down_read(struct percpu_rw
> 	if (try)
> 		return false;
>
> -	/*
> -	 * We either call schedule() in the wait, or we'll fall through
> -	 * and reschedule on the preempt_enable() in percpu_down_read().
> -	 */
> -	preempt_enable_no_resched();
> -
> -	/*
> -	 * Avoid lockdep for the down/up_read() we already have them.
> -	 */
> -	__down_read(&sem->rw_sem);
> -	this_cpu_inc(*sem->read_count);
> -	__up_read(&sem->rw_sem);
> -
> +	preempt_enable();
> +	percpu_rwsem_wait(sem, /* .reader = */ true );
> 	preempt_disable();
> +
> 	return true;
> }
> EXPORT_SYMBOL_GPL(__percpu_down_read);

Do we really need to export symbol here? This function is only called
from percpu-rwsem.h.

> @@ -117,7 +174,7 @@ void __percpu_up_read(struct percpu_rw_s
> 	 */
> 	__this_cpu_dec(*sem->read_count);
>
> -	/* Prod writer to recheck readers_active */
> +	/* Prod writer to re-evaluate readers_active_check() */
> 	rcuwait_wake_up(&sem->writer);
> }
> EXPORT_SYMBOL_GPL(__percpu_up_read);
> @@ -137,6 +194,8 @@ EXPORT_SYMBOL_GPL(__percpu_up_read);
>  * zero.  If this sum is zero, then it is stable due to the fact that if any
>  * newly arriving readers increment a given counter, they will immediately
>  * decrement that same counter.
> + *
> + * Assumes sem->block is set.
>  */
> static bool readers_active_check(struct percpu_rw_semaphore *sem)
> {
> @@ -160,23 +219,22 @@ void percpu_down_write(struct percpu_rw_
> 	/* Notify readers to take the slow path. */
> 	rcu_sync_enter(&sem->rss);
>
> -	__down_write(&sem->rw_sem);
> -
> 	/*
> -	 * Notify new readers to block; up until now, and thus throughout the
> -	 * longish rcu_sync_enter() above, new readers could still come in.
> +	 * Try set sem->block; this provides writer-writer exclusion.
> +	 * Having sem->block set makes new readers block.
> 	 */
> -	WRITE_ONCE(sem->readers_block, 1);
> +	if (!__percpu_down_write_trylock(sem))
> +		percpu_rwsem_wait(sem, /* .reader = */ false);
>
> -	smp_mb(); /* D matches A */
> +	/* smp_mb() implied by __percpu_down_writer_trylock() on success -- D matches A */
                                              ^^^
					       write
...

> --- a/kernel/locking/rwsem.h
> +++ b/kernel/locking/rwsem.h
> @@ -1,12 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -
> -#ifndef __INTERNAL_RWSEM_H
> -#define __INTERNAL_RWSEM_H
> -#include <linux/rwsem.h>
> -
> -extern void __down_read(struct rw_semaphore *sem);
> -extern void __up_read(struct rw_semaphore *sem);
> -extern void __down_write(struct rw_semaphore *sem);
> -extern void __up_write(struct rw_semaphore *sem);

This is a nice side effect.

Thanks,
Davidlohr