cgroup: Include dying leaders with live threads in PROCS iterations

Hello, Michal.

Quoted 10 lines · Michal Koutný

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.

Thanks.

--
tejun

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

Hello,

Quoted 6 lines · Suren Baghdasaryan

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

Quoted 6 lines

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.

Quoted 7 lines · Suren Baghdasaryan

Quoted 6 lines

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.

Quoted 6 lines

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

Quoted 12 lines · Michal Koutný

Quoted 10 lines

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.

Quoted 5 lines

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

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

So from user space's point of view the cgroup is empty and can be removed but rmdir() fails to do so.

Hello Suren.

Quoted 12 lines · Suren Baghdasaryan

Quoted 12 lines · Michal Koutný

Quoted 10 lines

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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 1 line

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?

Quoted 4 lines

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?

Quoted 4 lines

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?

Quoted 2 lines

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

H Michal,

Quoted 29 lines · Michal Koutný

Hello Suren.

Quoted 12 lines · Suren Baghdasaryan

Quoted 12 lines · Michal Koutný

Quoted 10 lines

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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 5 lines

Now to your issue.

Quoted 1 line

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 1 line

Quoted 4 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 3 lines

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

Quoted 2 lines

Michal

Thanks,
Suren.

Quoted 36 lines · Suren Baghdasaryan

H Michal,

Quoted 32 lines · Michal Koutný

Hello Suren.

Quoted 12 lines · Suren Baghdasaryan

Quoted 12 lines · Michal Koutný

Quoted 10 lines

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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 1 line

but I got a bug report from a customer about Android

Quoted 8 lines

Quoted 2 lines

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.

Quoted 24 lines

Quoted 7 lines

Quoted 5 lines

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.

Quoted 1 line

Quoted 13 lines

Quoted 4 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 2 lines

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().

Quoted 3 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

Michal

Thanks,
Suren.