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

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

[Suren Baghdasaryan]

Hi Tejun,

On Tue, Jun 11, 2019 at 12:10 PM Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> Hello, Michal.
>
> 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 but I got a bug report from a
customer about Android occasionally failing to remove a cgroup after
it killed all processes in it. AndroidManagerService (a privileged
user space process) reads cgroup.procs and sends SIGKILL to all listed
processes in that cgroup until cgroup.procs is empty at which point it
tries to delete cgroup directory (it is a leaf cgroup with empty
cgroup.procs). 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. 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... Meanwhile the tasks counted by
cgrp->nr_populated_xxx will not show up when cgroup.procs or
cgroup.tasks/tasks files are read by user space due to these changes:

+               /* and dying leaders w/o live member threads */
+               if (!atomic_read(&task->signal->live))
+                       goto repeat;
+       } else {
+               /* skip all dying ones */
+               if (task->flags & PF_EXITING)
+                       goto repeat;
+       }

After removing these checks cgroup.procs shows the processes up until
they are dead and the issue is gone.

So from user space's point of view the cgroup is empty and can be
removed but rmdir() fails to do so. I think this behavior is
inconsistent with the claim "cgroup which doesn't have any children
and is associated only with zombie processes is considered empty and
can be removed" from
https://elixir.bootlin.com/linux/v5.4.10/source/Documentation/admin-guide/cgroup-v2.rst#L222
.

Before contemplating what would be the right fix here I wanted to
check if cgroup.procs being empty is a reasonable indication for user
space to assume the cgroup is empty and can be removed? I think it is
but I might be wrong.
Note that this behavior was reproduced on cgroup v1 hierarchy but I
think it will happen on v2 as well. If needed I can cook up a
reproducer.
Thanks,
Suren.

>
> Thanks.

>
> --
> tejun

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

[Tejun Heo]

Hello,

On Fri, Jan 10, 2020 at 01:47:19PM -0800, Suren Baghdasaryan wrote:
> So from user space's point of view the cgroup is empty and can be
> removed but rmdir() fails to do so. I think this behavior is
> inconsistent with the claim "cgroup which doesn't have any children
> and is associated only with zombie processes is considered empty and
> can be removed" from
> https://elixir.bootlin.com/linux/v5.4.10/source/Documentation/admin-guide/cgroup-v2.rst#L222

Yeah, the current behavior isn't quite consistent with the
documentation and what we prolly wanna do is allowing destroying a
cgroup with only dead processes in it.  That said, the correct (or at
least workable) signal which indicates that a cgroup is ready for
removal is cgroup.events::populated being zero, which is a poll(2)able
event.

Thanks.

-- 
tejun

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

[Suren Baghdasaryan]

On Fri, Jan 10, 2020 at 1:56 PM Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> Hello,
>
> On Fri, Jan 10, 2020 at 01:47:19PM -0800, Suren Baghdasaryan wrote:
> > So from user space's point of view the cgroup is empty and can be
> > removed but rmdir() fails to do so. I think this behavior is
> > inconsistent with the claim "cgroup which doesn't have any children
> > and is associated only with zombie processes is considered empty and
> > can be removed" from
> > https://elixir.bootlin.com/linux/v5.4.10/source/Documentation/admin-guide/cgroup-v2.rst#L222
>
> Yeah, the current behavior isn't quite consistent with the
> documentation and what we prolly wanna do is allowing destroying a
> cgroup with only dead processes in it.  That said, the correct (or at
> least workable) signal which indicates that a cgroup is ready for
> removal is cgroup.events::populated being zero, which is a poll(2)able
> event.

Unfortunately it would not be workable for us as it's only available
for cgroup v2 controllers.
I can think of other ways to fix it in the userspace but there might
be other cgroup API users which are be broken after this change. I
would prefer to fix it in the kernel if at all possible rather than
chasing all possible users.
Thanks,
Suren.

>
> Thanks.
>
> --
> tejun

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

[Tejun Heo]

On Fri, Jan 10, 2020 at 02:14:19PM -0800, Suren Baghdasaryan wrote:
> > Yeah, the current behavior isn't quite consistent with the
> > documentation and what we prolly wanna do is allowing destroying a
> > cgroup with only dead processes in it.  That said, the correct (or at
> > least workable) signal which indicates that a cgroup is ready for
> > removal is cgroup.events::populated being zero, which is a poll(2)able
> > event.
> 
> Unfortunately it would not be workable for us as it's only available
> for cgroup v2 controllers.
> I can think of other ways to fix it in the userspace but there might
> be other cgroup API users which are be broken after this change. I
> would prefer to fix it in the kernel if at all possible rather than
> chasing all possible users.

Yeah, the right thing to do is allowing destruction of cgroups w/ only
dead processes in it.

-- 
tejun

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

[Suren Baghdasaryan]

On Fri, Jan 10, 2020 at 2:15 PM Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> On Fri, Jan 10, 2020 at 02:14:19PM -0800, Suren Baghdasaryan wrote:
> > > Yeah, the current behavior isn't quite consistent with the
> > > documentation and what we prolly wanna do is allowing destroying a
> > > cgroup with only dead processes in it.  That said, the correct (or at
> > > least workable) signal which indicates that a cgroup is ready for
> > > removal is cgroup.events::populated being zero, which is a poll(2)able
> > > event.
> >
> > Unfortunately it would not be workable for us as it's only available
> > for cgroup v2 controllers.
> > I can think of other ways to fix it in the userspace but there might
> > be other cgroup API users which are be broken after this change. I
> > would prefer to fix it in the kernel if at all possible rather than
> > chasing all possible users.
>
> Yeah, the right thing to do is allowing destruction of cgroups w/ only
> dead processes in it.

Thanks for confirmation. I'll see if I can figure this out.

>
> --
> tejun

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

[Michal Koutný]

[-- 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 --]

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

[Suren Baghdasaryan]

 H Michal,

On Fri, Jan 10, 2020 at 3:16 PM Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org> wrote:
>
> 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?

I reproduced this with 4.19 kernel that had Tejun's patches backported
but I believe the same behavior will happen with any kernel that
contains c03cd7738a83 ("cgroup: Include dying leaders with live
threads in PROCS iterations") including the latest. I'll try to
confirm this on 5.4 next week.

>
> > 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?

Correct, I confirmed that there are still tasks in cset->tasks list
when this happens. Actually when I reproduced this I logged one task
in cset->dying_tasks which was the group leader and another one in
cset->tasks. So the last task to reach cgroup_exit() was not the group
leader.
However these tasks will be ignored when we enumerate them using
css_task_iter_start()/css_task_iter_next() loop because
css_task_iter_advance() ignores tasks with PF_EXITING flag set or
group leaders with task->signal->live==0 (see the code lines I posted
in my first email). So threads are there, they are PF_EXITING but at
least one of them did not reach cgroup_exit() yet. When user space
reads cgroup.procs/tasks they are skipped. This is the change in
behavior which is different from what it used to be before this patch.
Prior to it these tasks would be visible even if they are exiting.

>
> > 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?

I think exit_mm() can be one of the contributors if the process owns a
considerable amount of memory but there might be other possible
reasons.

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

Thanks,
Suren.

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

[Suren Baghdasaryan]

On Fri, Jan 10, 2020 at 4:00 PM Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>
>  H Michal,
>
> On Fri, Jan 10, 2020 at 3:16 PM Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org> wrote:
> >
> > 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?
>
> I reproduced this with 4.19 kernel that had Tejun's patches backported
> but I believe the same behavior will happen with any kernel that
> contains c03cd7738a83 ("cgroup: Include dying leaders with live
> threads in PROCS iterations") including the latest. I'll try to
> confirm this on 5.4 next week.
>
> >
> > > 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?
>
> Correct, I confirmed that there are still tasks in cset->tasks list
> when this happens. Actually when I reproduced this I logged one task
> in cset->dying_tasks which was the group leader and another one in
> cset->tasks. So the last task to reach cgroup_exit() was not the group
> leader.
> However these tasks will be ignored when we enumerate them using
> css_task_iter_start()/css_task_iter_next() loop because
> css_task_iter_advance() ignores tasks with PF_EXITING flag set or
> group leaders with task->signal->live==0 (see the code lines I posted
> in my first email). So threads are there, they are PF_EXITING but at
> least one of them did not reach cgroup_exit() yet. When user space
> reads cgroup.procs/tasks they are skipped. This is the change in
> behavior which is different from what it used to be before this patch.
> Prior to it these tasks would be visible even if they are exiting.
>
> >
> > > 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?
>
> I think exit_mm() can be one of the contributors if the process owns a
> considerable amount of memory but there might be other possible
> reasons.
>
> > > 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).

Sorry, I missed this last part.
Agree, if all tasks are dead the cgroup should be removable and it
will be if all tasks reached cgroup_exit(). The issue happens when
tasks are marked PF_EXITING, userspace reads empty cgroups.procs and
issues rmdir() before the last killed task reaches cgroup_exit().

> > Unless you do individual
> > threads migration when a thread would be separated from its (dead)
> > leader. Does your case include such migrations?

No my case just kills all tasks in the cgroup until cgroup.procs is
empty and then tries to remove the cgroup itself. No migrations.

> >
> > Michal
>
> Thanks,
> Suren.

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

[Suren Baghdasaryan]

On Fri, Jan 10, 2020 at 4:55 PM Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>
> On Fri, Jan 10, 2020 at 4:00 PM Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> >
> >  H Michal,
> >
> > On Fri, Jan 10, 2020 at 3:16 PM Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org> wrote:
> > >
> > > 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?
> >
> > I reproduced this with 4.19 kernel that had Tejun's patches backported
> > but I believe the same behavior will happen with any kernel that
> > contains c03cd7738a83 ("cgroup: Include dying leaders with live
> > threads in PROCS iterations") including the latest. I'll try to
> > confirm this on 5.4 next week.

Hi Folks,
I confirmed that the issue is happening on 5.4 as well. I did
implement a fix for this and a kselftest to reproduce the problem.
Will send both the fix and the kselftest as a 2-patch series shortly.
Thanks,
Suren.

> >
> > >
> > > > 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?
> >
> > Correct, I confirmed that there are still tasks in cset->tasks list
> > when this happens. Actually when I reproduced this I logged one task
> > in cset->dying_tasks which was the group leader and another one in
> > cset->tasks. So the last task to reach cgroup_exit() was not the group
> > leader.
> > However these tasks will be ignored when we enumerate them using
> > css_task_iter_start()/css_task_iter_next() loop because
> > css_task_iter_advance() ignores tasks with PF_EXITING flag set or
> > group leaders with task->signal->live==0 (see the code lines I posted
> > in my first email). So threads are there, they are PF_EXITING but at
> > least one of them did not reach cgroup_exit() yet. When user space
> > reads cgroup.procs/tasks they are skipped. This is the change in
> > behavior which is different from what it used to be before this patch.
> > Prior to it these tasks would be visible even if they are exiting.
> >
> > >
> > > > 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?
> >
> > I think exit_mm() can be one of the contributors if the process owns a
> > considerable amount of memory but there might be other possible
> > reasons.
> >
> > > > 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).
>
> Sorry, I missed this last part.
> Agree, if all tasks are dead the cgroup should be removable and it
> will be if all tasks reached cgroup_exit(). The issue happens when
> tasks are marked PF_EXITING, userspace reads empty cgroups.procs and
> issues rmdir() before the last killed task reaches cgroup_exit().
>
> > > Unless you do individual
> > > threads migration when a thread would be separated from its (dead)
> > > leader. Does your case include such migrations?
>
> No my case just kills all tasks in the cgroup until cgroup.procs is
> empty and then tries to remove the cgroup itself. No migrations.
>
> > >
> > > Michal
> >
> > Thanks,
> > Suren.