Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1535322 > unrolled thread

[PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake to not use wait-queues

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake to not  use wait-queues Davidlohr Bueso <dave@stgolabs.net> - 2016-12-03 03:20 +0100
    Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake to  not use wait-queues Peter Zijlstra <peterz@infradead.org> - 2016-12-05 09:40 +0100
      Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake         to not use wait-queues Oleg Nesterov <oleg@redhat.com> - 2016-12-05 12:30 +0100
        Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake         to not use wait-queues Oleg Nesterov <oleg@redhat.com> - 2016-12-05 12:50 +0100
        Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake to  not use wait-queues Davidlohr Bueso <dave@stgolabs.net> - 2016-12-05 18:40 +0100
      Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake         to not use wait-queues Oleg Nesterov <oleg@redhat.com> - 2016-12-05 18:20 +0100
    Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake         to not use wait-queues Oleg Nesterov <oleg@redhat.com> - 2016-12-05 18:20 +0100

#1535322 — [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake to not use wait-queues

The use of any kind of wait queue is an overkill for pcpu-rwsems.
While one option would be to use the less heavy simple (swait)
flavor, this is still too much for what pcpu-rwsems needs. For one,
we do not care about any sort of queuing in that the only (rare) time
writers (and readers, for that matter) are queued is when trying to
acquire the regular contended rw_sem. There cannot be any further
queuing as writers are serialized by the rw_sem in the first place.

This patch, therefore, implements custom wait/wake, with an rcu-aware
writer task pointer. The only time this is !nil is when a writer is
determining if it is going to block, and reset as soon as we know that
the percpu_down_write() call has succeeded. All this is obviously done while
holding the regular rw_sem. As such, we can avoid the queue handling and
locking overhead (although we currently end up taking the waitqueue
spinlock fastpath, so it wouldn't be a very big an impact).

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 include/linux/percpu-rwsem.h  |  5 ++---
 kernel/locking/percpu-rwsem.c | 26 +++++++++++++++++++++-----
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
index 5b2e6159b744..9942b7e8bde8 100644
--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -4,7 +4,6 @@
 #include <linux/atomic.h>
 #include <linux/rwsem.h>
 #include <linux/percpu.h>
-#include <linux/wait.h>
 #include <linux/rcu_sync.h>
 #include <linux/lockdep.h>
 
@@ -12,7 +11,7 @@ struct percpu_rw_semaphore {
 	struct rcu_sync		rss;
 	unsigned int __percpu	*read_count;
 	struct rw_semaphore	rw_sem;
-	wait_queue_head_t	writer;
+	struct task_struct      *writer; /* blocked writer */
 	int			readers_block;
 };
 
@@ -22,7 +21,7 @@ static struct percpu_rw_semaphore name = {				\
 	.rss = __RCU_SYNC_INITIALIZER(name.rss, RCU_SCHED_SYNC),	\
 	.read_count = &__percpu_rwsem_rc_##name,			\
 	.rw_sem = __RWSEM_INITIALIZER(name.rw_sem),			\
-	.writer = __WAIT_QUEUE_HEAD_INITIALIZER(name.writer),		\
+	.writer = NULL,							\
 }
 
 extern int __percpu_down_read(struct percpu_rw_semaphore *, int);
diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index ce182599cf2e..7856a77396d3 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -1,7 +1,6 @@
 #include <linux/atomic.h>
 #include <linux/rwsem.h>
 #include <linux/percpu.h>
-#include <linux/wait.h>
 #include <linux/lockdep.h>
 #include <linux/percpu-rwsem.h>
 #include <linux/rcupdate.h>
@@ -18,7 +17,7 @@ int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
 	/* ->rw_sem represents the whole percpu_rw_semaphore for lockdep */
 	rcu_sync_init(&sem->rss, RCU_SCHED_SYNC);
 	__init_rwsem(&sem->rw_sem, name, rwsem_key);
-	init_waitqueue_head(&sem->writer);
+	sem->writer = NULL;
 	sem->readers_block = 0;
 	return 0;
 }
@@ -94,6 +93,8 @@ EXPORT_SYMBOL_GPL(__percpu_down_read);
 
 void __percpu_up_read(struct percpu_rw_semaphore *sem)
 {
+	struct task_struct *writer;
+
 	smp_mb(); /* B matches C */
 	/*
 	 * In other words, if they see our decrement (presumably to aggregate
@@ -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();
 }
 EXPORT_SYMBOL_GPL(__percpu_up_read);
 
@@ -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;
+
+		schedule();
+	}
+
+	rcu_assign_pointer(sem->writer, NULL);
+	__set_current_state(TASK_RUNNING);
 }
 EXPORT_SYMBOL_GPL(percpu_down_write);
 
-- 
2.6.6

[toc] | [next] | [standalone]

#1535893 — Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake to not use wait-queues

On Fri, Dec 02, 2016 at 06:18:39PM -0800, 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);

Don't think this is correct, I think Oleg suggested using
task_rcu_dereference(), which is a giant pile of magic.

The problem is that task_struct isn't RCU protected as such.

> +
> 	/* Prod writer to recheck readers_active */
> -	wake_up(&sem->writer);
> +	if (writer)
> +		wake_up_process(writer);
> +	rcu_read_unlock();
> }
> EXPORT_SYMBOL_GPL(__percpu_up_read);
> 
> @@ -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);

So this one matches rcu_dereference(), which is weird, because you now
have unmatched barriers.

> +	for (;;) {
> +		set_current_state(TASK_UNINTERRUPTIBLE);
> +
> +		if (readers_active_check(sem))
> +			break;
> +
> +		schedule();
> +	}
> +
> +	rcu_assign_pointer(sem->writer, NULL);

And this one does not, and the value being NULL this actually reverts to
WRITE_ONCE().

> +	__set_current_state(TASK_RUNNING);
> }
> EXPORT_SYMBOL_GPL(percpu_down_write);

[toc] | [prev] | [next] | [standalone]

#1536010 — Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake to not use wait-queues

Davidlohr, Peter, I'll try to read this patch later, just one note.

On 12/05, Peter Zijlstra wrote:
>
> On Fri, Dec 02, 2016 at 06:18:39PM -0800, 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);
>
> Don't think this is correct, I think Oleg suggested using
> task_rcu_dereference(), which is a giant pile of magic.

Yes, but on a second thought task_rcu_dereference() won't really help,
but we can just use rcu_dereference().

> The problem is that task_struct isn't RCU protected as such.

Yes. But percpu_down_write() should not be used after exit_notify(), so we
can rely on rcu_read_lock(), release_task()->call_rcu(delayed_put_task_struct)
can't be called until an exiting task passes exit_notify().

But then we probably need WARN_ON(current->exit_state) in percpu_down_write().

And personally I think this change should add the new helpers, they can have
more users. Something like

	struct xxx {
		struct task_struct *task;
	};

	xxx_wake_up(struct xxx *xxx)
	{
		rcu_read_lock();
		task = rcu_dereference(xxx->task);
		if (task)
			wake_up_process(task);
		rcu_read_unlock();
	}


	#define xxx_wait_event(xxx, event) {
		// comment to explain why
		WARN_ON(current->exit_state);

		xxx->task = current;

		...
	}

Oleg.

[toc] | [prev] | [next] | [standalone]

#1536020 — Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake to not use wait-queues

On 12/05, Oleg Nesterov wrote:
>
> Yes, but on a second thought task_rcu_dereference() won't really help,

I forgot to explain why, see below.

> 	#define xxx_wait_event(xxx, event) {
> 		// comment to explain why
> 		WARN_ON(current->exit_state);

Otherwise this process/thread can be already (auto)reaped and wakeup
can't rely on rcu.

And task_rcu_dereference() can't help because it can return NULL in
this case.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1536267 — Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake to not use wait-queues

On Mon, 05 Dec 2016, Oleg Nesterov wrote:

>Yes. But percpu_down_write() should not be used after exit_notify(), so we
>can rely on rcu_read_lock(), release_task()->call_rcu(delayed_put_task_struct)
>can't be called until an exiting task passes exit_notify().
>
>But then we probably need WARN_ON(current->exit_state) in percpu_down_write().

Hmm, my immediate thought would have been doing a PF_EXITING check, but of course
this enlarges the window of the warn being triggered, yet maintains what you are
saying in that percpu_down_write should not be used after do_exit/exit_notify.
Furthermore, reading the comment in task_rcu_dereference, I get your point and
we can loose the reference to the task, iow busted rcu read cr.

>
>And personally I think this change should add the new helpers, they can have
>more users. Something like
>
>	struct xxx {

What do you think of s/xxx/rcuwait?

Thanks,
Davidlohr

[toc] | [prev] | [next] | [standalone]

#1536252 — Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake to not use wait-queues

On 12/05, Peter Zijlstra wrote:
>
> > +	for (;;) {
> > +		set_current_state(TASK_UNINTERRUPTIBLE);
> > +
> > +		if (readers_active_check(sem))
> > +			break;
> > +
> > +		schedule();
> > +	}
> > +
> > +	rcu_assign_pointer(sem->writer, NULL);
>
> And this one does not, and the value being NULL this actually reverts to
> WRITE_ONCE().

Do we really care? We do not even need WRITE_ONCE() afaics, this is like
__set_current_state(TASK_RUNNING) after the main loop. We can't avoid the
spurious wakeups anyway after return from percpu_down_write().

Oleg.

[toc] | [prev] | [next] | [standalone]

#1536254 — Re: [PATCH v2 2/3] locking/percpu-rwsem: Rework writer block/wake to not use wait-queues

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web