Re: [PATCH 0/7] Add rcu_sync infrastructure to avoid _expedited() in percpu-rwsem

[Oleg Nesterov <oleg@redhat.com>]

On 07/15, Paul E. McKenney wrote:
>
> On Sun, Jul 12, 2015 at 01:35:35AM +0200, Oleg Nesterov wrote:
>
> > Let's start with the simple test-case,
> >
> > 	#!/bin/bash
> >
> > 	perf probe -x /lib/libc.so.6 syscall
> >
> > 	for i in {1..1000}; do
> > 		echo 1 >| /sys/kernel/debug/tracing/events/probe_libc/syscall/enable
> > 		echo 0 >| /sys/kernel/debug/tracing/events/probe_libc/syscall/enable
> > 	done
> >
> > It needs ~ 13.5 seconds (2 CPUs, KVM). If we simply replace
> > synchronize_sched_expedited() with synchronize_sched() it takes
> > ~ 67.5 seconds. This is not good.
>
> Yep, even if you avoided the write-release grace period, you would
> still be looking at something like 40 seconds, which is 3x.  Some
> might consider that to be a performance regression.  ;-)

Yes ;)

> > And just in case, I also measured
> >
> > 	for (i = 0; i < 1000000; ++i) {
> > 		percpu_down_write(&dup_mmap_sem);
> > 		percpu_up_write(&dup_mmap_sem);
> > 	}
> >
> > and it runs more than 1.5 times faster (to remind, only 2 CPUs),
> > but this is not that interesting, I agree.
>
> Your trick avoiding the grace periods during a writer-to-writer handoff
> are cute, and they are helping a lot here.

Yes. And even the fact that a single writer doesn't need to sleep in
percpu_up_write()->synchronize_sched() looks good imo.

Yes, yes, we can remove it if we penalize the readers, but I'd like to
avoid this.

> Concurrent readers would
> have a tough time of it with this workload, though.  They would all
> be serialized.

Sure. in this case it is not better than the normal rw_semaphore. Worse
actually.

> > And note that the actual change in percpu-rwsem is really simple,
> > and imo it even makes the code simpler. (the last patch is off-
> > topic cleanup).
> >
> > So the only complication is rcu_sync itself. But, rightly or not (I
> > am obviously biased), I believe this new rcu infrastructure is natural
> > and useful, and I think it can have more users too.
>
> I don't have an objection to it, even in its current form (I did
> review it long ago), but it does need to have a user!

Do you mean you need another user except percpu_rw_semaphore? I do
not see any right now...

Let me remind about sb_writers again. It actually has 3 rw_sem's
and I am trying to turn then into percpu_rw_semaphore's.

In this case freeze_super() will need 6 synchronize_sched_expedited().
This just looks ugly. But if we have rcu_sync primitives, all 3 sem's
in struct super_block can share the same "struct rcu_sync", and
freeze_super() will need only once synchronize_sched().

> > And. We can do more improvements in rcu_sync and percpu-rwsem, and
> > I don't only mean other optimizations from Peter. In particular, we
> > can extract the "wait for gp pass" from rcu_sync_enter() into another
> > helper, we can teach percpu_down_write() to allow multiple writers,
> > and more.
>
> As in a percpu_down_write() that allows up to (say) five concurrent
> write-holders?

Yes. Because this is what uprobes (and probably cgroups) actually needs.
It does not need the global lock. Just it needs to exclude the "readers"
(dup_mmap) globally.

And in fact the very first version I sent worked this way. Then I removed
this because a) this was a bit "unusual" for reviewers ;) and b) because
I raced with another commit which has already added the initial (and
sub-optimal) version of percpu_rw_semaphore.

Oleg.