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

Started byChristian Borntraeger <borntraeger@de.ibm.com>
First post2015-09-15 14:10 +0200
Last post2015-09-16 20:50 +0200
Articles 11 on this page of 31 — 6 participants

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 2 of 2 — ← Prev page 1 [2]

#1225654

FromTejun Heo <tj@kernel.org>
Date2015-09-16 03:30 +0200
Message-ID<q99HP-6jL-1@gated-at.bofh.it>
In reply to#1225585
Hello, Paul.

On Tue, Sep 15, 2015 at 04:38:18PM -0700, Paul E. McKenney wrote:
> Well, the decision as to what is too big for -stable is owned by the
> -stable maintainers, not by me.

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

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

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

Thanks.

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

#1225713

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2015-09-16 06:40 +0200
Message-ID<q9cFH-2eH-5@gated-at.bofh.it>
In reply to#1225654
On Tue, Sep 15, 2015 at 09:24:15PM -0400, Tejun Heo wrote:
> Hello, Paul.
> 
> On Tue, Sep 15, 2015 at 04:38:18PM -0700, Paul E. McKenney wrote:
> > Well, the decision as to what is too big for -stable is owned by the
> > -stable maintainers, not by me.
> 
> Is it tho?  Usually the subsystem maintainer knows the best and has
> most say in it.  I was mostly curious whether you'd think that the
> changes would be too risky.  If not, great.

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

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

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

							Thanx, Paul

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

#1225998

FromTejun Heo <tj@kernel.org>
Date2015-09-16 13:10 +0200
Message-ID<q9iL7-2Ej-5@gated-at.bofh.it>
In reply to#1225713
Hello,

On Tue, Sep 15, 2015 at 09:35:47PM -0700, Paul E. McKenney wrote:
> > > I am suggesting trying the options and seeing what works best, then
> > > working to convince people as needed.
> > 
> > Yeah, sure thing.  Let's wait for Christian.
> 
> Indeed.  Is there enough benefit to risk jamming this thing into 4.3?
> I believe that 4.4 should be a no-brainer.

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

Thanks!

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

#1225803

FromChristian Borntraeger <borntraeger@de.ibm.com>
Date2015-09-16 09:50 +0200
Message-ID<q9fDA-6qt-19@gated-at.bofh.it>
In reply to#1225654
Am 16.09.2015 um 03:24 schrieb Tejun Heo:
> Hello, Paul.
> 
> On Tue, Sep 15, 2015 at 04:38:18PM -0700, Paul E. McKenney wrote:
>> Well, the decision as to what is too big for -stable is owned by the
>> -stable maintainers, not by me.
> 
> Is it tho?  Usually the subsystem maintainer knows the best and has
> most say in it.  I was mostly curious whether you'd think that the
> changes would be too risky.  If not, great.
> 
>> I am suggesting trying the options and seeing what works best, then
>> working to convince people as needed.
> 
> Yeah, sure thing.  Let's wait for Christian.

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

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

Christian

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

#1225995

FromChristian Borntraeger <borntraeger@de.ibm.com>
Date2015-09-16 13:00 +0200
Message-ID<q9iBr-2dA-15@gated-at.bofh.it>
In reply to#1225803
Am 16.09.2015 um 09:44 schrieb Christian Borntraeger:
> Am 16.09.2015 um 03:24 schrieb Tejun Heo:
>> Hello, Paul.
>>
>> On Tue, Sep 15, 2015 at 04:38:18PM -0700, Paul E. McKenney wrote:
>>> Well, the decision as to what is too big for -stable is owned by the
>>> -stable maintainers, not by me.
>>
>> Is it tho?  Usually the subsystem maintainer knows the best and has
>> most say in it.  I was mostly curious whether you'd think that the
>> changes would be too risky.  If not, great.
>>
>>> I am suggesting trying the options and seeing what works best, then
>>> working to convince people as needed.
>>
>> Yeah, sure thing.  Let's wait for Christian.
> 
> Well, I have optimized my testcase now that is puts enough pressure to
> the system to  confuses system (the older 209 version, which still has
> some event loop issues) that systemd restarts the journal deamon and does
> several other recoveries.
> To avoid regressions - even for somewhat shaky userspaces - we should
> consider a revert for 4.2 stable.
> There are several followup patches, which makes the revert non-trivial,
> though.
> 
> The rework of the percpu rwsem seems to work fine, but we are beyond the
> merge window so 4.4 seems better to me. (and consider a revert for 4.3)

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

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

Christian







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

#1226002

FromTejun Heo <tj@kernel.org>
Date2015-09-16 13:10 +0200
Message-ID<q9iL8-2Ej-29@gated-at.bofh.it>
In reply to#1225995
Hello,

On Wed, Sep 16, 2015 at 12:58:00PM +0200, Christian Borntraeger wrote:
> FWIW, I added a printk to percpu_down_write. With KVM and uprobes disabled,
> just booting up a fedora20 gives me __6749__ percpu_down_write calls on 4.2.
> systemd seems to do that for the processes. 
>
> So a revert is really the right thing to do. In fact, I dont know if the
> rcu_sync_enter rework is enough. With systemd setting the cgroup seem to
> be NOT a cold/seldom case.

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

Thanks.

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

#1226034

FromChristian Borntraeger <borntraeger@de.ibm.com>
Date2015-09-16 14:00 +0200
Message-ID<q9jxw-3z3-11@gated-at.bofh.it>
In reply to#1226002
Am 16.09.2015 um 13:03 schrieb Tejun Heo:
> Hello,
> 
> On Wed, Sep 16, 2015 at 12:58:00PM +0200, Christian Borntraeger wrote:
>> FWIW, I added a printk to percpu_down_write. With KVM and uprobes disabled,
>> just booting up a fedora20 gives me __6749__ percpu_down_write calls on 4.2.
>> systemd seems to do that for the processes. 
>>
>> So a revert is really the right thing to do. In fact, I dont know if the
>> rcu_sync_enter rework is enough. With systemd setting the cgroup seem to
>> be NOT a cold/seldom case.
> 
> Booting would usually be the hottest operation for that and it's still
> *relatively* cold path compared to the reader side which is task
> fork/exit paths.  The whole point is shift overhead from hotter reader
> side.  Can you see problems with percpu_rwsem rework?

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

Christian

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

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

FromTejun Heo <tj@kernel.org>
Date2015-09-16 18:00 +0200
Subject[PATCH cgroup/for-4.3-fixes 1/2] Revert "cgroup: simplify threadgroup locking"
Message-ID<q9nhN-vs-31@gated-at.bofh.it>
In reply to#1225029
From f9f9e7b776142fb1c0782cade004cc8e0147a199 Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Wed, 16 Sep 2015 11:51:12 -0400

This reverts commit b5ba75b5fc0e8404e2c50cb68f39bb6a53fc916f.

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

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

Signed-off-by: Tejun Heo <tj@kernel.org>
Link: http://lkml.kernel.org/g/55F8097A.7000206@de.ibm.com
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: stable@vger.kernel.org # v4.2+
---
Hello,

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

Thanks.

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 2cf0f79..115091e 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2460,13 +2460,14 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
 	if (!cgrp)
 		return -ENODEV;
 
-	percpu_down_write(&cgroup_threadgroup_rwsem);
+retry_find_task:
 	rcu_read_lock();
 	if (pid) {
 		tsk = find_task_by_vpid(pid);
 		if (!tsk) {
+			rcu_read_unlock();
 			ret = -ESRCH;
-			goto out_unlock_rcu;
+			goto out_unlock_cgroup;
 		}
 	} else {
 		tsk = current;
@@ -2482,23 +2483,37 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
 	 */
 	if (tsk == kthreadd_task || (tsk->flags & PF_NO_SETAFFINITY)) {
 		ret = -EINVAL;
-		goto out_unlock_rcu;
+		rcu_read_unlock();
+		goto out_unlock_cgroup;
 	}
 
 	get_task_struct(tsk);
 	rcu_read_unlock();
 
+	percpu_down_write(&cgroup_threadgroup_rwsem);
+	if (threadgroup) {
+		if (!thread_group_leader(tsk)) {
+			/*
+			 * a race with de_thread from another thread's exec()
+			 * may strip us of our leadership, if this happens,
+			 * there is no choice but to throw this task away and
+			 * try again; this is
+			 * "double-double-toil-and-trouble-check locking".
+			 */
+			percpu_up_write(&cgroup_threadgroup_rwsem);
+			put_task_struct(tsk);
+			goto retry_find_task;
+		}
+	}
+
 	ret = cgroup_procs_write_permission(tsk, cgrp, of);
 	if (!ret)
 		ret = cgroup_attach_task(cgrp, tsk, threadgroup);
 
-	put_task_struct(tsk);
-	goto out_unlock_threadgroup;
-
-out_unlock_rcu:
-	rcu_read_unlock();
-out_unlock_threadgroup:
 	percpu_up_write(&cgroup_threadgroup_rwsem);
+
+	put_task_struct(tsk);
+out_unlock_cgroup:
 	cgroup_kn_unlock(of->kn);
 	return ret ?: nbytes;
 }
@@ -2643,8 +2658,6 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
 
 	lockdep_assert_held(&cgroup_mutex);
 
-	percpu_down_write(&cgroup_threadgroup_rwsem);
-
 	/* look up all csses currently attached to @cgrp's subtree */
 	down_read(&css_set_rwsem);
 	css_for_each_descendant_pre(css, cgroup_css(cgrp, NULL)) {
@@ -2700,8 +2713,17 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
 				goto out_finish;
 			last_task = task;
 
+			percpu_down_write(&cgroup_threadgroup_rwsem);
+			/* raced against de_thread() from another thread? */
+			if (!thread_group_leader(task)) {
+				percpu_up_write(&cgroup_threadgroup_rwsem);
+				put_task_struct(task);
+				continue;
+			}
+
 			ret = cgroup_migrate(src_cset->dfl_cgrp, task, true);
 
+			percpu_up_write(&cgroup_threadgroup_rwsem);
 			put_task_struct(task);
 
 			if (WARN(ret, "cgroup: failed to update controllers for the default hierarchy (%d), further operations may crash or hang\n", ret))
@@ -2711,7 +2733,6 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
 
 out_finish:
 	cgroup_migrate_finish(&preloaded_csets);
-	percpu_up_write(&cgroup_threadgroup_rwsem);
 	return ret;
 }
 
-- 
2.4.3

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

#1226222 — [PATCH cgroup/for-4.3-fixes 2/2] Revert "sched, cgroup: replace signal_struct->group_rwsem with a global percpu_rwsem"

FromTejun Heo <tj@kernel.org>
Date2015-09-16 18:00 +0200
Subject[PATCH cgroup/for-4.3-fixes 2/2] Revert "sched, cgroup: replace signal_struct->group_rwsem with a global percpu_rwsem"
Message-ID<q9nhO-vs-45@gated-at.bofh.it>
In reply to#1226217
From 0c986253b939cc14c69d4adbe2b4121bdf4aa220 Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Wed, 16 Sep 2015 11:51:12 -0400

This reverts commit d59cfc09c32a2ae31f1c3bc2983a0cd79afb3f14.

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

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

Signed-off-by: Tejun Heo <tj@kernel.org>
Link: http://lkml.kernel.org/g/55F8097A.7000206@de.ibm.com
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: stable@vger.kernel.org # v4.2+
---
 include/linux/cgroup-defs.h | 27 ++--------------
 include/linux/init_task.h   |  8 +++++
 include/linux/sched.h       | 12 +++++++
 kernel/cgroup.c             | 77 +++++++++++++++++++++++++++++++++------------
 kernel/fork.c               |  4 +++
 5 files changed, 83 insertions(+), 45 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 4d8fcf2..8492721 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -473,31 +473,8 @@ struct cgroup_subsys {
 	unsigned int depends_on;
 };
 
-extern struct percpu_rw_semaphore cgroup_threadgroup_rwsem;
-
-/**
- * cgroup_threadgroup_change_begin - threadgroup exclusion for cgroups
- * @tsk: target task
- *
- * Called from threadgroup_change_begin() and allows cgroup operations to
- * synchronize against threadgroup changes using a percpu_rw_semaphore.
- */
-static inline void cgroup_threadgroup_change_begin(struct task_struct *tsk)
-{
-	percpu_down_read(&cgroup_threadgroup_rwsem);
-}
-
-/**
- * cgroup_threadgroup_change_end - threadgroup exclusion for cgroups
- * @tsk: target task
- *
- * Called from threadgroup_change_end().  Counterpart of
- * cgroup_threadcgroup_change_begin().
- */
-static inline void cgroup_threadgroup_change_end(struct task_struct *tsk)
-{
-	percpu_up_read(&cgroup_threadgroup_rwsem);
-}
+void cgroup_threadgroup_change_begin(struct task_struct *tsk);
+void cgroup_threadgroup_change_end(struct task_struct *tsk);
 
 #else	/* CONFIG_CGROUPS */
 
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index d0b380e..e38681f 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -25,6 +25,13 @@
 extern struct files_struct init_files;
 extern struct fs_struct init_fs;
 
+#ifdef CONFIG_CGROUPS
+#define INIT_GROUP_RWSEM(sig)						\
+	.group_rwsem = __RWSEM_INITIALIZER(sig.group_rwsem),
+#else
+#define INIT_GROUP_RWSEM(sig)
+#endif
+
 #ifdef CONFIG_CPUSETS
 #define INIT_CPUSET_SEQ(tsk)							\
 	.mems_allowed_seq = SEQCNT_ZERO(tsk.mems_allowed_seq),
@@ -57,6 +64,7 @@ extern struct fs_struct init_fs;
 	INIT_PREV_CPUTIME(sig)						\
 	.cred_guard_mutex =						\
 		 __MUTEX_INITIALIZER(sig.cred_guard_mutex),		\
+	INIT_GROUP_RWSEM(sig)						\
 }
 
 extern struct nsproxy init_nsproxy;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index a4ab9da..b7b9501 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -762,6 +762,18 @@ struct signal_struct {
 	unsigned audit_tty_log_passwd;
 	struct tty_audit_buf *tty_audit_buf;
 #endif
+#ifdef CONFIG_CGROUPS
+	/*
+	 * group_rwsem prevents new tasks from entering the threadgroup and
+	 * member tasks from exiting,a more specifically, setting of
+	 * PF_EXITING.  fork and exit paths are protected with this rwsem
+	 * using threadgroup_change_begin/end().  Users which require
+	 * threadgroup to remain stable should use threadgroup_[un]lock()
+	 * which also takes care of exec path.  Currently, cgroup is the
+	 * only user.
+	 */
+	struct rw_semaphore group_rwsem;
+#endif
 
 	oom_flags_t oom_flags;
 	short oom_score_adj;		/* OOM kill score adjustment */
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 115091e..2c9eae6 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -46,7 +46,6 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/rwsem.h>
-#include <linux/percpu-rwsem.h>
 #include <linux/string.h>
 #include <linux/sort.h>
 #include <linux/kmod.h>
@@ -104,8 +103,6 @@ static DEFINE_SPINLOCK(cgroup_idr_lock);
  */
 static DEFINE_SPINLOCK(release_agent_path_lock);
 
-struct percpu_rw_semaphore cgroup_threadgroup_rwsem;
-
 #define cgroup_assert_mutex_or_rcu_locked()				\
 	RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&			\
 			   !lockdep_is_held(&cgroup_mutex),		\
@@ -874,6 +871,48 @@ static struct css_set *find_css_set(struct css_set *old_cset,
 	return cset;
 }
 
+void cgroup_threadgroup_change_begin(struct task_struct *tsk)
+{
+	down_read(&tsk->signal->group_rwsem);
+}
+
+void cgroup_threadgroup_change_end(struct task_struct *tsk)
+{
+	up_read(&tsk->signal->group_rwsem);
+}
+
+/**
+ * threadgroup_lock - lock threadgroup
+ * @tsk: member task of the threadgroup to lock
+ *
+ * Lock the threadgroup @tsk belongs to.  No new task is allowed to enter
+ * and member tasks aren't allowed to exit (as indicated by PF_EXITING) or
+ * change ->group_leader/pid.  This is useful for cases where the threadgroup
+ * needs to stay stable across blockable operations.
+ *
+ * fork and exit explicitly call threadgroup_change_{begin|end}() for
+ * synchronization.  While held, no new task will be added to threadgroup
+ * and no existing live task will have its PF_EXITING set.
+ *
+ * de_thread() does threadgroup_change_{begin|end}() when a non-leader
+ * sub-thread becomes a new leader.
+ */
+static void threadgroup_lock(struct task_struct *tsk)
+{
+	down_write(&tsk->signal->group_rwsem);
+}
+
+/**
+ * threadgroup_unlock - unlock threadgroup
+ * @tsk: member task of the threadgroup to unlock
+ *
+ * Reverse threadgroup_lock().
+ */
+static inline void threadgroup_unlock(struct task_struct *tsk)
+{
+	up_write(&tsk->signal->group_rwsem);
+}
+
 static struct cgroup_root *cgroup_root_from_kf(struct kernfs_root *kf_root)
 {
 	struct cgroup *root_cgrp = kf_root->kn->priv;
@@ -2074,9 +2113,9 @@ static void cgroup_task_migrate(struct cgroup *old_cgrp,
 	lockdep_assert_held(&css_set_rwsem);
 
 	/*
-	 * We are synchronized through cgroup_threadgroup_rwsem against
-	 * PF_EXITING setting such that we can't race against cgroup_exit()
-	 * changing the css_set to init_css_set and dropping the old one.
+	 * We are synchronized through threadgroup_lock() against PF_EXITING
+	 * setting such that we can't race against cgroup_exit() changing the
+	 * css_set to init_css_set and dropping the old one.
 	 */
 	WARN_ON_ONCE(tsk->flags & PF_EXITING);
 	old_cset = task_css_set(tsk);
@@ -2133,11 +2172,10 @@ static void cgroup_migrate_finish(struct list_head *preloaded_csets)
  * @src_cset and add it to @preloaded_csets, which should later be cleaned
  * up by cgroup_migrate_finish().
  *
- * This function may be called without holding cgroup_threadgroup_rwsem
- * even if the target is a process.  Threads may be created and destroyed
- * but as long as cgroup_mutex is not dropped, no new css_set can be put
- * into play and the preloaded css_sets are guaranteed to cover all
- * migrations.
+ * This function may be called without holding threadgroup_lock even if the
+ * target is a process.  Threads may be created and destroyed but as long
+ * as cgroup_mutex is not dropped, no new css_set can be put into play and
+ * the preloaded css_sets are guaranteed to cover all migrations.
  */
 static void cgroup_migrate_add_src(struct css_set *src_cset,
 				   struct cgroup *dst_cgrp,
@@ -2240,7 +2278,7 @@ static int cgroup_migrate_prepare_dst(struct cgroup *dst_cgrp,
  * @threadgroup: whether @leader points to the whole process or a single task
  *
  * Migrate a process or task denoted by @leader to @cgrp.  If migrating a
- * process, the caller must be holding cgroup_threadgroup_rwsem.  The
+ * process, the caller must be holding threadgroup_lock of @leader.  The
  * caller is also responsible for invoking cgroup_migrate_add_src() and
  * cgroup_migrate_prepare_dst() on the targets before invoking this
  * function and following up with cgroup_migrate_finish().
@@ -2368,7 +2406,7 @@ static int cgroup_migrate(struct cgroup *cgrp, struct task_struct *leader,
  * @leader: the task or the leader of the threadgroup to be attached
  * @threadgroup: attach the whole threadgroup?
  *
- * Call holding cgroup_mutex and cgroup_threadgroup_rwsem.
+ * Call holding cgroup_mutex and threadgroup_lock of @leader.
  */
 static int cgroup_attach_task(struct cgroup *dst_cgrp,
 			      struct task_struct *leader, bool threadgroup)
@@ -2490,7 +2528,7 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
 	get_task_struct(tsk);
 	rcu_read_unlock();
 
-	percpu_down_write(&cgroup_threadgroup_rwsem);
+	threadgroup_lock(tsk);
 	if (threadgroup) {
 		if (!thread_group_leader(tsk)) {
 			/*
@@ -2500,7 +2538,7 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
 			 * try again; this is
 			 * "double-double-toil-and-trouble-check locking".
 			 */
-			percpu_up_write(&cgroup_threadgroup_rwsem);
+			threadgroup_unlock(tsk);
 			put_task_struct(tsk);
 			goto retry_find_task;
 		}
@@ -2510,7 +2548,7 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
 	if (!ret)
 		ret = cgroup_attach_task(cgrp, tsk, threadgroup);
 
-	percpu_up_write(&cgroup_threadgroup_rwsem);
+	threadgroup_unlock(tsk);
 
 	put_task_struct(tsk);
 out_unlock_cgroup:
@@ -2713,17 +2751,17 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
 				goto out_finish;
 			last_task = task;
 
-			percpu_down_write(&cgroup_threadgroup_rwsem);
+			threadgroup_lock(task);
 			/* raced against de_thread() from another thread? */
 			if (!thread_group_leader(task)) {
-				percpu_up_write(&cgroup_threadgroup_rwsem);
+				threadgroup_unlock(task);
 				put_task_struct(task);
 				continue;
 			}
 
 			ret = cgroup_migrate(src_cset->dfl_cgrp, task, true);
 
-			percpu_up_write(&cgroup_threadgroup_rwsem);
+			threadgroup_unlock(task);
 			put_task_struct(task);
 
 			if (WARN(ret, "cgroup: failed to update controllers for the default hierarchy (%d), further operations may crash or hang\n", ret))
@@ -5045,7 +5083,6 @@ int __init cgroup_init(void)
 	unsigned long key;
 	int ssid, err;
 
-	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));
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 7d5f0f1..2845623 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1149,6 +1149,10 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	tty_audit_fork(sig);
 	sched_autogroup_fork(sig);
 
+#ifdef CONFIG_CGROUPS
+	init_rwsem(&sig->group_rwsem);
+#endif
+
 	sig->oom_score_adj = current->signal->oom_score_adj;
 	sig->oom_score_adj_min = current->signal->oom_score_adj_min;
 
-- 
2.4.3

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

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

FromOleg Nesterov <oleg@redhat.com>
Date2015-09-16 19:10 +0200
SubjectRe: [PATCH cgroup/for-4.3-fixes 1/2] Revert "cgroup: simplify threadgroup locking"
Message-ID<q9onv-2h2-5@gated-at.bofh.it>
In reply to#1226217
On 09/16, Tejun Heo wrote:
>
> From f9f9e7b776142fb1c0782cade004cc8e0147a199 Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Wed, 16 Sep 2015 11:51:12 -0400
>
> This reverts commit b5ba75b5fc0e8404e2c50cb68f39bb6a53fc916f.
>
> d59cfc09c32a ("sched, cgroup: replace signal_struct->group_rwsem with
> a global percpu_rwsem") and b5ba75b5fc0e ("cgroup: simplify
> threadgroup locking") changed how cgroup synchronizes against task
> fork and exits so that it uses global percpu_rwsem instead of
> per-process rwsem; unfortunately, the write [un]lock paths of
> percpu_rwsem always involve synchronize_rcu_expedited() which turned
> out to be too expensive.
>
> Improvements for percpu_rwsem are scheduled to be merged in the coming
> v4.4-rc1 merge window which alleviates this issue.  For now, revert
> the two commits to restore per-process rwsem.  They will be re-applied
> for the v4.4-rc1 merge window.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Link: http://lkml.kernel.org/g/55F8097A.7000206@de.ibm.com
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: stable@vger.kernel.org # v4.2+

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

Oleg.

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

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

FromChristian Borntraeger <borntraeger@de.ibm.com>
Date2015-09-16 20:50 +0200
SubjectRe: [PATCH cgroup/for-4.3-fixes 1/2] Revert "cgroup: simplify threadgroup locking"
Message-ID<q9pWh-4mk-3@gated-at.bofh.it>
In reply to#1226217
Both patches in combination
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com> # on top of 4.3-rc1


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

Christian

--
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] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web