Re: [PATCH 3/3 cgroup/for-5.2-fixes] cgroup: Include dying leaders with live threads in PROCS iterations

["Michal Koutný" <mkoutny-IBi9RG/b67k@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

Hello Suren.

On Fri, Jan 10, 2020 at 01:47:19PM -0800, Suren Baghdasaryan <surenb@google.com> wrote:
> > On Fri, Jun 07, 2019 at 07:09:53PM +0200, Michal Koutný wrote:
> > > Wouldn't it make more sense to call
> > >       css_set_move_task(tsk, cset, NULL, false);
> > > in cgroup_release instead of cgroup_exit then?
> > >
> > > css_set_move_task triggers the cgroup emptiness notification so if we
> > > list group leaders with running siblings as members of the cgroup (IMO
> > > correct), is it consistent to deliver the (un)populated event
> > > that early?
> > > By moving to cgroup_release we would also make this notification
> > > analogous to SIGCHLD delivery.
> >
> > So, the current behavior is mostly historical and I don't think this
> > is a better behavior.  That said, I'm not sure about the cost benefit
> > ratio of changing the behavior at this point given how long the code
> > has been this way.  Another aspect is that it'd expose zombie tasks
> > and possibly migrations of them to each controller.
> 
> Sorry for reviving an old discussion
Since you reply to my remark, I have to share that I found myself wrong
later wrt the emptiness notifications. Moving the task in cgroup_exit
doesn't matter if thread group contains other live tasks, the unpopulated
notification will be raised when the last task of thread group calls
group_exit, i.e. it is similar to SIGHLD.

Now to your issue.

> but I got a bug report from a customer about Android 
What kernel version is that? Can be it considered equal to the current
Linux?

> In my testing I was able to reproduce the failure in
> which case rmdir() fails with EBUSY from cgroup_destroy_locked()
> because cgroup_is_populated() returns true.
That'd mean that not all tasks in the cgroup did exit() (cgroup_exit()),
i.e. they're still running. Can you see them in cgroup.threads/tasks?

> cgroup_is_populated() depends on cgrp->nr_populated_xxx counters which
> IIUC will not be updated until cgroup_update_populated() is called
> from cgroup_exit() and that might get delayed...
Why do you think it's delayed?

> So from user space's point of view the cgroup is empty and can be
> removed but rmdir() fails to do so.
As Tejun says, cgroup with only dead _tasks_ should be removable (and if
I'm not mistaken it is in the current kernel). Unless you do individual
threads migration when a thread would be separated from its (dead)
leader. Does your case include such migrations?

Michal

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]