Re: [PATCH] sched, autogroup: Fix failure when writing to cpu.rt_runtime_us

[Zefan Li <lizefan@huawei.com>]

>> See this commit:
>>
>> commit f44937718ce3b8360f72f6c68c9481712517a867
>> Author: Mike Galbraith <efault@gmx.de>
>> Date:   Thu Jan 13 04:54:50 2011 +0100
>>
>>     sched, autogroup: Fix CONFIG_RT_GROUP_SCHED sched_setscheduler() failure
>> ...
>> +       /*
>> +        * Autogroup RT tasks are redirected to the root task group
>> 	...
>> +        * the policy change to proceed.  Thereafter, task_group()
>> +        * returns &root_task_group, ...
>> +        */
> 
> That comment is misleading; if you look at the actual code what we do is
> redirect RT programs to _run_ in the root_task_group, but their
> task_group() should still be autogroup.
> 
> Otherwise people could escape their cgroup by switching to and from a RT
> class.
> 

Before 8323f26ce342 "sched: Fix race in task_group()", task_group() of
those RT tasks always return root_task_group, but the escape can't happen.

After commit 8323f26ce342, task_group always return root_task_group except
for the case I showed:

1. Change scheduling policy before setsid():

 # cat /proc/sched_debug | grep test
 R           test  4194     24851.893077       945   120     24851.893077     11196.482331         0.000000 /

2. Change policy after setsid():

 R           test  4142      4962.517723       420   120      4962.517723      4974.126149         0.000000 /autogroup-44

I think we can fix it with:

diff --git a/kernel/sched/auto_group.c b/kernel/sched/auto_group.c
index 8a2e230..8c3a169 100644
--- a/kernel/sched/auto_group.c
+++ b/kernel/sched/auto_group.c
@@ -115,9 +115,6 @@ bool task_wants_autogroup(struct task_struct *p, struct task_group *tg)
 	if (tg != &root_task_group)
 		return false;
 
-	if (p->sched_class != &fair_sched_class)
-		return false;
-
 	/*
 	 * We can only assume the task group can't go away on us if
 	 * autogroup_move_group() can see us on ->thread_group list.

> So what I think you want is something like the below; preferably with a
> comment on ;-)
> 

This is exactly what I did at first, but besides the issue described above,
seems it might lead to starving RT tasks.

If there's some rt task in autogroups but none in root cgroup, it's allowed
to set rt_runtime to 0, so I think we have to disallow this setting, like what
we already do with global rt_runtime.

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 89e7283..3f6c3ad 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7460,6 +7460,9 @@ static inline int tg_has_rt_tasks(struct task_group *tg)
 {
 	struct task_struct *g, *p;
 
+	if (task_group_is_autogroup(tg))
+		return 0;
+
 	for_each_process_thread(g, p) {
 		if (rt_task(p) && task_group(p) == tg)
 			return 1;
@@ -7540,6 +7543,9 @@ static int __rt_schedulable(struct task_group *tg, u64 period, u64 runtime)
 		.rt_runtime = runtime,
 	};
 
+	if (tg == &root_task_group && runtime == 0)
+		return -EINVAL;
+
 	rcu_read_lock();
 	ret = walk_tg_tree(tg_rt_schedulable, tg_nop, &data);
 	rcu_read_unlock();

> ---
>  kernel/sched/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 1f37fe7f77a4..f4fd048ce7cf 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7644,6 +7644,9 @@ static inline int tg_has_rt_tasks(struct task_group *tg)
>  {
>  	struct task_struct *g, *p;
>  
> +	if (task_group_is_autogroup(tg))
> +		return 0;
> +
>  	for_each_process_thread(g, p) {
>  		if (rt_task(p) && task_group(p) == tg)
>  			return 1;
> .
>