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

Groups > linux.kernel > #1225029 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Christian Borntraeger <borntraeger@de.ibm.com> - 2015-09-15 14:10 +0200
    Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Peter Zijlstra <peterz@infradead.org> - 2015-09-15 15:10 +0200
      Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Christian Borntraeger <borntraeger@de.ibm.com> - 2015-09-15 15:40 +0200
        Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Tejun Heo <tj@kernel.org> - 2015-09-15 16:00 +0200
        Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Paolo Bonzini <pbonzini@redhat.com> - 2015-09-15 18:50 +0200
          Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-15 19:40 +0200
            Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Paolo Bonzini <pbonzini@redhat.com> - 2015-09-16 10:40 +0200
              Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Christian Borntraeger <borntraeger@de.ibm.com> - 2015-09-16 11:00 +0200
                Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Paolo Bonzini <pbonzini@redhat.com> - 2015-09-16 11:20 +0200
                  Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes  regression for libvirt/kvm Oleg Nesterov <oleg@redhat.com> - 2015-09-16 14:30 +0200
                    Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Paolo Bonzini <pbonzini@redhat.com> - 2015-09-16 14:40 +0200
                      Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes  regression for libvirt/kvm Oleg Nesterov <oleg@redhat.com> - 2015-09-16 14:50 +0200
                    Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Christian Borntraeger <borntraeger@de.ibm.com> - 2015-09-16 15:00 +0200
                    Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Tejun Heo <tj@kernel.org> - 2015-09-16 16:20 +0200
                      Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Paolo Bonzini <pbonzini@redhat.com> - 2015-09-16 16:20 +0200
          Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Christian Borntraeger <borntraeger@de.ibm.com> - 2015-09-15 23:20 +0200
            Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Tejun Heo <tj@kernel.org> - 2015-09-15 23:30 +0200
              Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-15 23:40 +0200
                Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Tejun Heo <tj@kernel.org> - 2015-09-16 00:30 +0200
                  Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-16 01:40 +0200
                    Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Tejun Heo <tj@kernel.org> - 2015-09-16 03:30 +0200
                      Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-16 06:40 +0200
                        Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Tejun Heo <tj@kernel.org> - 2015-09-16 13:10 +0200
                      Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Christian Borntraeger <borntraeger@de.ibm.com> - 2015-09-16 09:50 +0200
                        Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Christian Borntraeger <borntraeger@de.ibm.com> - 2015-09-16 13:00 +0200
                          Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Tejun Heo <tj@kernel.org> - 2015-09-16 13:10 +0200
                            Re: [4.2] commit d59cfc09c32 (sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem) causes regression for  libvirt/kvm Christian Borntraeger <borntraeger@de.ibm.com> - 2015-09-16 14:00 +0200
    [PATCH cgroup/for-4.3-fixes 1/2] Revert "cgroup: simplify  threadgroup locking" Tejun Heo <tj@kernel.org> - 2015-09-16 18:00 +0200
      [PATCH cgroup/for-4.3-fixes 2/2] Revert "sched, cgroup: replace  signal_struct->group_rwsem with a global percpu_rwsem" Tejun Heo <tj@kernel.org> - 2015-09-16 18:00 +0200
      Re: [PATCH cgroup/for-4.3-fixes 1/2] Revert "cgroup: simplify  threadgroup locking" Oleg Nesterov <oleg@redhat.com> - 2015-09-16 19:10 +0200
      Re: [PATCH cgroup/for-4.3-fixes 1/2] Revert "cgroup: simplify  threadgroup locking" Christian Borntraeger <borntraeger@de.ibm.com> - 2015-09-16 20:50 +0200

Page 1 of 2  [1] 2  Next page →

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1225085

On Tue, Sep 15, 2015 at 02:05:14PM +0200, Christian Borntraeger wrote:
> 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.

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225113

Am 15.09.2015 um 15:05 schrieb Peter Zijlstra:
> On Tue, Sep 15, 2015 at 02:05:14PM +0200, Christian Borntraeger wrote:
>> 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.
> 
> 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.

> 
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225131

Hello,

On Tue, Sep 15, 2015 at 03:36:34PM +0200, Christian Borntraeger wrote:
> >> 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.

> > 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.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225419

On 15/09/2015 15:36, Christian Borntraeger wrote:
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225465

On Tue, Sep 15, 2015 at 06:42:19PM +0200, Paolo Bonzini wrote:
> 
> 
> On 15/09/2015 15:36, Christian Borntraeger wrote:
> > 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.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225831

On 15/09/2015 19:38, Paul E. McKenney wrote:
> 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):

diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
index 834c4e52cb2d..8fb66b2aeed9 100644
--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -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 *);
diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index f32567254867..c33f8bc89384 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -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);
 }


> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225843

Am 16.09.2015 um 10:32 schrieb Paolo Bonzini:
> 
> 
> On 15/09/2015 19:38, Paul E. McKenney wrote:
>> 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):

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225867

On 16/09/2015 10:57, Christian Borntraeger wrote:
> Am 16.09.2015 um 10:32 schrieb Paolo Bonzini:
>>
>>
>> On 15/09/2015 19:38, Paul E. McKenney wrote:
>>> 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):
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On 09/16, Paolo Bonzini wrote:
>
>
> On 16/09/2015 10:57, Christian Borntraeger wrote:
> > Am 16.09.2015 um 10:32 schrieb Paolo Bonzini:
> >>
> >>
> >> On 15/09/2015 19:38, Paul E. McKenney wrote:
> >>> 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):
> >
> > 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.

>  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.

> it only
> makes it more rare.

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

> 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1226050

On 16/09/2015 14:22, Oleg Nesterov wrote:
> >  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

> And percpu_up_write() doesn't do synchronize_sched() at all.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On 09/16, Paolo Bonzini wrote:
>
>
> On 16/09/2015 14:22, Oleg Nesterov wrote:
> > >  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().

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

Yes, this is clear

> 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1226061

Am 16.09.2015 um 14:22 schrieb Oleg Nesterov:
>>  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.
> 
>> 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.

> 
>> 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.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1226124

Hello,

On Wed, Sep 16, 2015 at 02:22:49PM +0200, Oleg Nesterov wrote:
> > 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.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1226125

On 16/09/2015 16:16, Tejun Heo wrote:
> On Wed, Sep 16, 2015 at 02:22:49PM +0200, Oleg Nesterov wrote:
>>> > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225549

Am 15.09.2015 um 18:42 schrieb Paolo Bonzini:
> 
> 
> On 15/09/2015 15:36, Christian Borntraeger wrote:
>> 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.

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

Christian

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225553

Hello,

On Tue, Sep 15, 2015 at 11:11:45PM +0200, Christian Borntraeger wrote:
> > 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.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225558

On Tue, Sep 15, 2015 at 05:26:22PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Tue, Sep 15, 2015 at 11:11:45PM +0200, Christian Borntraeger wrote:
> > > 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.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225567

Hello,

On Tue, Sep 15, 2015 at 02:38:30PM -0700, Paul E. McKenney wrote:
> 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!

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1225585

On Tue, Sep 15, 2015 at 06:28:11PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Tue, Sep 15, 2015 at 02:38:30PM -0700, Paul E. McKenney wrote:
> > 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.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web