Date: Mon, 5 Dec 2016 18:13:53 +0100
From: Oleg Nesterov <oleg@redhat.com>
To: Davidlohr Bueso <dave@stgolabs.net>
Cc: mingo@kernel.org, peterz@infradead.org, john.stultz@linaro.org,
        dimitrysh@google.com, linux-kernel@vger.kernel.org,
        Davidlohr Bueso <dbueso@suse.de>
Subject: Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake
        to not use wait-queues
Message-ID: <20161205171352.GB13035@redhat.com>
References: <1479495277-9075-1-git-send-email-dave@stgolabs.net> <1479495277-9075-3-git-send-email-dave@stgolabs.net> <20161203021839.GB30078@linux-80c1.suse>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161203021839.GB30078@linux-80c1.suse>
User-Agent: Mutt/1.5.18 (2008-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1105
Lines: 38

On 12/02, Davidlohr Bueso wrote:
>
> @@ -102,8 +103,13 @@ void __percpu_up_read(struct percpu_rw_semaphore *sem)
> 	 */
> 	__this_cpu_dec(*sem->read_count);
>
> +	rcu_read_lock();
> +	writer = rcu_dereference(sem->writer);
> +
> 	/* Prod writer to recheck readers_active */
> -	wake_up(&sem->writer);
> +	if (writer)
> +		wake_up_process(writer);
> +	rcu_read_unlock();

This needs a barrier between __this_cpu_dec() and rcu_dereference(), I think.

> @@ -159,8 +165,18 @@ void percpu_down_write(struct percpu_rw_semaphore *sem)
> 	 * will wait for them.
> 	 */
>
> -	/* Wait for all now active readers to complete. */
> -	wait_event(sem->writer, readers_active_check(sem));
> +	WRITE_ONCE(sem->writer, current);
> +	for (;;) {
> +		set_current_state(TASK_UNINTERRUPTIBLE);
> +
> +		if (readers_active_check(sem))
> +			break;

This looks fine, we can rely on set_current_state() which inserts a barrier
between WRITE_ONCE() and readers_active_check(). So we do not even need
WRITE_ONCE().

And the fact this needs the barriers and the comments makes me think again
you should add the new helpers.

Oleg.