[PATCH 0/5] autogroup runtime enable/disable tuning fix

[PATCH 0/5] autogroup runtime enable/disable tuning fix

[Yong Zhang]

This patchset focus on autogroup runtime enable/disable tuning
issue.

Below is my test when disable autogroup at runtime:

vanilla:
  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND            
 1046 test1     20   0 25544  844  276 R 50.0  0.0   0:23.30 bash               
 1044 test2     20   0 25544  860  280 R 25.2  0.0   0:20.55 bash               
 1045 test2     20   0 25544  864  284 R 24.8  0.0   0:17.48 bash

patched:
  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND            
  991 test1     20   0 25544  844  276 R 31.7  0.0   2:05.93 bash               
  992 test2     20   0 25544  848  276 R 31.7  0.0   1:16.84 bash               
  994 test2     20   0 25544  856  284 R 29.8  0.0   1:02.67 bash 


---
Yong Zhang (5):
      sched, autogroup, sysctl: use proc_dointvec_minmax instead.
      sched: hide cfs_rq of autogroup if autogroup is disabled
      sched, autogroup: stop going ahead if autogroup is disabled
      sched: refactor task_group()
      sched, autogroup: runtime enable/disable tuning fix

 include/linux/sched.h    |    3 +++
 kernel/sched.c           |   10 +++++++++-
 kernel/sched_autogroup.c |   38 ++++++++++++++++++++++++++++++++++++++
 kernel/sched_autogroup.h |    5 +++++
 kernel/sched_debug.c     |    6 ++++++
 kernel/sysctl.c          |    2 +-
 6 files changed, 62 insertions(+), 2 deletions(-)

[PATCH 1/5] sched, autogroup, sysctl: use proc_dointvec_minmax instead.

[Yong Zhang]

sched_autogroup_enabled has min/max value, proc_dointvec_minmax()
is be used for this case.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <efault@gmx.de>
---
 kernel/sysctl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 0f1bd83..22e07ee 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -374,7 +374,7 @@ static struct ctl_table kern_table[] = {
 		.data		= &sysctl_sched_autogroup_enabled,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &zero,
 		.extra2		= &one,
 	},
-- 
1.7.1

[PATCH 2/5] sched: hide cfs_rq of autogroup if autogroup is disabled

[Yong Zhang]

When autogroup is disabled, sched_debug will show
all of the cfs_rq of autogroup by name "cfs_rq[0]:"
Actually that doesn't make sense and not necessary
to be showed.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <efault@gmx.de>
---
 kernel/sched_debug.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/kernel/sched_debug.c b/kernel/sched_debug.c
index eb6cb8e..121e2a2 100644
--- a/kernel/sched_debug.c
+++ b/kernel/sched_debug.c
@@ -169,6 +169,12 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)
 	struct sched_entity *last;
 	unsigned long flags;
 
+#ifdef CONFIG_SCHED_AUTOGROUP
+	int enabled = ACCESS_ONCE(sysctl_sched_autogroup_enabled);
+
+	if (task_group_is_autogroup(cfs_rq->tg) && !enabled)
+		return;
+#endif
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	SEQ_printf(m, "\ncfs_rq[%d]:%s\n", cpu, task_group_path(cfs_rq->tg));
 #else
-- 
1.7.1

[PATCH 3/5] sched, autogroup: stop going ahead if autogroup is disabled

[Yong Zhang]

when autogroup is disable from the beginning,
sched_autogroup_create_attach()
  autogroup_move_group()                    <== 1
    sched_move_task()                       <== 2
      task_move_group_fair()
        set_task_rq()
          task_group()
            autogroup_task_group()

We go the whole path without doing anything useful.

Then stop going further if autogroup is disabled.

But there will be a race window between 1 and 2, in which
sysctl_sched_autogroup_enabled is enabled. This issue
will be toke by following patch.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <efault@gmx.de>
---
 kernel/sched_autogroup.c |    4 ++++
 kernel/sched_autogroup.h |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/kernel/sched_autogroup.c b/kernel/sched_autogroup.c
index 9fb6562..137a096 100644
--- a/kernel/sched_autogroup.c
+++ b/kernel/sched_autogroup.c
@@ -161,11 +161,15 @@ autogroup_move_group(struct task_struct *p, struct autogroup *ag)
 
 	p->signal->autogroup = autogroup_kref_get(ag);
 
+	if (!ACCESS_ONCE(sysctl_sched_autogroup_enabled))
+		goto out;
+
 	t = p;
 	do {
 		sched_move_task(t);
 	} while_each_thread(p, t);
 
+out:
 	unlock_task_sighand(p, &flags);
 	autogroup_kref_put(prev);
 }
diff --git a/kernel/sched_autogroup.h b/kernel/sched_autogroup.h
index 7b859ff..0557705 100644
--- a/kernel/sched_autogroup.h
+++ b/kernel/sched_autogroup.h
@@ -1,6 +1,11 @@
 #ifdef CONFIG_SCHED_AUTOGROUP
 
 struct autogroup {
+	/*
+	 * reference doesn't mean how many thread attach to this
+	 * autogroup now. It just stands for the number of task
+	 * could use this autogroup.
+	 */
 	struct kref		kref;
 	struct task_group	*tg;
 	struct rw_semaphore	lock;
-- 
1.7.1

[PATCH 4/5] sched: refactor task_group()

[Yong Zhang]

move cgroup-related task group finding to cgroup_task_group().
No function change.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <efault@gmx.de>
---
 kernel/sched.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 18d38e4..5c74f79 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -601,7 +601,7 @@ static inline int cpu_of(struct rq *rq)
  * holds that lock for each task it moves into the cgroup. Therefore
  * by holding that lock, we pin the task to the current cgroup.
  */
-static inline struct task_group *task_group(struct task_struct *p)
+static inline struct task_group *cgroup_task_group(struct task_struct *p)
 {
 	struct task_group *tg;
 	struct cgroup_subsys_state *css;
@@ -613,6 +613,14 @@ static inline struct task_group *task_group(struct task_struct *p)
 			lockdep_is_held(&task_rq(p)->lock));
 	tg = container_of(css, struct task_group, css);
 
+	return tg;
+}
+
+static inline struct task_group *task_group(struct task_struct *p)
+{
+	struct task_group *tg;
+
+	tg = cgroup_task_group(p);
 	return autogroup_task_group(p, tg);
 }
 
-- 
1.7.1

[PATCH 5/5] sched, autogroup: runtime enable/disable tuning fix

[Yong Zhang]

Now enable/disable autogroup at runtime is just a flag,
it doesn't take effect on current process of the system.

when disable autogroup:

vanilla:
  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND            
 1046 test1     20   0 25544  844  276 R 50.0  0.0   0:23.30 bash               
 1044 test2     20   0 25544  860  280 R 25.2  0.0   0:20.55 bash               
 1045 test2     20   0 25544  864  284 R 24.8  0.0   0:17.48 bash

patched:
  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND            
  991 test1     20   0 25544  844  276 R 31.7  0.0   2:05.93 bash               
  992 test2     20   0 25544  848  276 R 31.7  0.0   1:16.84 bash               
  994 test2     20   0 25544  856  284 R 29.8  0.0   1:02.67 bash 

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <efault@gmx.de>
---
 include/linux/sched.h    |    3 +++
 kernel/sched_autogroup.c |   34 ++++++++++++++++++++++++++++++++++
 kernel/sysctl.c          |    2 +-
 3 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index d747f94..00791ce 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1949,6 +1949,9 @@ extern unsigned int sysctl_sched_compat_yield;
 
 #ifdef CONFIG_SCHED_AUTOGROUP
 extern unsigned int sysctl_sched_autogroup_enabled;
+extern int sysctl_sched_autogroup_handler(struct ctl_table *table, int write,
+		void __user *buffer, size_t *lenp,
+		loff_t *ppos);
 
 extern void sched_autogroup_create_attach(struct task_struct *p);
 extern void sched_autogroup_detach(struct task_struct *p);
diff --git a/kernel/sched_autogroup.c b/kernel/sched_autogroup.c
index 137a096..b1ad946 100644
--- a/kernel/sched_autogroup.c
+++ b/kernel/sched_autogroup.c
@@ -211,6 +211,40 @@ static int __init setup_autogroup(char *str)
 
 __setup("noautogroup", setup_autogroup);
 
+static DEFINE_MUTEX(autogroup_sysctl_mutex);
+
+int sysctl_sched_autogroup_handler(struct ctl_table *table, int write,
+		void __user *buffer, size_t *lenp,
+		loff_t *ppos)
+{
+	int old_value, ret;
+	struct task_struct *p, *n;
+
+	mutex_lock(&autogroup_sysctl_mutex);
+
+	old_value = sysctl_sched_autogroup_enabled;
+	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+
+	if (ret || !write || (old_value == sysctl_sched_autogroup_enabled))
+		goto out_unlock;
+
+	read_lock_irq(&tasklist_lock);
+	rcu_read_lock();
+
+	do_each_thread(n, p) {
+		if (cgroup_task_group(p) == &root_task_group)
+			sched_move_task(p);
+	} while_each_thread(n, p);
+
+	rcu_read_unlock();
+	read_unlock_irq(&tasklist_lock);
+
+out_unlock:
+	mutex_unlock(&autogroup_sysctl_mutex);
+
+	return ret;
+}
+
 #ifdef CONFIG_PROC_FS
 
 int proc_sched_autogroup_set_nice(struct task_struct *p, int *nice)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 22e07ee..2b12c65 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -374,7 +374,7 @@ static struct ctl_table kern_table[] = {
 		.data		= &sysctl_sched_autogroup_enabled,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
+		.proc_handler	= sysctl_sched_autogroup_handler,
 		.extra1		= &zero,
 		.extra2		= &one,
 	},
-- 
1.7.1

Re: [PATCH 5/5] sched, autogroup: runtime enable/disable tuning fix

[Mike Galbraith]

On Sun, 2011-02-20 at 15:08 +0800, Yong Zhang wrote:
> Now enable/disable autogroup at runtime is just a flag,
> it doesn't take effect on current process of the system.

Yup, they only move upon migration, which can take up to forever.  Linus
didn't like the idea of an active on/off switch, so it got whacked.

	-Mike

Re: [PATCH 5/5] sched, autogroup: runtime enable/disable tuning fix

[Yong Zhang]

On Sun, Feb 20, 2011 at 02:09:39PM +0100, Mike Galbraith wrote:
> On Sun, 2011-02-20 at 15:08 +0800, Yong Zhang wrote:
> > Now enable/disable autogroup at runtime is just a flag,
> > it doesn't take effect on current process of the system.
> 
> Yup, they only move upon migration, which can take up to forever.

Yeah. so on UP, the user can have autogroup forever or not, and
sched_autogroup_enable is a nop.

> Linus
> didn't like the idea of an active on/off switch, so it got whacked.

Seems I missed that very thread.


Apart from runtime enable/disable issue, I think patch1~3 could
still be applied beause they really fix some problem or bring
some improvement.

Thanks,
Yong

Re: [PATCH 5/5] sched, autogroup: runtime enable/disable tuning fix

[Mike Galbraith]

On Sun, 2011-02-20 at 22:10 +0800, Yong Zhang wrote:
> On Sun, Feb 20, 2011 at 02:09:39PM +0100, Mike Galbraith wrote:
> > On Sun, 2011-02-20 at 15:08 +0800, Yong Zhang wrote:
> > > Now enable/disable autogroup at runtime is just a flag,
> > > it doesn't take effect on current process of the system.
> > 
> > Yup, they only move upon migration, which can take up to forever.
> 
> Yeah. so on UP, the user can have autogroup forever or not, and
> sched_autogroup_enable is a nop.

Yup, and that's the best argument for doing the active switch.

> Apart from runtime enable/disable issue, I think patch1~3 could
> still be applied beause they really fix some problem or bring
> some improvement.

Yeah.

Your whole series looked fine to me at a glance (sunday;), with the
exception of #2, that one is maybe iffy, depending on point of view.

It's iffy only in that the proc interface is there to show the group
(whether used or not, it's not about tasks really, just group), so the
user can easily twiddle it if he so desires, which has nada to do with
the group being active or not.

That said, I'd agree to whacking the proc interface wholesale in a
heartbeat.  It's really a wart.  If it were frequently used, it'd be
indicative of failure for the whole concept.

	-Mike

Re: [PATCH 5/5] sched, autogroup: runtime enable/disable tuning fix

[Mike Galbraith]

On Sun, 2011-02-20 at 18:22 +0100, Mike Galbraith wrote:
> On Sun, 2011-02-20 at 22:10 +0800, Yong Zhang wrote:
> > On Sun, Feb 20, 2011 at 02:09:39PM +0100, Mike Galbraith wrote:
> > > On Sun, 2011-02-20 at 15:08 +0800, Yong Zhang wrote:
> > > > Now enable/disable autogroup at runtime is just a flag,
> > > > it doesn't take effect on current process of the system.
> > > 
> > > Yup, they only move upon migration, which can take up to forever.
> > 
> > Yeah. so on UP, the user can have autogroup forever or not, and
> > sched_autogroup_enable is a nop.
> 
> Yup, and that's the best argument for doing the active switch.
> 
> > Apart from runtime enable/disable issue, I think patch1~3 could
> > still be applied beause they really fix some problem or bring
> > some improvement.
> 
> Yeah.
> 
> Your whole series looked fine to me at a glance (sunday;), with the
> exception of #2, that one is maybe iffy, depending on point of view.

Hm, my mind wandered off to different file somewhere along the line :)

	-Mike

Re: [PATCH 5/5] sched, autogroup: runtime enable/disable tuning fix

[Yong Zhang]

On Mon, Feb 21, 2011 at 11:13 AM, Mike Galbraith <efault@gmx.de> wrote:
>> Your whole series looked fine to me at a glance (sunday;), with the
>> exception of #2, that one is maybe iffy, depending on point of view.
>
> Hm, my mind wandered off to different file somewhere along the line :)

Never mind :)

If we relax the runtime autogroup switch, maybe patch-2 is a little
overkill, because after autogroup is disabled there still could have
some tasks staying on it's autogroup.
So if we wan to keep current autogroup switch behavior, I think
we can make autogroup_path() independent on
sysctl_sched_autogroup_enabled, thus user will not be confused
by lots of groups named "cfs_rq[*]:"

But if this patch is accepted, IMHO, patch-2 is ok.

Thanks,
Yong


-- 
Only stand for myself

Re: [PATCH 5/5] sched, autogroup: runtime enable/disable tuning fix

[Peter Zijlstra]

On Sun, 2011-02-20 at 18:22 +0100, Mike Galbraith wrote:
> 
> Your whole series looked fine to me at a glance (sunday;), with the
> exception of #2, that one is maybe iffy, depending on point of view.

Right, so I've queued 1 and 3, 2 is like you said iffy, if there's an
autogroup it should display it.

As to 5, I really don't care, Linus did seem to care initially, don't
know how he feels about it now.

Re: [PATCH 5/5] sched, autogroup: runtime enable/disable tuning fix

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Sun, 2011-02-20 at 18:22 +0100, Mike Galbraith wrote:
> > 
> > Your whole series looked fine to me at a glance (sunday;), with the
> > exception of #2, that one is maybe iffy, depending on point of view.
> 
> Right, so I've queued 1 and 3, 2 is like you said iffy, if there's an
> autogroup it should display it.
> 
> As to 5, I really don't care, Linus did seem to care initially, don't
> know how he feels about it now.

I've Cc:-ed him.

Since Yong Zhang has noticed the difference and has written and tested the patch we 
should consider it i think, it's a QoI improvement. The knob is root-only, right?

Thanks,

	Ingo

Re: [PATCH 5/5] sched, autogroup: runtime enable/disable tuning fix

[Yong Zhang]

On Tue, Feb 22, 2011 at 12:36 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Sun, 2011-02-20 at 18:22 +0100, Mike Galbraith wrote:
>>
>> Your whole series looked fine to me at a glance (sunday;), with the
>> exception of #2, that one is maybe iffy, depending on point of view.
>
> Right, so I've queued 1 and 3, 2 is like you said iffy, if there's an
> autogroup it should display it.

So how about replacing patch-2 by below one for now?

---
From: Yong Zhang <yong.zhang0@gmail.com>
Subject: [PATCH] sched, autogroup: always show autogroup name in sched_debug

When autogroup is disabled, there will be lots of group
named "cfs_rq[cpu]:", thus will lead to confusion for
who read it.
And for now, autogroup runtime disable/enable will
not take effect immediately on current live processes,
so there maybe still have some processes attaching to
its autogroup.
So show autogroup name always. But for the root group,
its name will dance between "cfs_rq[cpu]:" and
"cfs_rq[cpu]:/autogroup-0" according to
sysctl_sched_autogroup_enabled.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <efault@gmx.de>
---
 kernel/sched_autogroup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched_autogroup.c b/kernel/sched_autogroup.c
index 64919dc..6114e3e 100644
--- a/kernel/sched_autogroup.c
+++ b/kernel/sched_autogroup.c
@@ -301,7 +301,7 @@ static inline int autogroup_path(struct task_group
*tg, char *buf, int buflen)
 {
 	int enabled = ACCESS_ONCE(sysctl_sched_autogroup_enabled);

-	if (!enabled || !tg->autogroup)
+	if (!tg->autogroup || (!tg->autogroup->id && !enabled))
 		return 0;

 	return snprintf(buf, buflen, "%s-%ld", "/autogroup", tg->autogroup->id);
-- 
1.7.1
-- 
Only stand for myself

Re: [PATCH 5/5] sched, autogroup: runtime enable/disable tuning fix

[Peter Zijlstra]

On Tue, 2011-02-22 at 11:13 +0800, Yong Zhang wrote:
> So show autogroup name always. But for the root group,
> its name will dance between "cfs_rq[cpu]:" and
> "cfs_rq[cpu]:/autogroup-0" according to
> sysctl_sched_autogroup_enabled. 

Would it make sense to force the root group to always be a !autogroup?

[PATCH V2] sched, autogroup: always show autogroup name in sched_debug

[Yong Zhang]

On Tue, Feb 22, 2011 at 10:56:06AM +0100, Peter Zijlstra wrote:
> On Tue, 2011-02-22 at 11:13 +0800, Yong Zhang wrote:
> > So show autogroup name always. But for the root group,
> > its name will dance between "cfs_rq[cpu]:" and
> > "cfs_rq[cpu]:/autogroup-0" according to
> > sysctl_sched_autogroup_enabled. 
> 
> Would it make sense to force the root group to always be a !autogroup?

Hmmm, I think it's ok.

---
From: Yong Zhang <yong.zhang0@gmail.com>
Subject: [PATCH] sched, autogroup: always show autogroup name in sched_debug

When autogroup is disabled, there will be lots of group
named "cfs_rq[cpu]:", thus will lead to confusion for
who read it.
And for now, autogroup runtime disable/enable will
not take effect immediately on current live processes,
so there maybe still have some processes attaching to
its autogroup.
So show autogroup name always. But for the root group,
its name will always be "cfs_rq[cpu]:/".

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <efault@gmx.de>
---
 kernel/sched_autogroup.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/kernel/sched_autogroup.c b/kernel/sched_autogroup.c
index 9fb6562..5b9a317 100644
--- a/kernel/sched_autogroup.c
+++ b/kernel/sched_autogroup.c
@@ -258,9 +258,7 @@ void proc_sched_autogroup_show_task(struct task_struct *p, struct seq_file *m)
 #ifdef CONFIG_SCHED_DEBUG
 static inline int autogroup_path(struct task_group *tg, char *buf, int buflen)
 {
-	int enabled = ACCESS_ONCE(sysctl_sched_autogroup_enabled);
-
-	if (!enabled || !tg->autogroup)
+	if (!tg->autogroup || !tg->autogroup->id)
 		return 0;
 
 	return snprintf(buf, buflen, "%s-%ld", "/autogroup", tg->autogroup->id);
-- 
1.7.1

Re: [PATCH V2] sched, autogroup: always show autogroup name in sched_debug

[Mike Galbraith]

On Tue, 2011-02-22 at 20:10 +0800, Yong Zhang wrote:
> On Tue, Feb 22, 2011 at 10:56:06AM +0100, Peter Zijlstra wrote:
> > On Tue, 2011-02-22 at 11:13 +0800, Yong Zhang wrote:
> > > So show autogroup name always. But for the root group,
> > > its name will dance between "cfs_rq[cpu]:" and
> > > "cfs_rq[cpu]:/autogroup-0" according to
> > > sysctl_sched_autogroup_enabled. 
> > 
> > Would it make sense to force the root group to always be a !autogroup?
> 
> Hmmm, I think it's ok.

Yup, it's not an autogroup, so let's stop claiming it.

sched, autogroup: stop claiming ownership of the root task group.

Disown it, and only display autogroup association if one exists.

Signed-off-by: Mike Galbraith <efault@gmx.de>

diff --git a/kernel/sched_autogroup.c b/kernel/sched_autogroup.c
index 9fb6562..506faa0 100644
--- a/kernel/sched_autogroup.c
+++ b/kernel/sched_autogroup.c
@@ -12,7 +12,6 @@ static atomic_t autogroup_seq_nr;
 static void __init autogroup_init(struct task_struct *init_task)
 {
 	autogroup_default.tg = &root_task_group;
-	root_task_group.autogroup = &autogroup_default;
 	kref_init(&autogroup_default.kref);
 	init_rwsem(&autogroup_default.lock);
 	init_task->signal->autogroup = &autogroup_default;
@@ -130,7 +129,7 @@ task_wants_autogroup(struct task_struct *p, struct task_group *tg)
 
 static inline bool task_group_is_autogroup(struct task_group *tg)
 {
-	return tg != &root_task_group && tg->autogroup;
+	return !!tg->autogroup;
 }
 
 static inline struct task_group *
@@ -247,10 +246,14 @@ void proc_sched_autogroup_show_task(struct task_struct *p, struct seq_file *m)
 {
 	struct autogroup *ag = autogroup_task_get(p);
 
+	if (!task_group_is_autogroup(ag->tg))
+		goto out;
+
 	down_read(&ag->lock);
 	seq_printf(m, "/autogroup-%ld nice %d\n", ag->id, ag->nice);
 	up_read(&ag->lock);
 
+out:
 	autogroup_kref_put(ag);
 }
 #endif /* CONFIG_PROC_FS */
@@ -258,9 +261,7 @@ void proc_sched_autogroup_show_task(struct task_struct *p, struct seq_file *m)
 #ifdef CONFIG_SCHED_DEBUG
 static inline int autogroup_path(struct task_group *tg, char *buf, int buflen)
 {
-	int enabled = ACCESS_ONCE(sysctl_sched_autogroup_enabled);
-
-	if (!enabled || !tg->autogroup)
+	if (!task_group_is_autogroup(tg))
 		return 0;
 
 	return snprintf(buf, buflen, "%s-%ld", "/autogroup", tg->autogroup->id);

Re: [PATCH V2] sched, autogroup: always show autogroup name in sched_debug

[Yong Zhang]

On Tue, Feb 22, 2011 at 03:02:00PM +0100, Mike Galbraith wrote:
> On Tue, 2011-02-22 at 20:10 +0800, Yong Zhang wrote:
> > On Tue, Feb 22, 2011 at 10:56:06AM +0100, Peter Zijlstra wrote:
> > > On Tue, 2011-02-22 at 11:13 +0800, Yong Zhang wrote:
> > > > So show autogroup name always. But for the root group,
> > > > its name will dance between "cfs_rq[cpu]:" and
> > > > "cfs_rq[cpu]:/autogroup-0" according to
> > > > sysctl_sched_autogroup_enabled. 
> > > 
> > > Would it make sense to force the root group to always be a !autogroup?
> > 
> > Hmmm, I think it's ok.
> 
> Yup, it's not an autogroup, so let's stop claiming it.
> 
> sched, autogroup: stop claiming ownership of the root task group.
> 
> Disown it, and only display autogroup association if one exists.
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>

Ah, I like this one better :)
Actually I misunderstanded what Peter mean.

Reviewed-by: Yong Zhang <yong.zhang0@gmail.com>

Thanks,
Yong
> 
> diff --git a/kernel/sched_autogroup.c b/kernel/sched_autogroup.c
> index 9fb6562..506faa0 100644
> --- a/kernel/sched_autogroup.c
> +++ b/kernel/sched_autogroup.c
> @@ -12,7 +12,6 @@ static atomic_t autogroup_seq_nr;
>  static void __init autogroup_init(struct task_struct *init_task)
>  {
>  	autogroup_default.tg = &root_task_group;
> -	root_task_group.autogroup = &autogroup_default;
>  	kref_init(&autogroup_default.kref);
>  	init_rwsem(&autogroup_default.lock);
>  	init_task->signal->autogroup = &autogroup_default;
> @@ -130,7 +129,7 @@ task_wants_autogroup(struct task_struct *p, struct task_group *tg)
>  
>  static inline bool task_group_is_autogroup(struct task_group *tg)
>  {
> -	return tg != &root_task_group && tg->autogroup;
> +	return !!tg->autogroup;
>  }
>  
>  static inline struct task_group *
> @@ -247,10 +246,14 @@ void proc_sched_autogroup_show_task(struct task_struct *p, struct seq_file *m)
>  {
>  	struct autogroup *ag = autogroup_task_get(p);
>  
> +	if (!task_group_is_autogroup(ag->tg))
> +		goto out;
> +
>  	down_read(&ag->lock);
>  	seq_printf(m, "/autogroup-%ld nice %d\n", ag->id, ag->nice);
>  	up_read(&ag->lock);
>  
> +out:
>  	autogroup_kref_put(ag);
>  }
>  #endif /* CONFIG_PROC_FS */
> @@ -258,9 +261,7 @@ void proc_sched_autogroup_show_task(struct task_struct *p, struct seq_file *m)
>  #ifdef CONFIG_SCHED_DEBUG
>  static inline int autogroup_path(struct task_group *tg, char *buf, int buflen)
>  {
> -	int enabled = ACCESS_ONCE(sysctl_sched_autogroup_enabled);
> -
> -	if (!enabled || !tg->autogroup)
> +	if (!task_group_is_autogroup(tg))
>  		return 0;
>  
>  	return snprintf(buf, buflen, "%s-%ld", "/autogroup", tg->autogroup->id);
> 

[tip:sched/core] sched, autogroup, sysctl: Use proc_dointvec_minmax() instead

[tip-bot for Yong Zhang]

Commit-ID:  1747b21fecbfb63fbf6b9624e8b92707960d5a97
Gitweb:     http://git.kernel.org/tip/1747b21fecbfb63fbf6b9624e8b92707960d5a97
Author:     Yong Zhang <yong.zhang0@gmail.com>
AuthorDate: Sun, 20 Feb 2011 15:08:12 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 23 Feb 2011 11:33:58 +0100

sched, autogroup, sysctl: Use proc_dointvec_minmax() instead

sched_autogroup_enabled has min/max value, proc_dointvec_minmax() is
be used for this case.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <1298185696-4403-2-git-send-email-yong.zhang0@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sysctl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index cb7c830..7b5eead 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -367,7 +367,7 @@ static struct ctl_table kern_table[] = {
 		.data		= &sysctl_sched_autogroup_enabled,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &zero,
 		.extra2		= &one,
 	},

[tip:sched/core] sched, autogroup: Stop going ahead if autogroup is disabled

[tip-bot for Yong Zhang]

Commit-ID:  800d4d30c8f20bd728e5741a3b77c4859a613f7c
Gitweb:     http://git.kernel.org/tip/800d4d30c8f20bd728e5741a3b77c4859a613f7c
Author:     Yong Zhang <yong.zhang0@gmail.com>
AuthorDate: Sun, 20 Feb 2011 15:08:14 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 23 Feb 2011 11:33:59 +0100

sched, autogroup: Stop going ahead if autogroup is disabled

when autogroup is disable from the beginning,
sched_autogroup_create_attach()
  autogroup_move_group()                    <== 1
    sched_move_task()                       <== 2
      task_move_group_fair()
        set_task_rq()
          task_group()
            autogroup_task_group()

We go the whole path without doing anything useful.

Then stop going further if autogroup is disabled.

But there will be a race window between 1 and 2, in which
sysctl_sched_autogroup_enabled is enabled. This issue
will be toke by following patch.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Galbraith <efault@gmx.de>
LKML-Reference: <1298185696-4403-4-git-send-email-yong.zhang0@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched_autogroup.c |    4 ++++
 kernel/sched_autogroup.h |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/kernel/sched_autogroup.c b/kernel/sched_autogroup.c
index 9fb6562..137a096 100644
--- a/kernel/sched_autogroup.c
+++ b/kernel/sched_autogroup.c
@@ -161,11 +161,15 @@ autogroup_move_group(struct task_struct *p, struct autogroup *ag)
 
 	p->signal->autogroup = autogroup_kref_get(ag);
 
+	if (!ACCESS_ONCE(sysctl_sched_autogroup_enabled))
+		goto out;
+
 	t = p;
 	do {
 		sched_move_task(t);
 	} while_each_thread(p, t);
 
+out:
 	unlock_task_sighand(p, &flags);
 	autogroup_kref_put(prev);
 }
diff --git a/kernel/sched_autogroup.h b/kernel/sched_autogroup.h
index 7b859ff..0557705 100644
--- a/kernel/sched_autogroup.h
+++ b/kernel/sched_autogroup.h
@@ -1,6 +1,11 @@
 #ifdef CONFIG_SCHED_AUTOGROUP
 
 struct autogroup {
+	/*
+	 * reference doesn't mean how many thread attach to this
+	 * autogroup now. It just stands for the number of task
+	 * could use this autogroup.
+	 */
 	struct kref		kref;
 	struct task_group	*tg;
 	struct rw_semaphore	lock;

[tip:sched/core] sched, autogroup: Stop claiming ownership of the root task group

[tip-bot for Mike Galbraith]

Commit-ID:  511f67a5997c4967c69a3961e2fc9f04d8d244ac
Gitweb:     http://git.kernel.org/tip/511f67a5997c4967c69a3961e2fc9f04d8d244ac
Author:     Mike Galbraith <efault@gmx.de>
AuthorDate: Tue, 22 Feb 2011 15:02:00 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 23 Feb 2011 11:34:03 +0100

sched, autogroup: Stop claiming ownership of the root task group

Disown it, and only display autogroup association if one exists.

Signed-off-by: Mike Galbraith <efault@gmx.de>
Reviewed-by: Yong Zhang <yong.zhang0@gmail.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <1298383320.8036.5.camel@marge.simson.net>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched_autogroup.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/sched_autogroup.c b/kernel/sched_autogroup.c
index 137a096..5946ac5 100644
--- a/kernel/sched_autogroup.c
+++ b/kernel/sched_autogroup.c
@@ -12,7 +12,6 @@ static atomic_t autogroup_seq_nr;
 static void __init autogroup_init(struct task_struct *init_task)
 {
 	autogroup_default.tg = &root_task_group;
-	root_task_group.autogroup = &autogroup_default;
 	kref_init(&autogroup_default.kref);
 	init_rwsem(&autogroup_default.lock);
 	init_task->signal->autogroup = &autogroup_default;
@@ -130,7 +129,7 @@ task_wants_autogroup(struct task_struct *p, struct task_group *tg)
 
 static inline bool task_group_is_autogroup(struct task_group *tg)
 {
-	return tg != &root_task_group && tg->autogroup;
+	return !!tg->autogroup;
 }
 
 static inline struct task_group *
@@ -251,10 +250,14 @@ void proc_sched_autogroup_show_task(struct task_struct *p, struct seq_file *m)
 {
 	struct autogroup *ag = autogroup_task_get(p);
 
+	if (!task_group_is_autogroup(ag->tg))
+		goto out;
+
 	down_read(&ag->lock);
 	seq_printf(m, "/autogroup-%ld nice %d\n", ag->id, ag->nice);
 	up_read(&ag->lock);
 
+out:
 	autogroup_kref_put(ag);
 }
 #endif /* CONFIG_PROC_FS */
@@ -262,9 +265,7 @@ void proc_sched_autogroup_show_task(struct task_struct *p, struct seq_file *m)
 #ifdef CONFIG_SCHED_DEBUG
 static inline int autogroup_path(struct task_group *tg, char *buf, int buflen)
 {
-	int enabled = ACCESS_ONCE(sysctl_sched_autogroup_enabled);
-
-	if (!enabled || !tg->autogroup)
+	if (!task_group_is_autogroup(tg))
 		return 0;
 
 	return snprintf(buf, buflen, "%s-%ld", "/autogroup", tg->autogroup->id);