Hi Quentin,
On Tue, Apr 06, 2021 at 12:10:41PM +0000, Quentin Perret wrote:
> Hi Pavan,
>
> On Tuesday 06 Apr 2021 at 16:29:13 (+0530), Pavankumar Kondeti wrote:
> > In Android GKI, CONFIG_FAIR_GROUP_SCHED is enabled [1] to help prioritize
> > important work. Given that CPU shares of root cgroup can't be changed,
> > leaving the tasks inside root cgroup will give them higher share
> > compared to the other tasks inside important cgroups. This is mitigated
> > by moving all tasks inside root cgroup to a different cgroup after
> > Android is booted. However, there are many kernel tasks stuck in the
> > root cgroup after the boot.
> >
> > We see all kworker threads are in the root cpu cgroup. This is because,
> > tasks with PF_NO_SETAFFINITY flag set are forbidden from cgroup migration.
> > This restriction is in place to avoid kworkers getting moved to a cpuset
> > which conflicts with kworker affinity. Relax this restriction by explicitly
> > checking if the task is moving out of a cpuset cgroup. This allows kworkers
> > to be moved out root cpu cgroup.
> >
> > We also see kthreadd_task and any kernel thread created after the Android
> > boot
> > also stuck in the root cgroup. The current code prevents kthreadd_task
> > moving
> > out root cgroup to avoid the possibility of creating new RT kernel threads
> > inside a cgroup with no RT runtime allocated. Apply this restriction when
> > tasks
> > are moving out of cpu cgroup under CONFIG_RT_GROUP_SCHED. This allows all
> > kernel threads to be moved out of root cpu cgroup if the kernel does not
> > enable RT group scheduling.
>
> OK, so IIUC this only works with cgroup v1 -- the unified hierarchy in
> v2 forces you to keep cpu and cpuset in 'sync'. But that should be fine,
> so this looks like a nice improvement to me.
>
Yes. I will mention this in commit description.
> >
> > struct task_struct *cgroup_procs_write_start(char *buf, bool threadgroup,
> > - bool *locked)
> > + bool *locked,
> > + struct cgroup *dst_cgrp)
> > __acquires(&cgroup_threadgroup_rwsem)
> > {
> > struct task_struct *tsk;
> > @@ -2784,15 +2785,28 @@ struct task_struct *cgroup_procs_write_start(char
> > *buf, bool threadgroup,
> > tsk = tsk->group_leader;
> >
> > /*
> > + * RT kthreads may be born in a cgroup with no rt_runtime allocated.
> > + * Just say no.
> > + */
> > +#ifdef CONFIG_RT_GROUP_SCHED
> > + if (tsk->no_cgroup_migration && (dst_cgrp->root->subsys_mask & (1U <<
> > cpu_cgrp_id))) {
> > + tsk = ERR_PTR(-EINVAL);
> > + goto out_unlock_threadgroup;
> > + }
> > +#endif
> > +
> > + /*
> > * kthreads may acquire PF_NO_SETAFFINITY during initialization.
> > * If userland migrates such a kthread to a non-root cgroup, it can
> > - * become trapped in a cpuset, or RT kthread may be born in a
> > - * cgroup with no rt_runtime allocated. Just say no.
> > + * become trapped in a cpuset. Just say no.
> > */
> > - if (tsk->no_cgroup_migration || (tsk->flags & PF_NO_SETAFFINITY)) {
> > +#ifdef CONFIG_CPUSETS
> > + if ((tsk->no_cgroup_migration || (tsk->flags & PF_NO_SETAFFINITY)) &&
> > + (dst_cgrp->root->subsys_mask & (1U << cpuset_cgrp_id)))
> > {
> > tsk = ERR_PTR(-EINVAL);
> > goto out_unlock_threadgroup;
> > }
> > +#endif
>
> Nit: maybe move this #ifdefery out to a header?
>
Agreed.
Thanks,
Pavan
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project.
