[4.2] commit d59cfc09c32 (sched, cgroup: replace signal_struct->group_rwsem with a global percpu_rwsem) causes regression for libvirt/kvm

PATCH In review Christian Borntraeger 6 participants 31 messages Sep 15, 2015 – Sep 16, 2015

Subsystems (7)

KVM MM core Scheduler exec and binfmt +3 more subsystems

lore

Share & tools

lore

Christian Borntraeger

author

2015-09-15 12:05 UTC #0 lore raw

Tejun,

commit d59cfc09c32a2ae31f1c3bc2983a0cd79afb3f14 (sched, cgroup: replace signal_struct->group_rwsem with a global percpu_rwsem) causes some noticably hickups when starting several kvm guests (which libvirt will move into cgroups
- each vcpu thread and each i/o thread)
When you now start lots of guests in parallel on a bigger system (32CPUs with 2way smt in my case) the system is so busy that systemd runs into several timeouts like "Did not receive a reply. Possible causes include: the remote application did not send a reply, the message bus security policy blocked the reply, the reply timeout expired, or the network connection was broken."

The problem seems to be that the newly used percpu_rwsem does a rcu_synchronize_sched_expedited for all write downs/ups.

Hacking the percpu_rw_semaphore to always go the slow path and avoid the synchronize_sched seems to fix the issue. For some (yet unknown to me) reason the synchronize_sched and the fast path seems to block writers for incredibly long times.

To trigger the problem, the guest must be CPU bound, iow idle guests seem to not trigger the pathological case. Any idea how to improve the situation, this looks like a real regression for larger kvm installations.

Christian

Peter Zijlstra

maintainer

2015-09-15 13:05 UTC #1 lore raw

Quoted 13 lines · Christian Borntraeger

Tejun,
commit d59cfc09c32a2ae31f1c3bc2983a0cd79afb3f14 (sched, cgroup: replace signal_struct->group_rwsem with a global percpu_rwsem) causes some noticably hickups when starting several kvm guests (which libvirt will move into cgroups
- each vcpu thread and each i/o thread)
When you now start lots of guests in parallel on a bigger system (32CPUs with 2way smt in my case) the system is so busy that systemd runs into several timeouts like "Did not receive a reply. Possible causes include: the remote application did not send a reply, the message bus security policy blocked the reply, the reply timeout expired, or the network connection was broken."

Quoted 2 lines

The problem seems to be that the newly used percpu_rwsem does a rcu_synchronize_sched_expedited for all write downs/ups.

Can you try:

git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev.2015.09.11ab

those include Oleg's rework of the percpu rwsem which should hopefully improve things somewhat.

But yes, pounding a global lock on a big machine will always suck.

Christian Borntraeger

author

2015-09-15 13:36 UTC #2 lore raw

Am 15.09.2015 um 15:05 schrieb Peter Zijlstra:

Quoted 14 lines · Christian Borntraeger

Quoted 13 lines
Tejun,
commit d59cfc09c32a2ae31f1c3bc2983a0cd79afb3f14 (sched, cgroup: replace signal_struct->group_rwsem with a global percpu_rwsem) causes some noticably hickups when starting several kvm guests (which libvirt will move into cgroups
- each vcpu thread and each i/o thread)
When you now start lots of guests in parallel on a bigger system (32CPUs with 2way smt in my case) the system is so busy that systemd runs into several timeouts like "Did not receive a reply. Possible causes include: the remote application did not send a reply, the message bus security policy blocked the reply, the reply timeout expired, or the network connection was broken."

Quoted 6 lines

Quoted 2 lines
The problem seems to be that the newly used percpu_rwsem does a rcu_synchronize_sched_expedited for all write downs/ups.
Can you try:
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev.2015.09.11ab

yes, dev.2015.09.11a seems to help, thanks. Getting rid of the expedited hammer was really helpful - I guess.

Quoted 5 lines

those include Oleg's rework of the percpu rwsem which should hopefully improve things somewhat.
But yes, pounding a global lock on a big machine will always suck.

By hacking out the fast path I actually degraded percpu rwsem to a real global lock, but things were still a lot faster.
I am wondering why the old code behaved in such fatal ways. Is there some interaction between waiting for a reschedule in the synchronize_sched writer and some fork code actually waiting for the read side to get the lock together with some rescheduling going on waiting for a lock that fork holds? lockdep does not give me an hints so I have no clue :-(

Christian

Tejun Heo

maintainer

2015-09-15 13:53 UTC #3 lore raw

Hello,

Quoted 9 lines · Christian Borntraeger

Quoted 6 lines
Quoted 2 lines
The problem seems to be that the newly used percpu_rwsem does a rcu_synchronize_sched_expedited for all write downs/ups.
Can you try:
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev.2015.09.11ab
yes, dev.2015.09.11a seems to help, thanks. Getting rid of the expedited hammer was really helpful - I guess.

Ah, that's nice. I mentioned this in the original patchset but percpu_rwsem as previously implemented could be too heavy on the writer side for this path and I was planning to implement rwsem based lglock if this blows up. That said, if Oleg's changes makes the issue go away, all the better.

Quoted 11 lines

Quoted 4 lines
those include Oleg's rework of the percpu rwsem which should hopefully improve things somewhat.
But yes, pounding a global lock on a big machine will always suck.
By hacking out the fast path I actually degraded percpu rwsem to a real global lock, but things were still a lot faster.
I am wondering why the old code behaved in such fatal ways. Is there some interaction between waiting for a reschedule in the synchronize_sched writer and some fork code actually waiting for the read side to get the lock together with some rescheduling going on waiting for a lock that fork holds? lockdep does not give me an hints so I have no clue :-(

percpu_rwsem is a global lock. My rough suspicion is that probably the writer locking path was taking too long (especially if the kernel has preemption disabled) making the writers getting backed up badly starving the readers.

Thanks.

Signature · 1 line

tejun

Paolo Bonzini

maintainer

2015-09-15 16:42 UTC #4 lore raw

In reply to Christian Borntraeger

Quoted 6 lines · Christian Borntraeger

I am wondering why the old code behaved in such fatal ways. Is there some interaction between waiting for a reschedule in the synchronize_sched writer and some fork code actually waiting for the read side to get the lock together with some rescheduling going on waiting for a lock that fork holds? lockdep does not give me an hints so I have no clue :-(

It may just be consuming too much CPU usage. kernel/rcu/tree.c warns about it:

* if you are using synchronize_sched_expedited() in a loop, please
* restructure your code to batch your updates, and then use a single
* synchronize_sched() instead.

and you may remember that in KVM we switched from RCU to SRCU exactly to avoid userspace-controlled synchronize_rcu_expedited().

In fact, I would say that any userspace-controlled call to *_expedited() is a bug waiting to happen and a bad idea---because userspace can, with little effort, end up calling it in a loop.

Paolo

Paul E. McKenney

2015-09-15 17:38 UTC #5 lore raw

Quoted 17 lines · Paolo Bonzini

Quoted 6 lines · Christian Borntraeger
I am wondering why the old code behaved in such fatal ways. Is there some interaction between waiting for a reschedule in the synchronize_sched writer and some fork code actually waiting for the read side to get the lock together with some rescheduling going on waiting for a lock that fork holds? lockdep does not give me an hints so I have no clue :-(
It may just be consuming too much CPU usage. kernel/rcu/tree.c warns about it:
* if you are using synchronize_sched_expedited() in a loop, please
* restructure your code to batch your updates, and then use a single
* synchronize_sched() instead.

Quoted 6 lines

and you may remember that in KVM we switched from RCU to SRCU exactly to avoid userspace-controlled synchronize_rcu_expedited().
In fact, I would say that any userspace-controlled call to *_expedited() is a bug waiting to happen and a bad idea---because userspace can, with little effort, end up calling it in a loop.

Excellent points!

Other options in such situations include the following:

o Rework so that the code uses call_rcu*() instead of *_expedited().

o Maintain a per-task or per-CPU counter so that every so many
*_expedited() invocations instead uses the non-expedited counterpart. (For example, synchronize_rcu instead of synchronize_rcu_expedited().)

Note that synchronize_srcu_expedited() is less troublesome than are the other *_expedited() functions, because synchronize_srcu_expedited() does not inflict OS jitter on other CPUs. This situation is being improved, so that the other *_expedited() functions inflict less OS jitter and (mostly) avoid inflicting OS jitter on nohz_full CPUs and idle CPUs (the latter being important for battery-powered systems). In addition, the *_expedited() functions avoid hammering CPUs with N-squared OS jitter in response to concurrent invocation from all CPUs because multiple concurrent *_expedited() calls will be satisfied by a single expedited grace-period operation. Nevertheless, as Paolo points out, it is still necessary to exercise caution when exposing synchronous grace periods to userspace control.

Thanx, Paul

Paolo Bonzini

maintainer

2015-09-16 08:32 UTC #6 lore raw

Quoted 10 lines · Paul E. McKenney

Excellent points!
Other options in such situations include the following:
o Rework so that the code uses call_rcu*() instead of *_expedited().
o Maintain a per-task or per-CPU counter so that every so many
*_expedited() invocations instead uses the non-expedited counterpart. (For example, synchronize_rcu instead of synchronize_rcu_expedited().)

Or just use ratelimit (untested):

include/linux/percpu-rwsem.h +2 -0

index 834c4e52cb2d..8fb66b2aeed9 100644
@@ -6,6 +6,7 @@
 #include <linux/percpu.h>
 #include <linux/wait.h>
 #include <linux/lockdep.h>
+#include <linux/ratelimit.h>
 
 struct percpu_rw_semaphore {
 	unsigned int __percpu	*fast_read_ctr;

@@ -13,6 +14,7 @@ struct percpu_rw_semaphore {
 	struct rw_semaphore	rw_sem;
 	atomic_t		slow_read_ctr;
 	wait_queue_head_t	write_waitq;
+	struct ratelimit_state	expedited_ratelimit;
 };
 
 extern void percpu_down_read(struct percpu_rw_semaphore *);

kernel/locking/percpu-rwsem.c +10 -2

index f32567254867..c33f8bc89384 100644
@@ -20,6 +20,8 @@ int __percpu_init_rwsem(struct percpu_rw_semaphore *brw,
 	atomic_set(&brw->write_ctr, 0);
 	atomic_set(&brw->slow_read_ctr, 0);
 	init_waitqueue_head(&brw->write_waitq);
+	/* Expedite one down_write and one up_write per second.  */
+	ratelimit_state_init(&brw->expedited_ratelimit, HZ, 2);
 	return 0;
 }

@@ -152,7 +156,10 @@ void percpu_down_write(struct percpu_rw_semaphore *brw)
 	 *    fast-path, it executes a full memory barrier before we return.
 	 *    See R_W case in the comment above update_fast_ctr().
 	 */
-	synchronize_sched_expedited();
+	if (__ratelimit(&brw->expedited_ratelimit))
+		synchronize_sched_expedited();
+	else
+		synchronize_sched();
 
 	/* exclude other writers, and block the new readers completely */
 	down_write(&brw->rw_sem);

@@ -172,7 +179,10 @@ void percpu_up_write(struct percpu_rw_semaphore *brw)
 	 * Insert the barrier before the next fast-path in down_read,
 	 * see W_R case in the comment above update_fast_ctr().
 	 */
-	synchronize_sched_expedited();
+	if (__ratelimit(&brw->expedited_ratelimit))
+		synchronize_sched_expedited();
+	else
+		synchronize_sched();
 	/* the last writer unblocks update_fast_ctr() */
 	atomic_dec(&brw->write_ctr);
 }

Quoted 3 lines

Note that synchronize_srcu_expedited() is less troublesome than are the other *_expedited() functions, because synchronize_srcu_expedited() does not inflict OS jitter on other CPUs.

Yup, synchronize_srcu_expedited() is just a busy wait and it can complete extremely fast if you use SRCU as a "local RCU" rather than a "sleepable RCU". However it doesn't apply here since you want to avoid SRCU's 2 memory barriers per lock/unlock.

Paolo

Christian Borntraeger

author

2015-09-16 08:57 UTC #7 lore raw

Am 16.09.2015 um 10:32 schrieb Paolo Bonzini:

Quoted 7 lines · Paul E. McKenney

Quoted 4 lines
Excellent points!
Other options in such situations include the following:

Quoted 8 lines

Quoted 6 lines
o Rework so that the code uses call_rcu*() instead of *_expedited().
o Maintain a per-task or per-CPU counter so that every so many
*_expedited() invocations instead uses the non-expedited counterpart. (For example, synchronize_rcu instead of synchronize_rcu_expedited().)
Or just use ratelimit (untested):

One of my tests was to always replace synchronize_sched_expedited with synchronize_sched and things turned out to be even worse. Not sure if it makes sense to test yopur in-the-middle approach?

Christian

Paolo Bonzini

maintainer

2015-09-16 09:12 UTC #8 lore raw

Quoted 15 lines · Christian Borntraeger

Am 16.09.2015 um 10:32 schrieb Paolo Bonzini:
Quoted 14 lines · Paul E. McKenney
Quoted 10 lines
Excellent points!
Other options in such situations include the following:
o Rework so that the code uses call_rcu*() instead of *_expedited().
o Maintain a per-task or per-CPU counter so that every so many
*_expedited() invocations instead uses the non-expedited counterpart. (For example, synchronize_rcu instead of synchronize_rcu_expedited().)

Quoted 5 lines

Quoted 1 line
Or just use ratelimit (untested):
One of my tests was to always replace synchronize_sched_expedited with synchronize_sched and things turned out to be even worse. Not sure if it makes sense to test yopur in-the-middle approach?

I don't think it applies here, since down_write/up_write is a synchronous API.

If the revert isn't easy, I think backporting rcu_sync is the best bet.
The issue is that rcu_sync doesn't eliminate synchronize_sched, it only
makes it more rare. So it's possible that it isn't eliminating the root cause of the problem.

Paolo

Oleg Nesterov

2015-09-16 12:22 UTC #9 lore raw

Quoted 20 lines · Paolo Bonzini

Quoted 17 lines · Christian Borntraeger
Am 16.09.2015 um 10:32 schrieb Paolo Bonzini:
Quoted 15 lines · Paul E. McKenney
Quoted 10 lines
Excellent points!
Other options in such situations include the following:
o Rework so that the code uses call_rcu*() instead of *_expedited().
o Maintain a per-task or per-CPU counter so that every so many
*_expedited() invocations instead uses the non-expedited counterpart. (For example, synchronize_rcu instead of synchronize_rcu_expedited().)
Or just use ratelimit (untested):

Quoted 8 lines

Quoted 3 lines
One of my tests was to always replace synchronize_sched_expedited with synchronize_sched and things turned out to be even worse. Not sure if it makes sense to test yopur in-the-middle approach?
I don't think it applies here, since down_write/up_write is a synchronous API.
If the revert isn't easy, I think backporting rcu_sync is the best bet.

I leave this to Paul and Tejun... at least I think this is not v4.2 material.

Quoted 1 line

The issue is that rcu_sync doesn't eliminate synchronize_sched,

Yes, but it eliminates _expedited(). This is good, but otoh this means that (say) individual __cgroup_procs_write() can take much more time. However, it won't block the readers and/or disturb the whole system. And percpu_up_write() doesn't do synchronize_sched() at all.

Quoted 2 lines

it only
makes it more rare.

Yes, so we can hope that multiple __cgroup_procs_write()'s can "share" a single synchronize_sched().

Quoted 2 lines

So it's possible that it isn't eliminating the root cause of the problem.

We will see... Just in case, currently the usage of percpu_down_write() is suboptimal. We do not need to do ->sync() under cgroup_mutex. But this needs some WIP changes in rcu_sync. Plus we can do more improvements, but this is off-topic right now.

Oleg.

Paolo Bonzini

maintainer

2015-09-16 12:35 UTC #10 lore raw

Quoted 5 lines · Oleg Nesterov

Quoted 1 line
The issue is that rcu_sync doesn't eliminate synchronize_sched,
Yes, but it eliminates _expedited(). This is good, but otoh this means that (say) individual __cgroup_procs_write() can take much more time. However, it won't block the readers and/or disturb the whole system.

According to Christian, removing the _expedited() "makes things worse" in that the system takes ages to boot up and systemd timeouts. So I'm still a bit wary about anything that uses RCU for the cgroups write side.

However, rcu_sync is okay with him, so perhaps it is really really effective. Christian, can you instrument how many synchronize_sched (out of the 6479 cgroup_procs_write calls) are actually executed at boot with the rcu rework?

Paolo

Quoted 1 line

And percpu_up_write() doesn't do synchronize_sched() at all.

Oleg Nesterov

2015-09-16 12:43 UTC #11 lore raw

Quoted 10 lines · Paolo Bonzini

Quoted 5 lines · Oleg Nesterov
Quoted 1 line
The issue is that rcu_sync doesn't eliminate synchronize_sched,
Yes, but it eliminates _expedited(). This is good, but otoh this means that (say) individual __cgroup_procs_write() can take much more time. However, it won't block the readers and/or disturb the whole system.
According to Christian, removing the _expedited() "makes things worse"

Yes sure, we can not just remove _expedited() from down/up_read().

Quoted 1 line

in that the system takes ages to boot up and systemd timeouts.

Yes, this is clear

Quoted 7 lines

So I'm
still a bit wary about anything that uses RCU for the cgroups write side.
However, rcu_sync is okay with him, so perhaps it is really really effective. Christian, can you instrument how many synchronize_sched (out of the 6479 cgroup_procs_write calls) are actually executed at boot with the rcu rework?

Heh, another change I have in mind. It would be nice to add some trace points. But firstly we should merge the current code.

Oleg.

Christian Borntraeger

author

2015-09-16 12:56 UTC #12 lore raw

In reply to Oleg Nesterov

Am 16.09.2015 um 14:22 schrieb Oleg Nesterov:

Quoted 7 lines

Quoted 1 line
The issue is that rcu_sync doesn't eliminate synchronize_sched,
Yes, but it eliminates _expedited(). This is good, but otoh this means that (say) individual __cgroup_procs_write() can take much more time. However, it won't block the readers and/or disturb the whole system. And percpu_up_write() doesn't do synchronize_sched() at all.

Quoted 5 lines

Quoted 2 lines
it only
makes it more rare.
Yes, so we can hope that multiple __cgroup_procs_write()'s can "share" a single synchronize_sched().

And in fact it does. Paolo suggested to trace how often we call synchronize_sched so I applied some advanced printk debugging technology ;-) Until login I have 41 and after starting the 70 guests this went up to 48. Nice work.

Quoted 11 lines

Quoted 3 lines
So it's possible that it isn't eliminating the root cause of the problem.
We will see... Just in case, currently the usage of percpu_down_write() is suboptimal. We do not need to do ->sync() under cgroup_mutex. But this needs some WIP changes in rcu_sync. Plus we can do more improvements, but this is off-topic right now.
Oleg.

Tejun Heo

maintainer

2015-09-16 14:16 UTC #13 lore raw

In reply to Oleg Nesterov

Hello,

Quoted 3 lines · Oleg Nesterov

Quoted 1 line
If the revert isn't easy, I think backporting rcu_sync is the best bet.
I leave this to Paul and Tejun... at least I think this is not v4.2 material.

Will route reverts through cgroup branch. Should be pretty painless. Nice job on percpu_rwsem. It's so much better than having to come up with a separate middleground solution.

Thanks.

Signature · 1 line

tejun

Paolo Bonzini

maintainer

2015-09-16 14:19 UTC #14 lore raw

Quoted 7 lines · Tejun Heo

Quoted 3 lines · Oleg Nesterov
Quoted 3 lines
Quoted 1 line
Quoted 1 line
If the revert isn't easy, I think backporting rcu_sync is the best bet.
I leave this to Paul and Tejun... at least I think this is not v4.2 material.
Will route reverts through cgroup branch. Should be pretty painless. Nice job on percpu_rwsem. It's so much better than having to come up with a separate middleground solution.

Thanks all for the quick resolution!

Paolo

Christian Borntraeger

author

2015-09-15 21:11 UTC #15 lore raw

In reply to Paolo Bonzini

Am 15.09.2015 um 18:42 schrieb Paolo Bonzini:

Quoted 17 lines · Christian Borntraeger

Quoted 6 lines
I am wondering why the old code behaved in such fatal ways. Is there some interaction between waiting for a reschedule in the synchronize_sched writer and some fork code actually waiting for the read side to get the lock together with some rescheduling going on waiting for a lock that fork holds? lockdep does not give me an hints so I have no clue :-(
It may just be consuming too much CPU usage. kernel/rcu/tree.c warns about it:
* if you are using synchronize_sched_expedited() in a loop, please
* restructure your code to batch your updates, and then use a single
* synchronize_sched() instead.

Quoted 6 lines

and you may remember that in KVM we switched from RCU to SRCU exactly to avoid userspace-controlled synchronize_rcu_expedited().
In fact, I would say that any userspace-controlled call to *_expedited() is a bug waiting to happen and a bad idea---because userspace can, with little effort, end up calling it in a loop.

Right. This also implies that we should fix this for 4.2 - I guess.

Christian

Tejun Heo

maintainer

2015-09-15 21:26 UTC #16 lore raw

Hello,

Quoted 5 lines · Christian Borntraeger

Quoted 3 lines
In fact, I would say that any userspace-controlled call to *_expedited() is a bug waiting to happen and a bad idea---because userspace can, with little effort, end up calling it in a loop.
Right. This also implies that we should fix this for 4.2 - I guess.

Are the percpu_rwsem changes enough? If so, we can try to backport those. If those are too risky, we can revert the patches which switched threadgroup lock to percpu_rwsem.

Thanks.

Signature · 1 line

tejun

Paul E. McKenney

2015-09-15 21:38 UTC #17 lore raw

Quoted 7 lines · Tejun Heo

Hello,
Quoted 4 lines · Christian Borntraeger
Quoted 3 lines
In fact, I would say that any userspace-controlled call to *_expedited() is a bug waiting to happen and a bad idea---because userspace can, with little effort, end up calling it in a loop.

Quoted 5 lines

Quoted 1 line
Right. This also implies that we should fix this for 4.2 - I guess.
Are the percpu_rwsem changes enough? If so, we can try to backport those. If those are too risky, we can revert the patches which switched threadgroup lock to percpu_rwsem.

I did take a shot at adding the rcu_sync stuff during this past merge window, but it did not converge quickly enough to make it. It looks quite good for the next merge window. There have been changes in most of the relevant areas, so probably best to just try them and see which works best.

Thanx, Paul

Tejun Heo

maintainer

2015-09-15 22:28 UTC #18 lore raw

Hello,

Quoted 5 lines · Paul E. McKenney

I did take a shot at adding the rcu_sync stuff during this past merge window, but it did not converge quickly enough to make it. It looks quite good for the next merge window. There have been changes in most of the relevant areas, so probably best to just try them and see which works best.

Heh, I'm having a bit of trouble following. Are you saying that the changes would be too big for -stable? If so, I'll send out reverts of the culprit patches and then reapply them for this cycle so that it can land together with the rcu changes in the next merge window, but it'd be great to find out whether the rcu changes are enough for the issue that Christian is seeing to go away. If not, I'll switch to a different locking scheme and mark those patches w/ stable tag.

Thanks!

Signature · 1 line

tejun

Paul E. McKenney

2015-09-15 23:38 UTC #19 lore raw

Quoted 9 lines · Tejun Heo

Hello,
Quoted 5 lines · Paul E. McKenney
I did take a shot at adding the rcu_sync stuff during this past merge window, but it did not converge quickly enough to make it. It looks quite good for the next merge window. There have been changes in most of the relevant areas, so probably best to just try them and see which works best.

Quoted 7 lines

Heh, I'm having a bit of trouble following. Are you saying that the changes would be too big for -stable? If so, I'll send out reverts of the culprit patches and then reapply them for this cycle so that it can land together with the rcu changes in the next merge window, but it'd be great to find out whether the rcu changes are enough for the issue that Christian is seeing to go away. If not, I'll switch to a different locking scheme and mark those patches w/ stable tag.

Well, the decision as to what is too big for -stable is owned by the -stable maintainers, not by me.

I am suggesting trying the options and seeing what works best, then working to convince people as needed.

Thanx, Paul

Tejun Heo

maintainer

2015-09-16 01:24 UTC #20 lore raw

Hello, Paul.

Quoted 2 lines · Paul E. McKenney

Well, the decision as to what is too big for -stable is owned by the -stable maintainers, not by me.

Is it tho? Usually the subsystem maintainer knows the best and has most say in it. I was mostly curious whether you'd think that the changes would be too risky. If not, great.

Quoted 2 lines

I am suggesting trying the options and seeing what works best, then working to convince people as needed.

Yeah, sure thing. Let's wait for Christian.

Thanks.

Signature · 1 line

tejun

Paul E. McKenney

2015-09-16 04:35 UTC #21 lore raw

Quoted 9 lines · Tejun Heo

Hello, Paul.
Quoted 2 lines · Paul E. McKenney
Well, the decision as to what is too big for -stable is owned by the -stable maintainers, not by me.
Is it tho? Usually the subsystem maintainer knows the best and has most say in it. I was mostly curious whether you'd think that the changes would be too risky. If not, great.

I do hope that they would listen to what I thought about it, but at the end of the day, it is the -stable maintainers who pull a given patch, or don't.

Quoted 4 lines

Quoted 2 lines
I am suggesting trying the options and seeing what works best, then working to convince people as needed.
Yeah, sure thing. Let's wait for Christian.

Indeed. Is there enough benefit to risk jamming this thing into 4.3? I believe that 4.4 should be a no-brainer.

Thanx, Paul

Tejun Heo

maintainer

2015-09-16 11:06 UTC #22 lore raw

Hello,

Quoted 7 lines · Paul E. McKenney

Quoted 4 lines
Quoted 2 lines
I am suggesting trying the options and seeing what works best, then working to convince people as needed.
Yeah, sure thing. Let's wait for Christian.
Indeed. Is there enough benefit to risk jamming this thing into 4.3? I believe that 4.4 should be a no-brainer.

In that case, I'm gonna revert the threadcgroup percpu_rwsem conversion patches through -stable and reapply them for 4.4 merge window.

Thanks!

Signature · 1 line

tejun

Christian Borntraeger

author

2015-09-16 07:44 UTC #23 lore raw

In reply to Tejun Heo

Am 16.09.2015 um 03:24 schrieb Tejun Heo:

Quoted 6 lines

Hello, Paul.
Quoted 2 lines · Paul E. McKenney
Well, the decision as to what is too big for -stable is owned by the -stable maintainers, not by me.

Quoted 8 lines

Is it tho? Usually the subsystem maintainer knows the best and has most say in it. I was mostly curious whether you'd think that the changes would be too risky. If not, great.
Quoted 2 lines
I am suggesting trying the options and seeing what works best, then working to convince people as needed.
Yeah, sure thing. Let's wait for Christian.

Well, I have optimized my testcase now that is puts enough pressure to the system to confuses system (the older 209 version, which still has some event loop issues) that systemd restarts the journal deamon and does several other recoveries.
To avoid regressions - even for somewhat shaky userspaces - we should consider a revert for 4.2 stable.
There are several followup patches, which makes the revert non-trivial, though.

The rework of the percpu rwsem seems to work fine, but we are beyond the merge window so 4.4 seems better to me. (and consider a revert for 4.3)

Christian

Christian Borntraeger

author

2015-09-16 10:58 UTC #24 lore raw

Am 16.09.2015 um 09:44 schrieb Christian Borntraeger:

Quoted 25 lines

Am 16.09.2015 um 03:24 schrieb Tejun Heo:
Quoted 6 lines
Hello, Paul.
Quoted 2 lines · Paul E. McKenney
Well, the decision as to what is too big for -stable is owned by the -stable maintainers, not by me.
Quoted 8 lines
Is it tho? Usually the subsystem maintainer knows the best and has most say in it. I was mostly curious whether you'd think that the changes would be too risky. If not, great.
Quoted 2 lines
I am suggesting trying the options and seeing what works best, then working to convince people as needed.
Yeah, sure thing. Let's wait for Christian.
Well, I have optimized my testcase now that is puts enough pressure to the system to confuses system (the older 209 version, which still has some event loop issues) that systemd restarts the journal deamon and does several other recoveries.
To avoid regressions - even for somewhat shaky userspaces - we should consider a revert for 4.2 stable.
There are several followup patches, which makes the revert non-trivial, though.

Quoted 2 lines

The rework of the percpu rwsem seems to work fine, but we are beyond the merge window so 4.4 seems better to me. (and consider a revert for 4.3)

FWIW, I added a printk to percpu_down_write. With KVM and uprobes disabled, just booting up a fedora20 gives me __6749__ percpu_down_write calls on 4.2. systemd seems to do that for the processes.

So a revert is really the right thing to do. In fact, I dont know if the rcu_sync_enter rework is enough. With systemd setting the cgroup seem to be NOT a cold/seldom case.

Christian

Tejun Heo

maintainer

2015-09-16 11:03 UTC #25 lore raw

Hello,

Quoted 7 lines · Christian Borntraeger

FWIW, I added a printk to percpu_down_write. With KVM and uprobes disabled, just booting up a fedora20 gives me __6749__ percpu_down_write calls on 4.2. systemd seems to do that for the processes.
So a revert is really the right thing to do. In fact, I dont know if the rcu_sync_enter rework is enough. With systemd setting the cgroup seem to be NOT a cold/seldom case.

Booting would usually be the hottest operation for that and it's still *relatively* cold path compared to the reader side which is task fork/exit paths. The whole point is shift overhead from hotter reader side. Can you see problems with percpu_rwsem rework?

Thanks.

Signature · 1 line

tejun

Christian Borntraeger

author

2015-09-16 11:50 UTC #26 lore raw

Am 16.09.2015 um 13:03 schrieb Tejun Heo:

Quoted 7 lines

Hello,
Quoted 4 lines · Christian Borntraeger
FWIW, I added a printk to percpu_down_write. With KVM and uprobes disabled, just booting up a fedora20 gives me __6749__ percpu_down_write calls on 4.2. systemd seems to do that for the processes.

Quoted 8 lines

Quoted 3 lines
So a revert is really the right thing to do. In fact, I dont know if the rcu_sync_enter rework is enough. With systemd setting the cgroup seem to be NOT a cold/seldom case.
Booting would usually be the hottest operation for that and it's still *relatively* cold path compared to the reader side which is task fork/exit paths. The whole point is shift overhead from hotter reader side. Can you see problems with percpu_rwsem rework?

As I said, it seems the rcu tree with that change seems to work fine on my system. This needs a more testing on other machines, though. I guess a revert plus a re-add in the 4.4 merge window should give us enough test coverage.

Christian

Tejun Heo

maintainer

2015-09-16 15:55 UTC #27 lore raw

PATCH 1/2 Revert "cgroup: simplify threadgroup locking"

In reply to Christian Borntraeger

Quoted 1 line

From f9f9e7b776142fb1c0782cade004cc8e0147a199 Mon Sep 17 00:00:00 2001

From: Tejun Heo <[email protected]>
Date: Wed, 16 Sep 2015 11:51:12 -0400

This reverts commit b5ba75b5fc0e8404e2c50cb68f39bb6a53fc916f.

d59cfc09c32a ("sched, cgroup: replace signal_struct->group_rwsem with a global percpu_rwsem") and b5ba75b5fc0e ("cgroup: simplify threadgroup locking") changed how cgroup synchronizes against task fork and exits so that it uses global percpu_rwsem instead of per-process rwsem; unfortunately, the write [un]lock paths of percpu_rwsem always involve synchronize_rcu_expedited() which turned out to be too expensive.

Improvements for percpu_rwsem are scheduled to be merged in the coming v4.4-rc1 merge window which alleviates this issue. For now, revert the two commits to restore per-process rwsem. They will be re-applied for the v4.4-rc1 merge window.

Signed-off-by Tejun Heo <[email protected]>

Link http://lkml.kernel.org/g/[email protected] open here

Reported-by Christian Borntraeger <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc [email protected] # v4.2+

---
Hello,

These are the two reverts that I'm pushing through cgroup/for-4.3-fixes. I'll re-apply the reverted patches on the for-4.4 branch so that they can land together with percpu_rwsem updates during the next merge window.

Thanks.

 kernel/cgroup.c | 45 +++++++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 12 deletions(-)

kernel/cgroup.c +33 ~~-12~~

index 2cf0f79..115091e 100644
@@ -2460,13 +2460,14 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
 	if (!cgrp)
 		return -ENODEV;
 
-	percpu_down_write(&cgroup_threadgroup_rwsem);
+retry_find_task:
 	rcu_read_lock();
 	if (pid) {
 		tsk = find_task_by_vpid(pid);
 		if (!tsk) {
+			rcu_read_unlock();
 			ret = -ESRCH;
-			goto out_unlock_rcu;
+			goto out_unlock_cgroup;
 		}
 	} else {
 		tsk = current;

@@ -2482,23 +2483,37 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
 	 */
 	if (tsk == kthreadd_task || (tsk->flags & PF_NO_SETAFFINITY)) {
 		ret = -EINVAL;
-		goto out_unlock_rcu;
+		rcu_read_unlock();
+		goto out_unlock_cgroup;
 	}
 
 	get_task_struct(tsk);
 	rcu_read_unlock();
 
+	percpu_down_write(&cgroup_threadgroup_rwsem);
+	if (threadgroup) {
+		if (!thread_group_leader(tsk)) {
+			/*
+			 * a race with de_thread from another thread's exec()
+			 * may strip us of our leadership, if this happens,
+			 * there is no choice but to throw this task away and
+			 * try again; this is
+			 * "double-double-toil-and-trouble-check locking".
+			 */
+			percpu_up_write(&cgroup_threadgroup_rwsem);
+			put_task_struct(tsk);
+			goto retry_find_task;
+		}
+	}
+
 	ret = cgroup_procs_write_permission(tsk, cgrp, of);
 	if (!ret)
 		ret = cgroup_attach_task(cgrp, tsk, threadgroup);
 
-	put_task_struct(tsk);
-	goto out_unlock_threadgroup;
-
-out_unlock_rcu:
-	rcu_read_unlock();
-out_unlock_threadgroup:
 	percpu_up_write(&cgroup_threadgroup_rwsem);
+
+	put_task_struct(tsk);
+out_unlock_cgroup:
 	cgroup_kn_unlock(of->kn);
 	return ret ?: nbytes;
 }

@@ -2643,8 +2658,6 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
 
 	lockdep_assert_held(&cgroup_mutex);
 
-	percpu_down_write(&cgroup_threadgroup_rwsem);
-
 	/* look up all csses currently attached to @cgrp's subtree */
 	down_read(&css_set_rwsem);
 	css_for_each_descendant_pre(css, cgroup_css(cgrp, NULL)) {

@@ -2700,8 +2713,17 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
 				goto out_finish;
 			last_task = task;
 
+			percpu_down_write(&cgroup_threadgroup_rwsem);
+			/* raced against de_thread() from another thread? */
+			if (!thread_group_leader(task)) {
+				percpu_up_write(&cgroup_threadgroup_rwsem);
+				put_task_struct(task);
+				continue;
+			}
+
 			ret = cgroup_migrate(src_cset->dfl_cgrp, task, true);
 
+			percpu_up_write(&cgroup_threadgroup_rwsem);
 			put_task_struct(task);
 
 			if (WARN(ret, "cgroup: failed to update controllers for the default hierarchy (%d), further operations may crash or hang\n", ret))

@@ -2711,7 +2733,6 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
 
 out_finish:
 	cgroup_migrate_finish(&preloaded_csets);
-	percpu_up_write(&cgroup_threadgroup_rwsem);
 	return ret;
 }

Signature · 1 line

2.4.3

Tejun Heo

maintainer

2015-09-16 15:56 UTC #28 lore raw

PATCH 2/2 Revert "sched, cgroup: replace signal_struct->group_rwsem with a global percpu_rwsem"

Quoted 1 line

From 0c986253b939cc14c69d4adbe2b4121bdf4aa220 Mon Sep 17 00:00:00 2001

From: Tejun Heo <[email protected]>
Date: Wed, 16 Sep 2015 11:51:12 -0400

This reverts commit d59cfc09c32a2ae31f1c3bc2983a0cd79afb3f14.

Signed-off-by Tejun Heo <[email protected]>

Link http://lkml.kernel.org/g/[email protected] open here

Reported-by Christian Borntraeger <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc [email protected] # v4.2+

---
 include/linux/cgroup-defs.h | 27 ++--------------
 include/linux/init_task.h   |  8 +++++
 include/linux/sched.h       | 12 +++++++
 kernel/cgroup.c             | 77 +++++++++++++++++++++++++++++++++------------
 kernel/fork.c               |  4 +++
 5 files changed, 83 insertions(+), 45 deletions(-)

include/linux/cgroup-defs.h +2 ~~-25~~

index 4d8fcf2..8492721 100644
@@ -473,31 +473,8 @@ struct cgroup_subsys {
 	unsigned int depends_on;
 };
 
-extern struct percpu_rw_semaphore cgroup_threadgroup_rwsem;
-
-/**
- * cgroup_threadgroup_change_begin - threadgroup exclusion for cgroups
- * @tsk: target task
- *
- * Called from threadgroup_change_begin() and allows cgroup operations to
- * synchronize against threadgroup changes using a percpu_rw_semaphore.
- */
-static inline void cgroup_threadgroup_change_begin(struct task_struct *tsk)
-{
-	percpu_down_read(&cgroup_threadgroup_rwsem);
-}
-
-/**
- * cgroup_threadgroup_change_end - threadgroup exclusion for cgroups
- * @tsk: target task
- *
- * Called from threadgroup_change_end().  Counterpart of
- * cgroup_threadcgroup_change_begin().
- */
-static inline void cgroup_threadgroup_change_end(struct task_struct *tsk)
-{
-	percpu_up_read(&cgroup_threadgroup_rwsem);
-}
+void cgroup_threadgroup_change_begin(struct task_struct *tsk);
+void cgroup_threadgroup_change_end(struct task_struct *tsk);
 
 #else	/* CONFIG_CGROUPS */

include/linux/init_task.h +8 -0

index d0b380e..e38681f 100644
@@ -25,6 +25,13 @@
 extern struct files_struct init_files;
 extern struct fs_struct init_fs;
 
+#ifdef CONFIG_CGROUPS
+#define INIT_GROUP_RWSEM(sig)						\
+	.group_rwsem = __RWSEM_INITIALIZER(sig.group_rwsem),
+#else
+#define INIT_GROUP_RWSEM(sig)
+#endif
+
 #ifdef CONFIG_CPUSETS
 #define INIT_CPUSET_SEQ(tsk)							\
 	.mems_allowed_seq = SEQCNT_ZERO(tsk.mems_allowed_seq),

@@ -57,6 +64,7 @@ extern struct fs_struct init_fs;
 	INIT_PREV_CPUTIME(sig)						\
 	.cred_guard_mutex =						\
 		 __MUTEX_INITIALIZER(sig.cred_guard_mutex),		\
+	INIT_GROUP_RWSEM(sig)						\
 }
 
 extern struct nsproxy init_nsproxy;

include/linux/sched.h +12 -0

index a4ab9da..b7b9501 100644
@@ -762,6 +762,18 @@ struct signal_struct {
 	unsigned audit_tty_log_passwd;
 	struct tty_audit_buf *tty_audit_buf;
 #endif
+#ifdef CONFIG_CGROUPS
+	/*
+	 * group_rwsem prevents new tasks from entering the threadgroup and
+	 * member tasks from exiting,a more specifically, setting of
+	 * PF_EXITING.  fork and exit paths are protected with this rwsem
+	 * using threadgroup_change_begin/end().  Users which require
+	 * threadgroup to remain stable should use threadgroup_[un]lock()
+	 * which also takes care of exec path.  Currently, cgroup is the
+	 * only user.
+	 */
+	struct rw_semaphore group_rwsem;
+#endif
 
 	oom_flags_t oom_flags;
 	short oom_score_adj;		/* OOM kill score adjustment */

kernel/cgroup.c +57 ~~-20~~

157 lines not on this page · raw message

kernel/fork.c +4 -0

index 7d5f0f1..2845623 100644
@@ -1149,6 +1149,10 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	tty_audit_fork(sig);
 	sched_autogroup_fork(sig);
 
+#ifdef CONFIG_CGROUPS
+	init_rwsem(&sig->group_rwsem);
+#endif
+
 	sig->oom_score_adj = current->signal->oom_score_adj;
 	sig->oom_score_adj_min = current->signal->oom_score_adj_min;

Signature · 1 line

2.4.3

Oleg Nesterov

2015-09-16 17:00 UTC #29 lore raw

Re: [PATCH cgroup/for-4.3-fixes 1/2] Revert "cgroup: simplify threadgroup locking"

In reply to Tejun Heo

Quoted 20 lines · Tejun Heo

From f9f9e7b776142fb1c0782cade004cc8e0147a199 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Wed, 16 Sep 2015 11:51:12 -0400
This reverts commit b5ba75b5fc0e8404e2c50cb68f39bb6a53fc916f.
d59cfc09c32a ("sched, cgroup: replace signal_struct->group_rwsem with a global percpu_rwsem") and b5ba75b5fc0e ("cgroup: simplify threadgroup locking") changed how cgroup synchronizes against task fork and exits so that it uses global percpu_rwsem instead of per-process rwsem; unfortunately, the write [un]lock paths of percpu_rwsem always involve synchronize_rcu_expedited() which turned out to be too expensive.
Improvements for percpu_rwsem are scheduled to be merged in the coming v4.4-rc1 merge window which alleviates this issue. For now, revert the two commits to restore per-process rwsem. They will be re-applied for the v4.4-rc1 merge window.

Quoted 8 lines

Signed-off-by Tejun Heo <[email protected]>
Link http://lkml.kernel.org/g/[email protected] open here
Reported-by Christian Borntraeger <[email protected]>
Cc Oleg Nesterov <[email protected]>
Cc "Paul E. McKenney" <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Paolo Bonzini <[email protected]>
Cc [email protected] # v4.2+

So just in case, I agree. Perhaps we could merge the percpu_rwsem changes in v4.3, but these patches look much safer for -stable.

Oleg.

Christian Borntraeger

author Tested-by on v1 1/2

2015-09-16 18:45 UTC #30 lore raw

Re: [PATCH cgroup/for-4.3-fixes 1/2] Revert "cgroup: simplify threadgroup locking"

In reply to Tejun Heo

Both patches in combination

Tested-by Christian Borntraeger <[email protected]> # on top of 4.3-rc1

As a side note, patch 2 does not apply cleanly on 4.2, so we probably need to provide a separate backport.

Christian