autogroup runtime enable/disable tuning fix

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

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

---
 kernel/sysctl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 0f1bd83..22e07ee 100644
@@ -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

Commit-ID:  1747b21fecbfb63fbf6b9624e8b92707960d5a97
Gitweb:     http://git.kernel.org/tip/1747b21fecbfb63fbf6b9624e8b92707960d5a97
Author:     Yong Zhang <[email protected]>
AuthorDate: Sun, 20 Feb 2011 15:08:12 +0800
Committer:  Ingo Molnar <[email protected]>
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.

LKML-Reference: <[email protected]>

---
 kernel/sysctl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index cb7c830..7b5eead 100644
@@ -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,
 	},

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.

---
 kernel/sched_debug.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

index eb6cb8e..121e2a2 100644
@@ -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

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.

---
 kernel/sched_autogroup.c |    4 ++++
 kernel/sched_autogroup.h |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)

index 9fb6562..137a096 100644
@@ -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);
 }

index 7b859ff..0557705 100644
@@ -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

Commit-ID:  800d4d30c8f20bd728e5741a3b77c4859a613f7c
Gitweb:     http://git.kernel.org/tip/800d4d30c8f20bd728e5741a3b77c4859a613f7c
Author:     Yong Zhang <[email protected]>
AuthorDate: Sun, 20 Feb 2011 15:08:14 +0800
Committer:  Ingo Molnar <[email protected]>
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.

LKML-Reference: <[email protected]>

---
 kernel/sched_autogroup.c |    4 ++++
 kernel/sched_autogroup.h |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)

index 9fb6562..137a096 100644
@@ -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);
 }

index 7b859ff..0557705 100644
@@ -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;

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

---
 kernel/sched.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

index 18d38e4..5c74f79 100644
@@ -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

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 

---
 include/linux/sched.h    |    3 +++
 kernel/sched_autogroup.c |   34 ++++++++++++++++++++++++++++++++++
 kernel/sysctl.c          |    2 +-
 3 files changed, 38 insertions(+), 1 deletions(-)

index d747f94..00791ce 100644
@@ -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);

index 137a096..b1ad946 100644
@@ -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)

index 22e07ee..2b12c65 100644
@@ -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

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

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

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

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

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

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

-Mike

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

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

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

---
From: Yong Zhang <[email protected]>
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.

---
 kernel/sched_autogroup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 64919dc..6114e3e 100644
@@ -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

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

Hmmm, I think it's ok.

---
From: Yong Zhang <[email protected]>
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]:/".

---
 kernel/sched_autogroup.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

index 9fb6562..5b9a317 100644
@@ -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

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.

index 9fb6562..506faa0 100644
@@ -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);

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

Thanks,
Yong

Commit-ID:  511f67a5997c4967c69a3961e2fc9f04d8d244ac
Gitweb:     http://git.kernel.org/tip/511f67a5997c4967c69a3961e2fc9f04d8d244ac
Author:     Mike Galbraith <[email protected]>
AuthorDate: Tue, 22 Feb 2011 15:02:00 +0100
Committer:  Ingo Molnar <[email protected]>
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.

LKML-Reference: <[email protected]>

---
 kernel/sched_autogroup.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

index 137a096..5946ac5 100644
@@ -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);