Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1441923 > unrolled thread
| Started by | John Stultz <john.stultz@linaro.org> |
|---|---|
| First post | 2016-07-13 02:10 +0200 |
| Last post | 2016-07-13 23:00 +0200 |
| Articles | 20 on this page of 66 — 7 participants |
Back to article view | Back to linux.kernel
Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-13 02:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-13 10:30 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 16:50 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 20:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 20:30 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 20:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 22:20 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-13 22:30 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 22:50 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-13 23:00 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 23:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:20 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:50 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-13 23:50 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 00:20 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 00:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 01:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 01:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 13:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 14:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 14:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 14:30 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 17:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 17:30 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 18:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 19:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 19:00 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 00:30 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 00:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 00:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 09:00 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 13:30 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 14:20 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 17:20 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 23:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 15:20 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 16:20 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 17:00 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 18:20 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 18:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 19:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 18:30 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 18:50 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 19:20 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 18:50 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 18:50 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 19:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 19:20 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 19:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 19:50 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 20:00 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 20:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 20:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 21:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-13 23:00 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-13 23:00 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:00 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Dmitry Shmidt <dimitrysh@google.com> - 2016-07-13 23:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-13 23:10 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes John Stultz <john.stultz@linaro.org> - 2016-07-13 22:20 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Dmitry Shmidt <dimitrysh@google.com> - 2016-07-13 22:40 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Colin Cross <ccross@google.com> - 2016-07-13 22:50 +0200
Re: Severe performance regression w/ 4.4+ on Android due to cgroup locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 23:00 +0200
Page 1 of 4 [1] 2 3 4 Next page →
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2016-07-13 02:10 +0200 |
| Subject | Severe performance regression w/ 4.4+ on Android due to cgroup locking changes |
| Message-ID | <rUfUu-3Km-11@gated-at.bofh.it> |
Hey Tejun,
So Dmitry Shmidt recently noticed that with 4.4 based systems we're
seeing quite a bit of performance overhead from
__cgroup_procs_write().
With 4.4 tree as it stands, we're seeing __cgroup_procs_write() quite
often take 10s of miliseconds to execute (with max times up in the
80ms range).
While with 4.1 it was quite often in the single usec range, and max
time values still in in sub-milisecond range.
The majority of these performance regressions seem to come from the
locking changes in:
3014dde762f6 ("cgroup: simplify threadgroup locking")
and
1ed1328792ff ("sched, cgroup: replace signal_struct->group_rwsem with
a global percpu_rwsem")
Dmitry has found that by reverting these two changes (which don't
revert easiliy), we can get back down to tens 10-100 usec range for
most calls, with max values occasionally spiking to ~18ms.
Those two commits do talk about performance regressions, that were
supposedly alleviated by percpu_rwsem changes, but I'm not sure we are
seeing this.
In 1ed1328792ff, the commit talks about the write path being a fairly
cold path, but with Android I worry this may not actually be the case,
as Android uses cpuset cgroups to group tasks into foreground and
background tasks, but this means when switching applications, tasks
are migrated between cgroups. Putting an additional 80 milisecond
delay on this adds potentially visible latencies on task switching.
Reverting those two changes in the Android common.git tree doesn't
feel like a good long term solution here, so I was wondering if you
had any thoughts on how to further reduce the performance regression
here?
All the credit for finding this goes to Dmitry, I just was able to
reproduce his results and thoguht we should bring it up for discussion
here.
thanks
-john
[toc] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-07-13 10:30 +0200 |
| Message-ID | <rUnIl-rS-1@gated-at.bofh.it> |
| In reply to | #1441923 |
On Tue, Jul 12, 2016 at 05:00:04PM -0700, John Stultz wrote:
> Hey Tejun,
>
> So Dmitry Shmidt recently noticed that with 4.4 based systems we're
> seeing quite a bit of performance overhead from
> __cgroup_procs_write().
>
> With 4.4 tree as it stands, we're seeing __cgroup_procs_write() quite
> often take 10s of miliseconds to execute (with max times up in the
> 80ms range).
>
> While with 4.1 it was quite often in the single usec range, and max
> time values still in in sub-milisecond range.
>
> The majority of these performance regressions seem to come from the
> locking changes in:
>
> 3014dde762f6 ("cgroup: simplify threadgroup locking")
> and
> 1ed1328792ff ("sched, cgroup: replace signal_struct->group_rwsem with
> a global percpu_rwsem")
>
> Dmitry has found that by reverting these two changes (which don't
> revert easiliy), we can get back down to tens 10-100 usec range for
> most calls, with max values occasionally spiking to ~18ms.
>
> Those two commits do talk about performance regressions, that were
> supposedly alleviated by percpu_rwsem changes, but I'm not sure we are
> seeing this.
Do you have 'funny' RCU options that quickly force a grace period when
you go idle or something?
But yes, it does not surprise me to find this commit is causing
problems.
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-07-13 16:50 +0200 |
| Message-ID | <rUtE5-4my-11@gated-at.bofh.it> |
| In reply to | #1442161 |
On Wed, Jul 13, 2016 at 10:21:12AM +0200, Peter Zijlstra wrote:
> On Tue, Jul 12, 2016 at 05:00:04PM -0700, John Stultz wrote:
> > Hey Tejun,
> >
> > So Dmitry Shmidt recently noticed that with 4.4 based systems we're
> > seeing quite a bit of performance overhead from
> > __cgroup_procs_write().
> >
> > With 4.4 tree as it stands, we're seeing __cgroup_procs_write() quite
> > often take 10s of miliseconds to execute (with max times up in the
> > 80ms range).
> >
> > While with 4.1 it was quite often in the single usec range, and max
> > time values still in in sub-milisecond range.
> >
> > The majority of these performance regressions seem to come from the
> > locking changes in:
> >
> > 3014dde762f6 ("cgroup: simplify threadgroup locking")
> > and
> > 1ed1328792ff ("sched, cgroup: replace signal_struct->group_rwsem with
> > a global percpu_rwsem")
> >
> > Dmitry has found that by reverting these two changes (which don't
> > revert easiliy), we can get back down to tens 10-100 usec range for
> > most calls, with max values occasionally spiking to ~18ms.
> >
> > Those two commits do talk about performance regressions, that were
> > supposedly alleviated by percpu_rwsem changes, but I'm not sure we are
> > seeing this.
>
> Do you have 'funny' RCU options that quickly force a grace period when
> you go idle or something?
>
> But yes, it does not surprise me to find this commit is causing
> problems.
Hmmm... Looks like RCU is present both before and after. But please
do send along your .config.
Speaking of .config, is CONFIG_PREEMPT=y? If so, does the workload
feature preemption and migration? If that is the case, you might be
seeing contention on the per-CPU cgroup_threadgroup_rwsem, given that
the second patch seems to be adding acquisitions.
Thanx, Paul
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-07-13 20:40 +0200 |
| Message-ID | <rUxeG-6P0-31@gated-at.bofh.it> |
| In reply to | #1442508 |
On Wed, Jul 13, 2016 at 11:13:26AM -0700, Dmitry Shmidt wrote:
> On Wed, Jul 13, 2016 at 7:42 AM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Jul 13, 2016 at 10:21:12AM +0200, Peter Zijlstra wrote:
> >> On Tue, Jul 12, 2016 at 05:00:04PM -0700, John Stultz wrote:
> >> > Hey Tejun,
> >> >
> >> > So Dmitry Shmidt recently noticed that with 4.4 based systems we're
> >> > seeing quite a bit of performance overhead from
> >> > __cgroup_procs_write().
> >> >
> >> > With 4.4 tree as it stands, we're seeing __cgroup_procs_write() quite
> >> > often take 10s of miliseconds to execute (with max times up in the
> >> > 80ms range).
> >> >
> >> > While with 4.1 it was quite often in the single usec range, and max
> >> > time values still in in sub-milisecond range.
> >> >
> >> > The majority of these performance regressions seem to come from the
> >> > locking changes in:
> >> >
> >> > 3014dde762f6 ("cgroup: simplify threadgroup locking")
> >> > and
> >> > 1ed1328792ff ("sched, cgroup: replace signal_struct->group_rwsem with
> >> > a global percpu_rwsem")
> >> >
> >> > Dmitry has found that by reverting these two changes (which don't
> >> > revert easiliy), we can get back down to tens 10-100 usec range for
> >> > most calls, with max values occasionally spiking to ~18ms.
> >> >
> >> > Those two commits do talk about performance regressions, that were
> >> > supposedly alleviated by percpu_rwsem changes, but I'm not sure we are
> >> > seeing this.
> >>
> >> Do you have 'funny' RCU options that quickly force a grace period when
> >> you go idle or something?
> >>
> >> But yes, it does not surprise me to find this commit is causing
> >> problems.
> >
> > Hmmm... Looks like RCU is present both before and after. But please
> > do send along your .config.
>
> Attached
No funny RCU Kconfig options set -- vanilla preemptible RCU.
> > Speaking of .config, is CONFIG_PREEMPT=y? If so, does the workload
> > feature preemption and migration? If that is the case, you might be
> > seeing contention on the per-CPU cgroup_threadgroup_rwsem, given that
> > the second patch seems to be adding acquisitions.
>
> CONFIG_PREEMPT=y is set.
> We see this issue during the boot, so it supposes to be enough CPU load to
> cause preemption and migration.
How early during boot? Presumably after the scheduler has started...
Thanx, Paul
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2016-07-13 20:30 +0200 |
| Message-ID | <rUx50-6Li-21@gated-at.bofh.it> |
| In reply to | #1441923 |
(cc'ing Oleg)
Hello,
On Tue, Jul 12, 2016 at 05:00:04PM -0700, John Stultz wrote:
> So Dmitry Shmidt recently noticed that with 4.4 based systems we're
> seeing quite a bit of performance overhead from
> __cgroup_procs_write().
>
> With 4.4 tree as it stands, we're seeing __cgroup_procs_write() quite
> often take 10s of miliseconds to execute (with max times up in the
> 80ms range).
Yikes, that's pretty high. Does this happen only while the system is
generally busy or regardless of overall load?
> While with 4.1 it was quite often in the single usec range, and max
> time values still in in sub-milisecond range.
>
> The majority of these performance regressions seem to come from the
> locking changes in:
>
> 3014dde762f6 ("cgroup: simplify threadgroup locking")
> and
> 1ed1328792ff ("sched, cgroup: replace signal_struct->group_rwsem with
> a global percpu_rwsem")
>
> Dmitry has found that by reverting these two changes (which don't
> revert easiliy), we can get back down to tens 10-100 usec range for
> most calls, with max values occasionally spiking to ~18ms.
>
> Those two commits do talk about performance regressions, that were
> supposedly alleviated by percpu_rwsem changes, but I'm not sure we are
> seeing this.
>
> In 1ed1328792ff, the commit talks about the write path being a fairly
> cold path, but with Android I worry this may not actually be the case,
> as Android uses cpuset cgroups to group tasks into foreground and
> background tasks, but this means when switching applications, tasks
> are migrated between cgroups. Putting an additional 80 milisecond
> delay on this adds potentially visible latencies on task switching.
Switching between foreground and background isn't a hot path. It's
human initiated operations after all. It taking 80 msecs sure is
problematic but I'm skeptical that this is from actual contention
given that the only reader side holders are fork and exit paths.
> Reverting those two changes in the Android common.git tree doesn't
> feel like a good long term solution here, so I was wondering if you
> had any thoughts on how to further reduce the performance regression
> here?
One interesting thing to try would be replacing it with a regular
non-percpu rwsem and see how it behaves. That should easily tell us
whether this is from actual contention or artifacts from percpu_rwsem
implementation.
Thanks.
--
tejun
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2016-07-13 20:40 +0200 |
| Message-ID | <rUxeG-6P0-17@gated-at.bofh.it> |
| In reply to | #1442747 |
On Wed, Jul 13, 2016 at 02:21:02PM -0400, Tejun Heo wrote:
> One interesting thing to try would be replacing it with a regular
> non-percpu rwsem and see how it behaves. That should easily tell us
> whether this is from actual contention or artifacts from percpu_rwsem
> implementation.
So, something like the following. Can you please see whether this
makes any difference?
Thanks.
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 5b17de6..bc1e4d8 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -14,7 +14,7 @@
#include <linux/mutex.h>
#include <linux/rcupdate.h>
#include <linux/percpu-refcount.h>
-#include <linux/percpu-rwsem.h>
+#include <linux/rwsem.h>
#include <linux/workqueue.h>
#ifdef CONFIG_CGROUPS
@@ -518,7 +518,7 @@ struct cgroup_subsys {
unsigned int depends_on;
};
-extern struct percpu_rw_semaphore cgroup_threadgroup_rwsem;
+extern struct rw_semaphore cgroup_threadgroup_rwsem;
/**
* cgroup_threadgroup_change_begin - threadgroup exclusion for cgroups
@@ -529,7 +529,7 @@ extern struct percpu_rw_semaphore cgroup_threadgroup_rwsem;
*/
static inline void cgroup_threadgroup_change_begin(struct task_struct *tsk)
{
- percpu_down_read(&cgroup_threadgroup_rwsem);
+ down_read(&cgroup_threadgroup_rwsem);
}
/**
@@ -541,7 +541,7 @@ static inline void cgroup_threadgroup_change_begin(struct task_struct *tsk)
*/
static inline void cgroup_threadgroup_change_end(struct task_struct *tsk)
{
- percpu_up_read(&cgroup_threadgroup_rwsem);
+ up_read(&cgroup_threadgroup_rwsem);
}
#else /* CONFIG_CGROUPS */
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 86cb5c6..ed9c142 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -113,7 +113,7 @@ static DEFINE_SPINLOCK(cgroup_file_kn_lock);
*/
static DEFINE_SPINLOCK(release_agent_path_lock);
-struct percpu_rw_semaphore cgroup_threadgroup_rwsem;
+DECLARE_RWSEM(cgroup_threadgroup_rwsem);
#define cgroup_assert_mutex_or_rcu_locked() \
RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
@@ -2899,7 +2899,7 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
if (!cgrp)
return -ENODEV;
- percpu_down_write(&cgroup_threadgroup_rwsem);
+ down_write(&cgroup_threadgroup_rwsem);
rcu_read_lock();
if (pid) {
tsk = find_task_by_vpid(pid);
@@ -2937,7 +2937,7 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
out_unlock_rcu:
rcu_read_unlock();
out_unlock_threadgroup:
- percpu_up_write(&cgroup_threadgroup_rwsem);
+ up_write(&cgroup_threadgroup_rwsem);
for_each_subsys(ss, ssid)
if (ss->post_attach)
ss->post_attach();
@@ -3077,7 +3077,7 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
lockdep_assert_held(&cgroup_mutex);
- percpu_down_write(&cgroup_threadgroup_rwsem);
+ down_write(&cgroup_threadgroup_rwsem);
/* look up all csses currently attached to @cgrp's subtree */
spin_lock_bh(&css_set_lock);
@@ -3112,7 +3112,7 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
ret = cgroup_taskset_migrate(&tset, cgrp->root);
out_finish:
cgroup_migrate_finish(&preloaded_csets);
- percpu_up_write(&cgroup_threadgroup_rwsem);
+ up_write(&cgroup_threadgroup_rwsem);
return ret;
}
@@ -5601,7 +5601,7 @@ int __init cgroup_init(void)
int ssid;
BUILD_BUG_ON(CGROUP_SUBSYS_COUNT > 16);
- BUG_ON(percpu_init_rwsem(&cgroup_threadgroup_rwsem));
+ //BUG_ON(percpu_init_rwsem(&cgroup_threadgroup_rwsem));
BUG_ON(cgroup_init_cftypes(NULL, cgroup_dfl_base_files));
BUG_ON(cgroup_init_cftypes(NULL, cgroup_legacy_base_files));
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2016-07-13 22:20 +0200 |
| Message-ID | <rUyNs-7VU-11@gated-at.bofh.it> |
| In reply to | #1442754 |
Hello, John. On Wed, Jul 13, 2016 at 01:13:11PM -0700, John Stultz wrote: > On Wed, Jul 13, 2016 at 11:33 AM, Tejun Heo <tj@kernel.org> wrote: > > On Wed, Jul 13, 2016 at 02:21:02PM -0400, Tejun Heo wrote: > >> One interesting thing to try would be replacing it with a regular > >> non-percpu rwsem and see how it behaves. That should easily tell us > >> whether this is from actual contention or artifacts from percpu_rwsem > >> implementation. > > > > So, something like the following. Can you please see whether this > > makes any difference? > > Yea. So this brings it down for me closer to what we're seeing with > the Dmitry's patch reverting the two problematic commits, usually > 10-50us with one early spike at 18ms. So, it's a percpu rwsem issue then. I haven't really followed the perpcpu rwsem changes closely. Oleg, are multi-milisec delay expected on down write expected with the current implementation of percpu_rwsem? Thanks. -- tejun
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-07-13 22:30 +0200 |
| Message-ID | <rUyX7-7ZC-1@gated-at.bofh.it> |
| In reply to | #1442812 |
On Wed, Jul 13, 2016 at 04:18:23PM -0400, Tejun Heo wrote: > Hello, John. > > On Wed, Jul 13, 2016 at 01:13:11PM -0700, John Stultz wrote: > > On Wed, Jul 13, 2016 at 11:33 AM, Tejun Heo <tj@kernel.org> wrote: > > > On Wed, Jul 13, 2016 at 02:21:02PM -0400, Tejun Heo wrote: > > >> One interesting thing to try would be replacing it with a regular > > >> non-percpu rwsem and see how it behaves. That should easily tell us > > >> whether this is from actual contention or artifacts from percpu_rwsem > > >> implementation. > > > > > > So, something like the following. Can you please see whether this > > > makes any difference? > > > > Yea. So this brings it down for me closer to what we're seeing with > > the Dmitry's patch reverting the two problematic commits, usually > > 10-50us with one early spike at 18ms. > > So, it's a percpu rwsem issue then. I haven't really followed the > perpcpu rwsem changes closely. Oleg, are multi-milisec delay expected > on down write expected with the current implementation of > percpu_rwsem? There is a synchronize_sched() in there, so sorta. That thing is heavily geared towards readers, as is the only 'sane' choice for global locks.
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2016-07-13 22:50 +0200 |
| Message-ID | <rUzgu-87F-9@gated-at.bofh.it> |
| In reply to | #1442817 |
Hello,
On Wed, Jul 13, 2016 at 10:26:57PM +0200, Peter Zijlstra wrote:
> > So, it's a percpu rwsem issue then. I haven't really followed the
> > perpcpu rwsem changes closely. Oleg, are multi-milisec delay expected
> > on down write expected with the current implementation of
> > percpu_rwsem?
>
> There is a synchronize_sched() in there, so sorta. That thing is heavily
> geared towards readers, as is the only 'sane' choice for global locks.
It used to use the expedited variant until 001dac627ff3
("locking/percpu-rwsem: Make use of the rcu_sync infrastructure"), so
it might have been okay before then.
Skewing towards readers is fine but tens of millisecs of delays
definitely can't fit some use cases. There's a balance between CPU
overhead and latency here. If down writes are infrequent enough, it
doesn't make sense to aggressively trade off latency for lower
processing overhead for some use cases.
The options that I can see are
1. Somehow make percpu_rwsem's write behavior more responsive in a way
which is acceptable all use cases. This would be great but
probably impossible.
2. Add a fast-writer option to percpu_rwsem so that users which care
about write latency can opt in for higher processing overhead for
lower latency.
3. Implement a custom per-cpu locking construct for the particular use
case.
#3 would inherently be similar to #2 in its behavior. If #1 isn't
possible, #2 looks like the best course of action.
Thanks.
--
tejun
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-07-13 23:00 +0200 |
| Message-ID | <rUzqa-8bo-19@gated-at.bofh.it> |
| In reply to | #1442831 |
On Wed, Jul 13, 2016 at 04:39:44PM -0400, Tejun Heo wrote:
> > There is a synchronize_sched() in there, so sorta. That thing is heavily
> > geared towards readers, as is the only 'sane' choice for global locks.
>
> It used to use the expedited variant until 001dac627ff3
> ("locking/percpu-rwsem: Make use of the rcu_sync infrastructure"), so
> it might have been okay before then.
Right, but expedited stuff sprays IPIs around the entire system. That's
stuff other people complain about.
> The options that I can see are
>
> 1. Somehow make percpu_rwsem's write behavior more responsive in a way
> which is acceptable all use cases. This would be great but
> probably impossible.
>
> 2. Add a fast-writer option to percpu_rwsem so that users which care
> about write latency can opt in for higher processing overhead for
> lower latency.
So, IIRC, the trade-off is a full memory barrier in read_lock and
read_unlock() vs sync_sched() in write.
Full memory barriers are expensive and while the combined cost might
well exceed the cost of the sync_sched() it doesn't suffer the latency
issues.
Not sure if we can frob the two in a single codebase, but I can have a
poke if Oleg or Paul doesn't beat me to it.
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2016-07-13 23:10 +0200 |
| Message-ID | <rUzzP-8ue-3@gated-at.bofh.it> |
| In reply to | #1442844 |
On Wed, Jul 13, 2016 at 02:03:15PM -0700, Paul E. McKenney wrote: > Take the patch that I just sent out and make the choice of normal > vs. expedited depend on CONFIG_PREEMPT_RT or whatever the -rt guys are > calling it these days. Is there a low-latency Kconfig option other > than CONFIG_NO_HZ_FULL? Sounds like a plan to me. Thanks. -- tejun
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-07-13 23:20 +0200 |
| Message-ID | <rUzJv-7e-13@gated-at.bofh.it> |
| In reply to | #1442852 |
On Wed, Jul 13, 2016 at 05:05:26PM -0400, Tejun Heo wrote: > On Wed, Jul 13, 2016 at 02:03:15PM -0700, Paul E. McKenney wrote: > > Take the patch that I just sent out and make the choice of normal > > vs. expedited depend on CONFIG_PREEMPT_RT or whatever the -rt guys are > > calling it these days. Is there a low-latency Kconfig option other > > than CONFIG_NO_HZ_FULL? > > Sounds like a plan to me. I like the way we like each other's idea. Mutually assured laziness? ;-) Thanx, Paul
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-07-13 23:50 +0200 |
| Message-ID | <rUAcx-ja-9@gated-at.bofh.it> |
| In reply to | #1442866 |
On Wed, Jul 13, 2016 at 02:18:41PM -0700, Paul E. McKenney wrote:
> On Wed, Jul 13, 2016 at 05:05:26PM -0400, Tejun Heo wrote:
> > On Wed, Jul 13, 2016 at 02:03:15PM -0700, Paul E. McKenney wrote:
> > > Take the patch that I just sent out and make the choice of normal
> > > vs. expedited depend on CONFIG_PREEMPT_RT or whatever the -rt guys are
> > > calling it these days. Is there a low-latency Kconfig option other
> > > than CONFIG_NO_HZ_FULL?
> >
> > Sounds like a plan to me.
>
> I like the way we like each other's idea. Mutually assured laziness? ;-)
But here is what mine might look like. Untested, probably does
not even build. Note that the default is -no- expediting, use the
rcusync.expedited kernel parameter to enable it.
Thanx, Paul
------------------------------------------------------------------------
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 82b42c958d1c..b8bc9854e548 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3229,6 +3229,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
energy efficiency by requiring that the kthreads
periodically wake up to do the polling.
+ rcusync.expedited [KNL]
+ Specify that the rcusync mechanism use expedited
+ grace periods. As of mid-2016, this affects
+ per-CPU rwsems.
+
rcutree.blimit= [KNL]
Set maximum number of finished RCU callbacks to
process in one batch.
diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index be922c9f3d37..5bc5bef2e00a 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -22,6 +22,14 @@
#include <linux/rcu_sync.h>
#include <linux/sched.h>
+#include <linux/moduleparam.h>
+#include <linux/module.h>
+
+MODULE_ALIAS("rcusync");
+#ifdef MODULE_PARAM_PREFIX
+#undef MODULE_PARAM_PREFIX
+#endif
+#define MODULE_PARAM_PREFIX "rcusync."
#ifdef CONFIG_PROVE_RCU
#define __INIT_HELD(func) .held = func,
@@ -29,7 +37,7 @@
#define __INIT_HELD(func)
#endif
-static const struct {
+static struct {
void (*sync)(void);
void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
void (*wait)(void);
@@ -62,6 +70,20 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
#define rss_lock gp_wait.lock
+static bool expedited;
+module_param(expedited, bool, 0444);
+
+static int __init rcu_sync_early_init(void)
+{
+ if (expedited) {
+ gp_ops[RCU_SYNC].sync = synchronize_rcu_expedited;
+ gp_ops[RCU_SCHED_SYNC].sync = synchronize_sched_expedited;
+ gp_ops[RCU_BH_SYNC].sync = synchronize_rcu_bh_expedited;
+ }
+ return 0;
+}
+early_initcall(rcu_sync_early_init);
+
#ifdef CONFIG_PROVE_RCU
void rcu_sync_lockdep_assert(struct rcu_sync *rsp)
{
[toc] | [prev] | [next] | [standalone]
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2016-07-13 23:50 +0200 |
| Message-ID | <rUAcx-ja-17@gated-at.bofh.it> |
| In reply to | #1442883 |
On Wed, Jul 13, 2016 at 2:42 PM, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > On Wed, Jul 13, 2016 at 02:18:41PM -0700, Paul E. McKenney wrote: >> On Wed, Jul 13, 2016 at 05:05:26PM -0400, Tejun Heo wrote: >> > On Wed, Jul 13, 2016 at 02:03:15PM -0700, Paul E. McKenney wrote: >> > > Take the patch that I just sent out and make the choice of normal >> > > vs. expedited depend on CONFIG_PREEMPT_RT or whatever the -rt guys are >> > > calling it these days. Is there a low-latency Kconfig option other >> > > than CONFIG_NO_HZ_FULL? >> > >> > Sounds like a plan to me. >> >> I like the way we like each other's idea. Mutually assured laziness? ;-) > > But here is what mine might look like. Untested, probably does > not even build. Note that the default is -no- expediting, use the > rcusync.expedited kernel parameter to enable it. I was working on something similar, but using a config option. Would adding a config option for the default make sense here, since I'd probably prefer to have one less thing to always specify on the cmdline? thanks -john
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-07-14 00:20 +0200 |
| Message-ID | <rUAFz-Ll-11@gated-at.bofh.it> |
| In reply to | #1442886 |
On Wed, Jul 13, 2016 at 02:46:37PM -0700, John Stultz wrote:
> On Wed, Jul 13, 2016 at 2:42 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Jul 13, 2016 at 02:18:41PM -0700, Paul E. McKenney wrote:
> >> On Wed, Jul 13, 2016 at 05:05:26PM -0400, Tejun Heo wrote:
> >> > On Wed, Jul 13, 2016 at 02:03:15PM -0700, Paul E. McKenney wrote:
> >> > > Take the patch that I just sent out and make the choice of normal
> >> > > vs. expedited depend on CONFIG_PREEMPT_RT or whatever the -rt guys are
> >> > > calling it these days. Is there a low-latency Kconfig option other
> >> > > than CONFIG_NO_HZ_FULL?
> >> >
> >> > Sounds like a plan to me.
> >>
> >> I like the way we like each other's idea. Mutually assured laziness? ;-)
> >
> > But here is what mine might look like. Untested, probably does
> > not even build. Note that the default is -no- expediting, use the
> > rcusync.expedited kernel parameter to enable it.
>
> I was working on something similar, but using a config option. Would
> adding a config option for the default make sense here, since I'd
> probably prefer to have one less thing to always specify on the
> cmdline?
As long as you don't mind it depending on CONFIG_RCU_EXPERT, no problem.
Perhaps like the following, on top of the previous patch?
Or if you are going to put it in defconfig files only, I can make it
so that it isn't changeable at menuconfig time.
Thanx, Paul
------------------------------------------------------------------------
commit 57d8274b6906fad135a74beb59d4a99552659686
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date: Wed Jul 13 15:13:31 2016 -0700
rcu: Provide RCUSYNC_EXPEDITE option for rcusync.expedited default
This commit provides an RCUSYNC_EXPEDITE Kconfig option that specifies
the default value for the rcusync.expedited kernel parameter. This
makes it easier to use rcusync.expedited functionality in cases where
specifying kernel boot parameters should be avoided.
Reported-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
diff --git a/init/Kconfig b/init/Kconfig
index a068265fbcaf..de548d6ff82b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -782,6 +782,23 @@ config RCU_EXPEDITE_BOOT
Accept the default if unsure.
+config RCUSYNC_EXPEDITE
+ bool "Expedite rcusync operations for per-CPU rwsems"
+ depends on RCU_EXPERT
+ default n
+ help
+ Use this option to speed up per-CPU rwsem operations that are
+ in turn used by some cgroups operations. However, note well
+ that specifying this option will enable expedited RCU grace
+ periods. These expedited grace periods can in turn introduce
+ OS jitter, which can interfere with real-time, low-latency,
+ HPC, and userspace-polling RDMA workloads. That said, this
+ OS jitter will not affect CPUs that are in nohz_full mode for
+ the duration of the per-CPU rwsem operation in question.
+
+ Say Y here if you want fast cgroups at the expense of OS jitter.
+ Say N here if you are unsure.
+
endmenu # "RCU Subsystem"
config BUILD_BIN2C
diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index 5bc5bef2e00a..58c594b41d48 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -70,7 +70,7 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
#define rss_lock gp_wait.lock
-static bool expedited;
+static bool expedited = IS_ENABLED(CONFIG_RCUSYNC_EXPEDITE);
module_param(expedited, bool, 0444);
static int __init rcu_sync_early_init(void)
[toc] | [prev] | [next] | [standalone]
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2016-07-14 00:40 +0200 |
| Message-ID | <rUAYW-SU-5@gated-at.bofh.it> |
| In reply to | #1442914 |
On Wed, Jul 13, 2016 at 3:17 PM, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > On Wed, Jul 13, 2016 at 02:46:37PM -0700, John Stultz wrote: >> On Wed, Jul 13, 2016 at 2:42 PM, Paul E. McKenney >> <paulmck@linux.vnet.ibm.com> wrote: >> > On Wed, Jul 13, 2016 at 02:18:41PM -0700, Paul E. McKenney wrote: >> >> On Wed, Jul 13, 2016 at 05:05:26PM -0400, Tejun Heo wrote: >> >> > On Wed, Jul 13, 2016 at 02:03:15PM -0700, Paul E. McKenney wrote: >> >> > > Take the patch that I just sent out and make the choice of normal >> >> > > vs. expedited depend on CONFIG_PREEMPT_RT or whatever the -rt guys are >> >> > > calling it these days. Is there a low-latency Kconfig option other >> >> > > than CONFIG_NO_HZ_FULL? >> >> > >> >> > Sounds like a plan to me. >> >> >> >> I like the way we like each other's idea. Mutually assured laziness? ;-) >> > >> > But here is what mine might look like. Untested, probably does >> > not even build. Note that the default is -no- expediting, use the >> > rcusync.expedited kernel parameter to enable it. >> >> I was working on something similar, but using a config option. Would >> adding a config option for the default make sense here, since I'd >> probably prefer to have one less thing to always specify on the >> cmdline? > > As long as you don't mind it depending on CONFIG_RCU_EXPERT, no problem. > > Perhaps like the following, on top of the previous patch? > > Or if you are going to put it in defconfig files only, I can make it > so that it isn't changeable at menuconfig time. I think having it discoverable via menuconfig is useful, and I've got no objections to it being under RCU_EXPERT (assuming I don't badly muck up my RCU settings accidentally :). I only had that one nit about maybe wanting to put something in dmesg when we're using the expedited methods. But otherwise both patches look great and are working well! Do you mind marking them both for stable 4.4+? Tested-by: John Stultz <john.stultz@linaro.org> Acked-by: John Stultz <john.stultz@linaro.org> Also, do make sure Dmitry gets the reported-by credit for the first patch. thanks -john
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-07-14 01:10 +0200 |
| Message-ID | <rUBrX-1jq-9@gated-at.bofh.it> |
| In reply to | #1442924 |
On Wed, Jul 13, 2016 at 03:39:37PM -0700, John Stultz wrote:
> On Wed, Jul 13, 2016 at 3:17 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Jul 13, 2016 at 02:46:37PM -0700, John Stultz wrote:
> >> On Wed, Jul 13, 2016 at 2:42 PM, Paul E. McKenney
> >> <paulmck@linux.vnet.ibm.com> wrote:
> >> > On Wed, Jul 13, 2016 at 02:18:41PM -0700, Paul E. McKenney wrote:
> >> >> On Wed, Jul 13, 2016 at 05:05:26PM -0400, Tejun Heo wrote:
> >> >> > On Wed, Jul 13, 2016 at 02:03:15PM -0700, Paul E. McKenney wrote:
> >> >> > > Take the patch that I just sent out and make the choice of normal
> >> >> > > vs. expedited depend on CONFIG_PREEMPT_RT or whatever the -rt guys are
> >> >> > > calling it these days. Is there a low-latency Kconfig option other
> >> >> > > than CONFIG_NO_HZ_FULL?
> >> >> >
> >> >> > Sounds like a plan to me.
> >> >>
> >> >> I like the way we like each other's idea. Mutually assured laziness? ;-)
> >> >
> >> > But here is what mine might look like. Untested, probably does
> >> > not even build. Note that the default is -no- expediting, use the
> >> > rcusync.expedited kernel parameter to enable it.
> >>
> >> I was working on something similar, but using a config option. Would
> >> adding a config option for the default make sense here, since I'd
> >> probably prefer to have one less thing to always specify on the
> >> cmdline?
> >
> > As long as you don't mind it depending on CONFIG_RCU_EXPERT, no problem.
> >
> > Perhaps like the following, on top of the previous patch?
> >
> > Or if you are going to put it in defconfig files only, I can make it
> > so that it isn't changeable at menuconfig time.
>
> I think having it discoverable via menuconfig is useful, and I've got
> no objections to it being under RCU_EXPERT
> (assuming I don't badly muck up my RCU settings accidentally :).
But isn't mucking up your RCU settings half of the fun? ;-)
> I only had that one nit about maybe wanting to put something in dmesg
> when we're using the expedited methods.
Updated, please see below.
> But otherwise both patches look great and are working well!
>
> Do you mind marking them both for stable 4.4+?
OK, looks like it does qualify in the "fix a notable performance or
interactivity issue" category.
> Tested-by: John Stultz <john.stultz@linaro.org>
> Acked-by: John Stultz <john.stultz@linaro.org>
>
> Also, do make sure Dmitry gets the reported-by credit for the first patch.
Done! The updated first patch is below, and the second will follow.
Thanx, Paul
------------------------------------------------------------------------
commit 59435eb836ee73b30ed6ada525125b67b4029321
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date: Wed Jul 13 14:43:46 2016 -0700
rcu: Provide rcusync.expedited kernel boot parameter
Dmitry Shmidt and John Stultz noticed that __cgroup_procs_write()
sometimes incurred excessive overheads, ranging up into the tens of
milliseconds. Further testing confirmed speculation that this was due
to synchronize_sched() within rcusync being invoked by per-CPU rwsems.
This testing also showed that substituting synchronize_sched_expedited()
for synchronize_sched() greatly reduced the overheads to below 200
microseconds, with the occasional excursion into the low single digits
worth of milliseconds.
This commit therefore provides a rcusync.expedited kernel boot parameter
that causes rcusync to use expedited grace-period primitives.
Reported-by: Dmitry Shmidt <dimitrysh@google.com>
Reported-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Tested-by: John Stultz <john.stultz@linaro.org>
Acked-by: John Stultz <john.stultz@linaro.org>
Cc: <stable@vger.kernel.org> # 4.4.x-
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 82b42c958d1c..b8bc9854e548 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3229,6 +3229,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
energy efficiency by requiring that the kthreads
periodically wake up to do the polling.
+ rcusync.expedited [KNL]
+ Specify that the rcusync mechanism use expedited
+ grace periods. As of mid-2016, this affects
+ per-CPU rwsems.
+
rcutree.blimit= [KNL]
Set maximum number of finished RCU callbacks to
process in one batch.
diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index be922c9f3d37..0d0dc992cce7 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -22,6 +22,14 @@
#include <linux/rcu_sync.h>
#include <linux/sched.h>
+#include <linux/moduleparam.h>
+#include <linux/module.h>
+
+MODULE_ALIAS("rcusync");
+#ifdef MODULE_PARAM_PREFIX
+#undef MODULE_PARAM_PREFIX
+#endif
+#define MODULE_PARAM_PREFIX "rcusync."
#ifdef CONFIG_PROVE_RCU
#define __INIT_HELD(func) .held = func,
@@ -29,14 +37,14 @@
#define __INIT_HELD(func)
#endif
-static const struct {
+static struct {
void (*sync)(void);
void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
void (*wait)(void);
#ifdef CONFIG_PROVE_RCU
int (*held)(void);
#endif
-} gp_ops[] = {
+} gp_ops[] __read_mostly = {
[RCU_SYNC] = {
.sync = synchronize_rcu,
.call = call_rcu,
@@ -62,6 +70,21 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
#define rss_lock gp_wait.lock
+static bool expedited;
+module_param(expedited, bool, 0444);
+
+static int __init rcu_sync_early_init(void)
+{
+ if (expedited) {
+ pr_info("RCU_SYNC: Expedited operation in effect.\n");
+ gp_ops[RCU_SYNC].sync = synchronize_rcu_expedited;
+ gp_ops[RCU_SCHED_SYNC].sync = synchronize_sched_expedited;
+ gp_ops[RCU_BH_SYNC].sync = synchronize_rcu_bh_expedited;
+ }
+ return 0;
+}
+early_initcall(rcu_sync_early_init);
+
#ifdef CONFIG_PROVE_RCU
void rcu_sync_lockdep_assert(struct rcu_sync *rsp)
{
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-07-14 01:10 +0200 |
| Message-ID | <rUBrX-1jq-7@gated-at.bofh.it> |
| In reply to | #1442935 |
On Wed, Jul 13, 2016 at 04:02:38PM -0700, Paul E. McKenney wrote:
> On Wed, Jul 13, 2016 at 03:39:37PM -0700, John Stultz wrote:
> > On Wed, Jul 13, 2016 at 3:17 PM, Paul E. McKenney
> > <paulmck@linux.vnet.ibm.com> wrote:
> > > On Wed, Jul 13, 2016 at 02:46:37PM -0700, John Stultz wrote:
> > >> On Wed, Jul 13, 2016 at 2:42 PM, Paul E. McKenney
> > >> <paulmck@linux.vnet.ibm.com> wrote:
> > >> > On Wed, Jul 13, 2016 at 02:18:41PM -0700, Paul E. McKenney wrote:
> > >> >> On Wed, Jul 13, 2016 at 05:05:26PM -0400, Tejun Heo wrote:
> > >> >> > On Wed, Jul 13, 2016 at 02:03:15PM -0700, Paul E. McKenney wrote:
> > >> >> > > Take the patch that I just sent out and make the choice of normal
> > >> >> > > vs. expedited depend on CONFIG_PREEMPT_RT or whatever the -rt guys are
> > >> >> > > calling it these days. Is there a low-latency Kconfig option other
> > >> >> > > than CONFIG_NO_HZ_FULL?
> > >> >> >
> > >> >> > Sounds like a plan to me.
> > >> >>
> > >> >> I like the way we like each other's idea. Mutually assured laziness? ;-)
> > >> >
> > >> > But here is what mine might look like. Untested, probably does
> > >> > not even build. Note that the default is -no- expediting, use the
> > >> > rcusync.expedited kernel parameter to enable it.
> > >>
> > >> I was working on something similar, but using a config option. Would
> > >> adding a config option for the default make sense here, since I'd
> > >> probably prefer to have one less thing to always specify on the
> > >> cmdline?
> > >
> > > As long as you don't mind it depending on CONFIG_RCU_EXPERT, no problem.
> > >
> > > Perhaps like the following, on top of the previous patch?
> > >
> > > Or if you are going to put it in defconfig files only, I can make it
> > > so that it isn't changeable at menuconfig time.
> >
> > I think having it discoverable via menuconfig is useful, and I've got
> > no objections to it being under RCU_EXPERT
> > (assuming I don't badly muck up my RCU settings accidentally :).
>
> But isn't mucking up your RCU settings half of the fun? ;-)
>
> > I only had that one nit about maybe wanting to put something in dmesg
> > when we're using the expedited methods.
>
> Updated, please see below.
>
> > But otherwise both patches look great and are working well!
> >
> > Do you mind marking them both for stable 4.4+?
>
> OK, looks like it does qualify in the "fix a notable performance or
> interactivity issue" category.
>
> > Tested-by: John Stultz <john.stultz@linaro.org>
> > Acked-by: John Stultz <john.stultz@linaro.org>
> >
> > Also, do make sure Dmitry gets the reported-by credit for the first patch.
>
> Done! The updated first patch is below, and the second will follow.
As promised/threatened...
Thanx, Paul
------------------------------------------------------------------------
commit b4edebb8f5664a3a51be1e3ff3d7f1cb2d3d5c88
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date: Wed Jul 13 15:13:31 2016 -0700
rcu: Provide RCUSYNC_EXPEDITE option for rcusync.expedited default
This commit provides an RCUSYNC_EXPEDITE Kconfig option that specifies
the default value for the rcusync.expedited kernel parameter. This
makes it easier to use rcusync.expedited functionality in cases where
specifying kernel boot parameters should be avoided.
Reported-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Tested-by: John Stultz <john.stultz@linaro.org>
Acked-by: John Stultz <john.stultz@linaro.org>
Cc: <stable@vger.kernel.org> # 4.4.x-
diff --git a/init/Kconfig b/init/Kconfig
index a068265fbcaf..de548d6ff82b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -782,6 +782,23 @@ config RCU_EXPEDITE_BOOT
Accept the default if unsure.
+config RCUSYNC_EXPEDITE
+ bool "Expedite rcusync operations for per-CPU rwsems"
+ depends on RCU_EXPERT
+ default n
+ help
+ Use this option to speed up per-CPU rwsem operations that are
+ in turn used by some cgroups operations. However, note well
+ that specifying this option will enable expedited RCU grace
+ periods. These expedited grace periods can in turn introduce
+ OS jitter, which can interfere with real-time, low-latency,
+ HPC, and userspace-polling RDMA workloads. That said, this
+ OS jitter will not affect CPUs that are in nohz_full mode for
+ the duration of the per-CPU rwsem operation in question.
+
+ Say Y here if you want fast cgroups at the expense of OS jitter.
+ Say N here if you are unsure.
+
endmenu # "RCU Subsystem"
config BUILD_BIN2C
diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index 0d0dc992cce7..cc75d5071543 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -70,7 +70,7 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
#define rss_lock gp_wait.lock
-static bool expedited;
+static bool expedited = IS_ENABLED(CONFIG_RCUSYNC_EXPEDITE);
module_param(expedited, bool, 0444);
static int __init rcu_sync_early_init(void)
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2016-07-14 13:40 +0200 |
| Message-ID | <rUN9L-G3-21@gated-at.bofh.it> |
| In reply to | #1442936 |
Hello, On Wed, Jul 13, 2016 at 04:04:04PM -0700, Paul E. McKenney wrote: > commit b4edebb8f5664a3a51be1e3ff3d7f1cb2d3d5c88 > Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > Date: Wed Jul 13 15:13:31 2016 -0700 > > rcu: Provide RCUSYNC_EXPEDITE option for rcusync.expedited default > > This commit provides an RCUSYNC_EXPEDITE Kconfig option that specifies > the default value for the rcusync.expedited kernel parameter. This > makes it easier to use rcusync.expedited functionality in cases where > specifying kernel boot parameters should be avoided. > > Reported-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > Tested-by: John Stultz <john.stultz@linaro.org> > Acked-by: John Stultz <john.stultz@linaro.org> > Cc: <stable@vger.kernel.org> # 4.4.x- I think it probably makes sense to make this the default on !RT at least with a separate patch w/o stable cc'd. While most use cases will be fine with the latency on write path, it also means that the reader side is blocked for the duration which can hurt. rwsem implies a lot more readers and thus more read lock operations than writes. It's weird to trade off higher latency for lower cpu usage when it would also slow down all readers. Thanks. -- tejun
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-07-14 14:10 +0200 |
| Message-ID | <rUNCO-16b-13@gated-at.bofh.it> |
| In reply to | #1443378 |
On Thu, Jul 14, 2016 at 07:35:05AM -0400, Tejun Heo wrote: > Hello, > > On Wed, Jul 13, 2016 at 04:04:04PM -0700, Paul E. McKenney wrote: > > commit b4edebb8f5664a3a51be1e3ff3d7f1cb2d3d5c88 > > Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > Date: Wed Jul 13 15:13:31 2016 -0700 > > > > rcu: Provide RCUSYNC_EXPEDITE option for rcusync.expedited default > > > > This commit provides an RCUSYNC_EXPEDITE Kconfig option that specifies > > the default value for the rcusync.expedited kernel parameter. This > > makes it easier to use rcusync.expedited functionality in cases where > > specifying kernel boot parameters should be avoided. > > > > Reported-by: John Stultz <john.stultz@linaro.org> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > Tested-by: John Stultz <john.stultz@linaro.org> > > Acked-by: John Stultz <john.stultz@linaro.org> > > Cc: <stable@vger.kernel.org> # 4.4.x- > > I think it probably makes sense to make this the default on !RT at > least with a separate patch w/o stable cc'd. While most use cases > will be fine with the latency on write path, it also means that the > reader side is blocked for the duration which can hurt. rwsem implies > a lot more readers and thus more read lock operations than writes. > It's weird to trade off higher latency for lower cpu usage when it > would also slow down all readers. NAK, no expedited muck by default. There's more than just RT that doesn't like IPI sprays.
[toc] | [prev] | [next] | [standalone]
Page 1 of 4 [1] 2 3 4 Next page →
Back to top | Article view | linux.kernel
csiph-web