[PATCH] sched/core: Fix cpu controller for !RT_GROUP_SCHED

[PATCH] sched/core: Fix cpu controller for !RT_GROUP_SCHED

[Juri Lelli]

On !CONFIG_RT_GROUP_SCHED configurations it is currently not possible to
move RT tasks between cgroups to which cpu controller has been attached;
but it is oddly possible to first move tasks around and then make them
RT (setschedule to FIFO/RR).

E.g.:

  # mkdir /sys/fs/cgroup/cpu,cpuacct/group1
  # chrt -fp 10 $$
  # echo $$ > /sys/fs/cgroup/cpu,cpuacct/group1/tasks
  bash: echo: write error: Invalid argument
  # chrt -op 0 $$
  # echo $$ > /sys/fs/cgroup/cpu,cpuacct/group1/tasks
  # chrt -fp 10 $$
  # cat /sys/fs/cgroup/cpu,cpuacct/group1/tasks
  2345
  2598
  # chrt -p 2345
  pid 2345's current scheduling policy: SCHED_FIFO
  pid 2345's current scheduling priority: 10

Existing code comes with a comment saying the "we don't support RT-tasks
being in separate groups". Such comment is however stale and belongs to
pre-RT_GROUP_SCHED times. Also, it doesn't make much sense for
!RT_GROUP_ SCHED configurations, since checks related to RT bandwidth
are not performed at all in these cases.

Make moving RT tasks between cpu controller groups viable by removing
special case check for RT (and DEADLINE) tasks.

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
Hi,

Although I'm pretty assertive in the changelog, I actually wonder what
am I missing here and why (if) current behavior is needed and makes
sense.

Any input?

Thanks,

Juri
---
 kernel/sched/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 29984d8c41f0..37386b8bd1ad 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6464,10 +6464,6 @@ static int cpu_cgroup_can_attach(struct cgroup_taskset *tset)
 #ifdef CONFIG_RT_GROUP_SCHED
 		if (!sched_rt_can_attach(css_tg(css), task))
 			return -EINVAL;
-#else
-		/* We don't support RT-tasks being in separate groups */
-		if (task->sched_class != &fair_sched_class)
-			return -EINVAL;
 #endif
 		/*
 		 * Serialize against wake_up_new_task() such that if its
-- 
2.17.2

Re: [PATCH] sched/core: Fix cpu controller for !RT_GROUP_SCHED

[Tejun Heo]

Hello,

On Wed, Jun 05, 2019 at 01:49:35PM +0200, Juri Lelli wrote:
> On !CONFIG_RT_GROUP_SCHED configurations it is currently not possible to
> move RT tasks between cgroups to which cpu controller has been attached;
> but it is oddly possible to first move tasks around and then make them
> RT (setschedule to FIFO/RR).
> 
> E.g.:
> 
>   # mkdir /sys/fs/cgroup/cpu,cpuacct/group1
>   # chrt -fp 10 $$
>   # echo $$ > /sys/fs/cgroup/cpu,cpuacct/group1/tasks
>   bash: echo: write error: Invalid argument
>   # chrt -op 0 $$
>   # echo $$ > /sys/fs/cgroup/cpu,cpuacct/group1/tasks
>   # chrt -fp 10 $$
>   # cat /sys/fs/cgroup/cpu,cpuacct/group1/tasks
>   2345
>   2598
>   # chrt -p 2345
>   pid 2345's current scheduling policy: SCHED_FIFO
>   pid 2345's current scheduling priority: 10
> 
> Existing code comes with a comment saying the "we don't support RT-tasks
> being in separate groups". Such comment is however stale and belongs to
> pre-RT_GROUP_SCHED times. Also, it doesn't make much sense for
> !RT_GROUP_ SCHED configurations, since checks related to RT bandwidth
> are not performed at all in these cases.
> 
> Make moving RT tasks between cpu controller groups viable by removing
> special case check for RT (and DEADLINE) tasks.
> 
> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
> ---
> Hi,
> 
> Although I'm pretty assertive in the changelog, I actually wonder what
> am I missing here and why (if) current behavior is needed and makes
> sense.
> 
> Any input?

Yeah, RT tasks being transprent to the cpu controller when
!RT_GROUP_SCHED makes sense to me, especially given that the rules
around it are already inconsistent.  Please feel free to add

  Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH] sched/core: Fix cpu controller for !RT_GROUP_SCHED

[Michal Koutný]

On Wed, Jun 05, 2019 at 01:49:35PM +0200, Juri Lelli <juri.lelli@redhat.com> wrote:
> Existing code comes with a comment saying the "we don't support RT-tasks
> being in separate groups".
I'm also inclined to this check not being completely correct.

This guard also prevents enabling cpu controller on unified hierarchy
with !CONFIG_RT_GROUP_SCHED. (If there are any kernel RT threads in root
cgroup, they can't be migrated to the newly create cpu controller's root
in cgroup_update_dfl_csses().)

I considered relaxing the check to non-root cgroups only, however, as
your example shows, it doesn't prevent reaching the avoided state by
other paths. I'm not that familiar with RT sched to tell whether
RT-priority tasks in different task_groups break any assumptions.

Michal

Re: [PATCH] sched/core: Fix cpu controller for !RT_GROUP_SCHED

[Michal Koutný]

On Wed, Jun 05, 2019 at 04:20:03PM +0200, Michal Koutný <mkoutny@suse.com> wrote:
> I considered relaxing the check to non-root cgroups only, however, as
> your example shows, it doesn't prevent reaching the avoided state by
> other paths. I'm not that familiar with RT sched to tell whether
> RT-priority tasks in different task_groups break any assumptions.
So I had another look and the check is bogus.

The RT sched with !CONFIG_RT_GROUP_SCHED works only with the struct
rt_rq embedded in the generic struct rq -- regardless of the task's
membership in the cpu controller hierarchy.

Perhaps, the commit message may mention this also prevents enabling cpu
controller on unified hierarchy (if there are any (kernel) RT tasks to
migrate).

Reviewed-by: Michal Koutný <mkoutny@suse.com>

Re: [PATCH] sched/core: Fix cpu controller for !RT_GROUP_SCHED

[Juri Lelli]

Hi,

On 19/06/19 11:29, Michal Koutný wrote:
> On Wed, Jun 05, 2019 at 04:20:03PM +0200, Michal Koutný <mkoutny@suse.com> wrote:
> > I considered relaxing the check to non-root cgroups only, however, as
> > your example shows, it doesn't prevent reaching the avoided state by
> > other paths. I'm not that familiar with RT sched to tell whether
> > RT-priority tasks in different task_groups break any assumptions.
> So I had another look and the check is bogus.
> 
> The RT sched with !CONFIG_RT_GROUP_SCHED works only with the struct
> rt_rq embedded in the generic struct rq -- regardless of the task's
> membership in the cpu controller hierarchy.

Yep.

> Perhaps, the commit message may mention this also prevents enabling cpu
> controller on unified hierarchy (if there are any (kernel) RT tasks to
> migrate).

Sure. Can add such a comment.

> Reviewed-by: Michal Koutný <mkoutny@suse.com>

Thanks!

Peter?

Best,

Juri