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

Groups > linux.kernel > #1525635 > unrolled thread

[PATCH -tip 0/3] locking/percpu-rwsem: writer-side optimizations

Back to article view | Back to linux.kernel

Contents

  [PATCH -tip 0/3] locking/percpu-rwsem: writer-side optimizations Davidlohr Bueso <dave@stgolabs.net> - 2016-11-18 20:00 +0100
    [PATCH 1/3] locking/percpu-rwsem: Move text file into Documentation/locking/ Davidlohr Bueso <dave@stgolabs.net> - 2016-11-18 20:00 +0100
    [PATCH 2/3] locking/percpu-rwsem: Replace bulky wait-queues with swait Davidlohr Bueso <dave@stgolabs.net> - 2016-11-18 20:00 +0100
      Re: [PATCH 2/3] locking/percpu-rwsem: Replace bulky wait-queues         with swait Oleg Nesterov <oleg@redhat.com> - 2016-11-21 14:00 +0100
        Re: [PATCH 2/3] locking/percpu-rwsem: Replace bulky wait-queues with  swait Davidlohr Bueso <dave@stgolabs.net> - 2016-11-21 18:30 +0100
    [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer wakeups Davidlohr Bueso <dave@stgolabs.net> - 2016-11-18 20:00 +0100
      Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer  wakeups Peter Zijlstra <peterz@infradead.org> - 2016-11-21 13:30 +0100
        Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer         wakeups Oleg Nesterov <oleg@redhat.com> - 2016-11-21 13:50 +0100
          Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer         wakeups Oleg Nesterov <oleg@redhat.com> - 2016-11-21 16:10 +0100
            Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer  wakeups Davidlohr Bueso <dave@stgolabs.net> - 2016-11-22 05:00 +0100
              Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer         wakeups Oleg Nesterov <oleg@redhat.com> - 2016-11-23 15:50 +0100
      Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer         wakeups Oleg Nesterov <oleg@redhat.com> - 2016-11-21 13:30 +0100

#1525635 — [PATCH -tip 0/3] locking/percpu-rwsem: writer-side optimizations

Hi,

Here are two updates intended for those rare updater cases.

wrt numbers, I'm still trying to figure out this android boot environment
(that calls cgroup_proc_write ipc paths) which might care about these
changes. In the mean time, I'd like to know if anyone has any objections
to these optimizations.

Passed kernel builds with lockdep, as well as an overnight run doing
locktoture.

Thanks.

Davidlohr Bueso (3):
  locking/percpu-rwsem: Move text file into Documentation/locking/
  locking/percpu-rwsem: Replace bulky wait-queues with swait
  locking/percpu-rwsem: Avoid unnecessary writer wakeups

 Documentation/locking/percpu-rw-semaphore.txt | 27 ++++++++++
 Documentation/percpu-rw-semaphore.txt         | 27 ----------
 include/linux/percpu-rwsem.h                  |  6 +--
 kernel/locking/percpu-rwsem.c                 | 78 +++++++++++++++------------
 4 files changed, 73 insertions(+), 65 deletions(-)
 create mode 100644 Documentation/locking/percpu-rw-semaphore.txt
 delete mode 100644 Documentation/percpu-rw-semaphore.txt

-- 
2.6.6

[toc] | [next] | [standalone]

#1525637 — [PATCH 1/3] locking/percpu-rwsem: Move text file into Documentation/locking/

Although this is rather useless and the actual code is orders
of magnitude more detailed and informative than this document.

Might as well keep it instead, I guess its already there and
could give a user a quick why/when use them vs regular rwsems.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 Documentation/locking/percpu-rw-semaphore.txt | 27 +++++++++++++++++++++++++++
 Documentation/percpu-rw-semaphore.txt         | 27 ---------------------------
 2 files changed, 27 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/locking/percpu-rw-semaphore.txt
 delete mode 100644 Documentation/percpu-rw-semaphore.txt

diff --git a/Documentation/locking/percpu-rw-semaphore.txt b/Documentation/locking/percpu-rw-semaphore.txt
new file mode 100644
index 000000000000..7d3c82431909
--- /dev/null
+++ b/Documentation/locking/percpu-rw-semaphore.txt
@@ -0,0 +1,27 @@
+Percpu rw semaphores
+--------------------
+
+Percpu rw semaphores is a new read-write semaphore design that is
+optimized for locking for reading.
+
+The problem with traditional read-write semaphores is that when multiple
+cores take the lock for reading, the cache line containing the semaphore
+is bouncing between L1 caches of the cores, causing performance
+degradation.
+
+Locking for reading is very fast, it uses RCU and it avoids any atomic
+instruction in the lock and unlock path. On the other hand, locking for
+writing is very expensive, it calls synchronize_rcu() that can take
+hundreds of milliseconds.
+
+The lock is declared with "struct percpu_rw_semaphore" type.
+The lock is initialized percpu_init_rwsem, it returns 0 on success and
+-ENOMEM on allocation failure.
+The lock must be freed with percpu_free_rwsem to avoid memory leak.
+
+The lock is locked for read with percpu_down_read, percpu_up_read and
+for write with percpu_down_write, percpu_up_write.
+
+The idea of using RCU for optimized rw-lock was introduced by
+Eric Dumazet <eric.dumazet@gmail.com>.
+The code was written by Mikulas Patocka <mpatocka@redhat.com>
diff --git a/Documentation/percpu-rw-semaphore.txt b/Documentation/percpu-rw-semaphore.txt
deleted file mode 100644
index 7d3c82431909..000000000000
--- a/Documentation/percpu-rw-semaphore.txt
+++ /dev/null
@@ -1,27 +0,0 @@
-Percpu rw semaphores
---------------------
-
-Percpu rw semaphores is a new read-write semaphore design that is
-optimized for locking for reading.
-
-The problem with traditional read-write semaphores is that when multiple
-cores take the lock for reading, the cache line containing the semaphore
-is bouncing between L1 caches of the cores, causing performance
-degradation.
-
-Locking for reading is very fast, it uses RCU and it avoids any atomic
-instruction in the lock and unlock path. On the other hand, locking for
-writing is very expensive, it calls synchronize_rcu() that can take
-hundreds of milliseconds.
-
-The lock is declared with "struct percpu_rw_semaphore" type.
-The lock is initialized percpu_init_rwsem, it returns 0 on success and
--ENOMEM on allocation failure.
-The lock must be freed with percpu_free_rwsem to avoid memory leak.
-
-The lock is locked for read with percpu_down_read, percpu_up_read and
-for write with percpu_down_write, percpu_up_write.
-
-The idea of using RCU for optimized rw-lock was introduced by
-Eric Dumazet <eric.dumazet@gmail.com>.
-The code was written by Mikulas Patocka <mpatocka@redhat.com>
-- 
2.6.6

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

#1525644 — [PATCH 2/3] locking/percpu-rwsem: Replace bulky wait-queues with swait

In the case of the percpu-rwsem, they don't need any of the
fancy/bulky features, such as custom callbacks or fine grained
wakeups.

Users that can convert to simple wait-queues are encouraged to
do so for the various rt and (indirect) performance benefits.

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

diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
index 5b2e6159b744..82d54a4b9988 100644
--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -4,7 +4,7 @@
 #include <linux/atomic.h>
 #include <linux/rwsem.h>
 #include <linux/percpu.h>
-#include <linux/wait.h>
+#include <linux/swait.h>
 #include <linux/rcu_sync.h>
 #include <linux/lockdep.h>
 
@@ -12,7 +12,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 swait_queue_head	writer;
 	int			readers_block;
 };
 
@@ -22,7 +22,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 = __SWAIT_QUEUE_HEAD_INITIALIZER(name.writer),		\
 }
 
 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..cb71201855f2 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -1,7 +1,7 @@
 #include <linux/atomic.h>
 #include <linux/rwsem.h>
 #include <linux/percpu.h>
-#include <linux/wait.h>
+#include <linux/swait.h>
 #include <linux/lockdep.h>
 #include <linux/percpu-rwsem.h>
 #include <linux/rcupdate.h>
@@ -18,7 +18,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);
+	init_swait_queue_head(&sem->writer);
 	sem->readers_block = 0;
 	return 0;
 }
@@ -103,7 +103,7 @@ void __percpu_up_read(struct percpu_rw_semaphore *sem)
 	__this_cpu_dec(*sem->read_count);
 
 	/* Prod writer to recheck readers_active */
-	wake_up(&sem->writer);
+	swake_up(&sem->writer);
 }
 EXPORT_SYMBOL_GPL(__percpu_up_read);
 
@@ -160,7 +160,7 @@ void percpu_down_write(struct percpu_rw_semaphore *sem)
 	 */
 
 	/* Wait for all now active readers to complete. */
-	wait_event(sem->writer, readers_active_check(sem));
+	swait_event(sem->writer, readers_active_check(sem));
 }
 EXPORT_SYMBOL_GPL(percpu_down_write);
 
-- 
2.6.6

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

#1526652 — Re: [PATCH 2/3] locking/percpu-rwsem: Replace bulky wait-queues with swait

On 11/18, Davidlohr Bueso wrote:
>
> @@ -12,7 +12,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 swait_queue_head	writer;

I won't argue, but even swait_queue_head is overkill in this case.

We can just add "struct task_struct *writer" into percpu_rw_semaphore,

__percpu_up_read:

	rcu_read_lock();
	writer = task_rcu_dereference(&sem->writer);
	if (writer)
		wake_up_process(writer);
	rcu_read_unlock();

percpu_down_write() can set sem->writer == current and do the simple
while-not-condition-schedule() loop.

But this probably needs a couple of new helpers, and probably they
can have more users.

Oleg.

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

#1526942 — Re: [PATCH 2/3] locking/percpu-rwsem: Replace bulky wait-queues with swait

On Mon, 21 Nov 2016, Oleg Nesterov wrote:

>On 11/18, Davidlohr Bueso wrote:
>>
>> @@ -12,7 +12,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 swait_queue_head	writer;
>
>I won't argue, but even swait_queue_head is overkill in this case.
>
>We can just add "struct task_struct *writer" into percpu_rw_semaphore,

Given that this is how all things locking/ work, I very much agree with
you, lemme send a v2.

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

#1525651 — [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer wakeups

There is obviously no point in doing the wakeup if the
reader wakee task can do the active readers check on
behalf of the writer on its way to unlocking itself.

The downside is that when readers_active_check() does
return true we end up iterating the per-CPU counter twice.
This trade-off, however, does seem reasonable in that if
we are here, (i) we have already lost any hope for reader
side performance and; (ii) because this lock is used mainly
for sharing, it is not crazy to expect to have readers
incoming for the lock during this window -- therefore
sending writers right back to sleep for every reader up().

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 kernel/locking/percpu-rwsem.c | 72 ++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 32 deletions(-)

diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index cb71201855f2..8e6fbf117f14 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -8,6 +8,44 @@
 #include <linux/sched.h>
 #include <linux/errno.h>
 
+#define per_cpu_sum(var)						\
+({									\
+	typeof(var) __sum = 0;						\
+	int cpu;							\
+	compiletime_assert_atomic_type(__sum);				\
+	for_each_possible_cpu(cpu)					\
+		__sum += per_cpu(var, cpu);				\
+	__sum;								\
+})
+
+/*
+ * Return true if the modular sum of the sem->read_count per-CPU variable is
+ * 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.
+ */
+static bool readers_active_check(struct percpu_rw_semaphore *sem)
+{
+	if (per_cpu_sum(*sem->read_count) != 0)
+		return false;
+
+	/*
+	 * If we observed the decrement; ensure we see the entire critical
+	 * section. In the case of __readers_active_check we avoid the
+	 * critical section sync, as the writer wakee will fully re-check
+	 * to continue.
+	 */
+
+	smp_mb(); /* C matches B */
+
+	return true;
+}
+
+static bool __readers_active_check(struct percpu_rw_semaphore *sem)
+{
+	return !(per_cpu_sum(*sem->read_count) !=0);
+}
+
 int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
 			const char *name, struct lock_class_key *rwsem_key)
 {
@@ -103,41 +141,11 @@ void __percpu_up_read(struct percpu_rw_semaphore *sem)
 	__this_cpu_dec(*sem->read_count);
 
 	/* Prod writer to recheck readers_active */
-	swake_up(&sem->writer);
+	if (__readers_active_check(sem))
+		swake_up(&sem->writer);
 }
 EXPORT_SYMBOL_GPL(__percpu_up_read);
 
-#define per_cpu_sum(var)						\
-({									\
-	typeof(var) __sum = 0;						\
-	int cpu;							\
-	compiletime_assert_atomic_type(__sum);				\
-	for_each_possible_cpu(cpu)					\
-		__sum += per_cpu(var, cpu);				\
-	__sum;								\
-})
-
-/*
- * Return true if the modular sum of the sem->read_count per-CPU variable is
- * 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.
- */
-static bool readers_active_check(struct percpu_rw_semaphore *sem)
-{
-	if (per_cpu_sum(*sem->read_count) != 0)
-		return false;
-
-	/*
-	 * If we observed the decrement; ensure we see the entire critical
-	 * section.
-	 */
-
-	smp_mb(); /* C matches B */
-
-	return true;
-}
-
 void percpu_down_write(struct percpu_rw_semaphore *sem)
 {
 	/* Notify readers to take the slow path. */
-- 
2.6.6

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

#1526625 — Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer wakeups

On Mon, Nov 21, 2016 at 01:23:44PM +0100, Oleg Nesterov wrote:
> On 11/18, Davidlohr Bueso wrote:
> >
> > +static bool __readers_active_check(struct percpu_rw_semaphore *sem)
> > +{
> > +	return !(per_cpu_sum(*sem->read_count) !=0);
> > +}
> 
> Hmm,
> 
> 	return per_cpu_sum(*sem->read_count) == 0;
> 
> looks more clear, but this is minor,

Very much so; that must be one of the most convoluted statements
possible :-).

> 
> >  int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
> >  			const char *name, struct lock_class_key *rwsem_key)
> >  {
> > @@ -103,41 +141,11 @@ void __percpu_up_read(struct percpu_rw_semaphore *sem)
> >  	__this_cpu_dec(*sem->read_count);
> >  
> >  	/* Prod writer to recheck readers_active */
> > -	swake_up(&sem->writer);
> > +	if (__readers_active_check(sem))
> > +		swake_up(&sem->writer);
> 
> Suppose we have 2 active readers which call __percpu_up_read() at the same
> time and the pending writer sleeps.
> 
> What guarantees that one of these readers will observe per_cpu_sum() == 0 ?
> They both can read the old value of the remote per-cpu counter, no?

In particular, you're thinking of what provides the guarantee that the
woken CPU observes the same state the wakee saw? Isn't this one of the
Program-Order guarantees the scheduler _should_ provide?

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

#1526638 — Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer wakeups

On 11/21, Peter Zijlstra wrote:
>
> On Mon, Nov 21, 2016 at 01:23:44PM +0100, Oleg Nesterov wrote:
> > >  int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
> > >  			const char *name, struct lock_class_key *rwsem_key)
> > >  {
> > > @@ -103,41 +141,11 @@ void __percpu_up_read(struct percpu_rw_semaphore *sem)
> > >  	__this_cpu_dec(*sem->read_count);
> > >
> > >  	/* Prod writer to recheck readers_active */
> > > -	swake_up(&sem->writer);
> > > +	if (__readers_active_check(sem))
> > > +		swake_up(&sem->writer);
> >
> > Suppose we have 2 active readers which call __percpu_up_read() at the same
> > time and the pending writer sleeps.
> >
> > What guarantees that one of these readers will observe per_cpu_sum() == 0 ?
> > They both can read the old value of the remote per-cpu counter, no?
>
> In particular, you're thinking of what provides the guarantee that the
> woken CPU observes the same state the wakee saw?

No, no, I meant that afaics both readers can see per_cpu_sum() != 0 and
thus the writer won't be woken up. Till the next down_read/up_read.

Suppose that we have 2 CPU's, both counters == 1, both readers decrement.
its counter at the same time.

	READER_ON_CPU_0			READER_ON_CPU_1

	--ctr_0;			--ctr_1;

	if (ctr_0 + ctr_1)		if (ctr_0 + ctr_1)
		wakeup();			wakeup();

Why we can't miss a wakeup?

This patch doesn't even add a barrier, but I think wmb() won't be enough
anyway.

Oleg.

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

#1526753 — Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer wakeups

On 11/21, Oleg Nesterov wrote:
>
> No, no, I meant that afaics both readers can see per_cpu_sum() != 0 and
> thus the writer won't be woken up. Till the next down_read/up_read.
>
> Suppose that we have 2 CPU's, both counters == 1, both readers decrement.
> its counter at the same time.
>
> 	READER_ON_CPU_0			READER_ON_CPU_1
>
> 	--ctr_0;			--ctr_1;
>
> 	if (ctr_0 + ctr_1)		if (ctr_0 + ctr_1)
> 		wakeup();			wakeup();
>
> Why we can't miss a wakeup?
>
> This patch doesn't even add a barrier, but I think wmb() won't be enough
> anyway.

And in fact I am not sure this optimization makes sense... But it would be
nice to avoid wake_up() when the writer sleeps in rcu_sync_enter(). Or this
is the "slow mode" sem (cgroup_threadgroup_rwsem).

I need to re-check, but what do you think about the change below?

Oleg.

--- x/kernel/locking/percpu-rwsem.c
+++ x/kernel/locking/percpu-rwsem.c
@@ -103,7 +103,9 @@ void __percpu_up_read(struct percpu_rw_s
 	__this_cpu_dec(*sem->read_count);
 
 	/* Prod writer to recheck readers_active */
-	wake_up(&sem->writer);
+	smp_mb();
+	if (sem->readers_block)
+		wake_up(&sem->writer);
 }
 EXPORT_SYMBOL_GPL(__percpu_up_read);
 

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

#1527212 — Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer wakeups

On Mon, 21 Nov 2016, Oleg Nesterov wrote:

>On 11/21, Oleg Nesterov wrote:
>>
>> No, no, I meant that afaics both readers can see per_cpu_sum() != 0 and
>> thus the writer won't be woken up. Till the next down_read/up_read.
>>
>> Suppose that we have 2 CPU's, both counters == 1, both readers decrement.
>> its counter at the same time.
>>
>> 	READER_ON_CPU_0			READER_ON_CPU_1
>>
>> 	--ctr_0;			--ctr_1;
>>
>> 	if (ctr_0 + ctr_1)		if (ctr_0 + ctr_1)
>> 		wakeup();			wakeup();
>>
>> Why we can't miss a wakeup?

But the patch is really: if (!(ctr_0 + ctr_1)). wrt to stale values is this
like due to the data dependency we only see the real value of this_cpu ctr,
and no guarantee for the other cpus? If so I had not considered that scenario,
and yes we'd need stronger guarantees.

I'd have to wonder if other users of per_cpu_sum() would fall into a similar
trap. Hmm and each user seems to implement its own copy of the same thing.

>And in fact I am not sure this optimization makes sense... But it would be
>nice to avoid wake_up() when the writer sleeps in rcu_sync_enter(). Or this
>is the "slow mode" sem (cgroup_threadgroup_rwsem).

Why do you think using per_cpu_sum() does not make sense? As mentioned in the
changelog it optimizes for incoming readers while the writer is doing sync_enter
and getting the regular rwsem. What am I missing?

>
>I need to re-check, but what do you think about the change below?

While optimizing for multiple writers (rcu_sync_enter) is certainly valid
(at least considering the cgroups rwsem you mention), I think that my
heuristic covers the otherwise more common case. Could both optimizations
not work together?

Of course, the window of where readers_block == 1 is quite large, so there
can be a lot of false positives.

Thanks,
Davidlohr

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

#1528463 — Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer wakeups

On 11/21, Davidlohr Bueso wrote:
>
> On Mon, 21 Nov 2016, Oleg Nesterov wrote:
>
>> On 11/21, Oleg Nesterov wrote:
>>>
>>> No, no, I meant that afaics both readers can see per_cpu_sum() != 0 and
>>> thus the writer won't be woken up. Till the next down_read/up_read.
>>>
>>> Suppose that we have 2 CPU's, both counters == 1, both readers decrement.
>>> its counter at the same time.
>>>
>>> 	READER_ON_CPU_0			READER_ON_CPU_1
>>>
>>> 	--ctr_0;			--ctr_1;
>>>
>>> 	if (ctr_0 + ctr_1)		if (ctr_0 + ctr_1)
>>> 		wakeup();			wakeup();
>>>
>>> Why we can't miss a wakeup?
>
> But the patch is really: if (!(ctr_0 + ctr_1)).

Of course, I meant if (ctr_0 + ctr_1 == 0).

>> And in fact I am not sure this optimization makes sense... But it would be
>> nice to avoid wake_up() when the writer sleeps in rcu_sync_enter(). Or this
>> is the "slow mode" sem (cgroup_threadgroup_rwsem).
>
> Why do you think using per_cpu_sum() does not make sense? As mentioned in the
> changelog it optimizes for incoming readers while the writer is doing sync_enter
> and getting the regular rwsem. What am I missing?

And this does make sense, but see below,

>> I need to re-check, but what do you think about the change below?
>
> While optimizing for multiple writers (rcu_sync_enter) is certainly valid
> (at least considering the cgroups rwsem you mention),

No, it is not for multiple writers. rcu_sync_enter() is slow, the new
readers can come and acquire/release this lock. And if it is a "slow mode"
sem then every up() does wakeup which we want to eliminate.

But after sem->readers_block is already true, I am not sure the additional
per_cpu_sum() is a win (even if it was correct), the new readers can't come.

Except __percpu_down_read()->__percpu_up_read() which we want to optimize
too, but in this case we do not need per_cpu_sum() too.

I'll try to make a patch this week... I had this optimization in mind from
the very beginning, I event mentioned it during the last discussion, but
never had time. Basically we should not inc if readers_block == T.

Oleg.

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

#1526627 — Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer wakeups

On 11/18, Davidlohr Bueso wrote:
>
> +static bool __readers_active_check(struct percpu_rw_semaphore *sem)
> +{
> +	return !(per_cpu_sum(*sem->read_count) !=0);
> +}

Hmm,

	return per_cpu_sum(*sem->read_count) == 0;

looks more clear, but this is minor,

>  int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
>  			const char *name, struct lock_class_key *rwsem_key)
>  {
> @@ -103,41 +141,11 @@ void __percpu_up_read(struct percpu_rw_semaphore *sem)
>  	__this_cpu_dec(*sem->read_count);
>  
>  	/* Prod writer to recheck readers_active */
> -	swake_up(&sem->writer);
> +	if (__readers_active_check(sem))
> +		swake_up(&sem->writer);

Suppose we have 2 active readers which call __percpu_up_read() at the same
time and the pending writer sleeps.

What guarantees that one of these readers will observe per_cpu_sum() == 0 ?
They both can read the old value of the remote per-cpu counter, no?

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web