[PATCH] freezer, sched: report the frozen task stat as 'D'

[PATCH] freezer, sched: report the frozen task stat as 'D'

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

Before the commit f5d39b020809 ("freezer,sched: Rewrite core freezer
logic"), the frozen task stat was reported as 'D' in cgroup v1.
However, after rewriting core freezer logic, the frozen task stat is
reported as 'R'. This is confusing, especially when a task with stat of
'S' is frozen.

This can be reproduced as bellow step:
cd /sys/fs/cgroup/freezer/
mkdir test
sleep 1000 &
[1] 739         // task whose stat is 'S'
echo 739 > test/cgroup.procs
echo FROZEN > test/freezer.state
ps -aux | grep 739
root     739  0.1  0.0   8376  1812 pts/0    R    10:56   0:00 sleep 1000

As shown above, a task whose stat is 'S' was changed to 'R' when it was
frozen. To solve this issue, simply maintain the same reported state as
before the rewrite.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
 include/linux/sched.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index f0f23efdb245..878acce2f8d6 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1630,8 +1630,9 @@ static inline unsigned int __task_state_index(unsigned int tsk_state,
 	 * We're lying here, but rather than expose a completely new task state
 	 * to userspace, we can make this appear as if the task has gone through
 	 * a regular rt_mutex_lock() call.
+	 * Report the frozen task uninterruptible.
 	 */
-	if (tsk_state & TASK_RTLOCK_WAIT)
+	if (tsk_state & TASK_RTLOCK_WAIT || tsk_state & TASK_FROZEN)
 		state = TASK_UNINTERRUPTIBLE;
 
 	return fls(state);
-- 
2.34.1

Re: [PATCH] freezer, sched: report the frozen task stat as 'D'

[Valentin Schneider]

On 11/11/24 13:54, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
>
> Before the commit f5d39b020809 ("freezer,sched: Rewrite core freezer
> logic"), the frozen task stat was reported as 'D' in cgroup v1.
> However, after rewriting core freezer logic, the frozen task stat is
> reported as 'R'. This is confusing, especially when a task with stat of
> 'S' is frozen.
>
> This can be reproduced as bellow step:
> cd /sys/fs/cgroup/freezer/
> mkdir test
> sleep 1000 &
> [1] 739         // task whose stat is 'S'
> echo 739 > test/cgroup.procs
> echo FROZEN > test/freezer.state
> ps -aux | grep 739
> root     739  0.1  0.0   8376  1812 pts/0    R    10:56   0:00 sleep 1000
>
> As shown above, a task whose stat is 'S' was changed to 'R' when it was
> frozen. To solve this issue, simply maintain the same reported state as
> before the rewrite.
>
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> ---
>  include/linux/sched.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index f0f23efdb245..878acce2f8d6 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1630,8 +1630,9 @@ static inline unsigned int __task_state_index(unsigned int tsk_state,
>  	 * We're lying here, but rather than expose a completely new task state
>  	 * to userspace, we can make this appear as if the task has gone through
>  	 * a regular rt_mutex_lock() call.
> +	 * Report the frozen task uninterruptible.
>  	 */
> -	if (tsk_state & TASK_RTLOCK_WAIT)
> +	if (tsk_state & TASK_RTLOCK_WAIT || tsk_state & TASK_FROZEN)

Hmph, for RTLOCK this was good enough since !PREEMPT_RT really would be
TASK_UNINTERRUPTIBLE. 

For the freezer AFAICT there's more variation, following your example:

  crash> task -R __state,saved_state 195
  PID: 195      TASK: ffff888004abab80  CPU: 1    COMMAND: "sh"
    __state = 32768    -> 0x8000 aka TASK_FROZEN
    saved_state = 8193 -> 0x2001 aka TASK_INTERRUPTIBLE | TASK_FREEZABLE

so we ought to read ->saved_state, but as pointed out in [1] this is
racy...

[1]: https://lore.kernel.org/lkml/20220120162520.570782-3-valentin.schneider@arm.com/

>  		state = TASK_UNINTERRUPTIBLE;
>  
>  	return fls(state);
> -- 
> 2.34.1

Re: [PATCH] freezer, sched: report the frozen task stat as 'D'

[Chen Ridong]

On 2024/11/13 22:34, Valentin Schneider wrote:
> On 11/11/24 13:54, Chen Ridong wrote:
>> From: Chen Ridong <chenridong@huawei.com>
>>
>> Before the commit f5d39b020809 ("freezer,sched: Rewrite core freezer
>> logic"), the frozen task stat was reported as 'D' in cgroup v1.
>> However, after rewriting core freezer logic, the frozen task stat is
>> reported as 'R'. This is confusing, especially when a task with stat of
>> 'S' is frozen.
>>
>> This can be reproduced as bellow step:
>> cd /sys/fs/cgroup/freezer/
>> mkdir test
>> sleep 1000 &
>> [1] 739         // task whose stat is 'S'
>> echo 739 > test/cgroup.procs
>> echo FROZEN > test/freezer.state
>> ps -aux | grep 739
>> root     739  0.1  0.0   8376  1812 pts/0    R    10:56   0:00 sleep 1000
>>
>> As shown above, a task whose stat is 'S' was changed to 'R' when it was
>> frozen. To solve this issue, simply maintain the same reported state as
>> before the rewrite.
>>
>> Signed-off-by: Chen Ridong <chenridong@huawei.com>
>> ---
>>  include/linux/sched.h | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index f0f23efdb245..878acce2f8d6 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1630,8 +1630,9 @@ static inline unsigned int __task_state_index(unsigned int tsk_state,
>>  	 * We're lying here, but rather than expose a completely new task state
>>  	 * to userspace, we can make this appear as if the task has gone through
>>  	 * a regular rt_mutex_lock() call.
>> +	 * Report the frozen task uninterruptible.
>>  	 */
>> -	if (tsk_state & TASK_RTLOCK_WAIT)
>> +	if (tsk_state & TASK_RTLOCK_WAIT || tsk_state & TASK_FROZEN)
> 
> Hmph, for RTLOCK this was good enough since !PREEMPT_RT really would be
> TASK_UNINTERRUPTIBLE. 
> 
> For the freezer AFAICT there's more variation, following your example:
> 

Thank you for your reply.
I tried to freeze a task whose stat is 'R', the saved_state is variable.

crash> task -R __state,saved_state 821
PID: 821    TASK: ffff888101df8000  CPU: 6   COMMAND: "test.sh"
  __state = 32768,
  saved_state = 0,

However, if a task is frozen, it will be better to show same stat in
case of confusion. In cgroup v2, it is shown as 'S'. In cgroup v1, the
prior stat shown was 'D'. Therefore, it might be a good choice to
maintain the same reported state as before the rewrite.

Or does anyone have a better idea?

Best regards,
Ridong

>   crash> task -R __state,saved_state 195
>   PID: 195      TASK: ffff888004abab80  CPU: 1    COMMAND: "sh"
>     __state = 32768    -> 0x8000 aka TASK_FROZEN
>     saved_state = 8193 -> 0x2001 aka TASK_INTERRUPTIBLE | TASK_FREEZABLE
> 
> so we ought to read ->saved_state, but as pointed out in [1] this is
> racy...
> 
> [1]: https://lore.kernel.org/lkml/20220120162520.570782-3-valentin.schneider@arm.com/
> 
>>  		state = TASK_UNINTERRUPTIBLE;
>>  
>>  	return fls(state);
>> -- 
>> 2.34.1
> 
> 

Re: [PATCH] freezer, sched: report the frozen task stat as 'D'

[Tejun Heo]

Hello,

On Thu, Nov 14, 2024 at 03:47:58PM +0800, Chen Ridong wrote:
> On 2024/11/13 22:34, Valentin Schneider wrote:
> > On 11/11/24 13:54, Chen Ridong wrote:
> >> From: Chen Ridong <chenridong@huawei.com>
> >>
> >> Before the commit f5d39b020809 ("freezer,sched: Rewrite core freezer
> >> logic"), the frozen task stat was reported as 'D' in cgroup v1.
> >> However, after rewriting core freezer logic, the frozen task stat is
> >> reported as 'R'. This is confusing, especially when a task with stat of
> >> 'S' is frozen.
> >>
> >> This can be reproduced as bellow step:
> >> cd /sys/fs/cgroup/freezer/
> >> mkdir test
> >> sleep 1000 &
> >> [1] 739         // task whose stat is 'S'
> >> echo 739 > test/cgroup.procs
> >> echo FROZEN > test/freezer.state
> >> ps -aux | grep 739
> >> root     739  0.1  0.0   8376  1812 pts/0    R    10:56   0:00 sleep 1000
> >>
> >> As shown above, a task whose stat is 'S' was changed to 'R' when it was
> >> frozen. To solve this issue, simply maintain the same reported state as
> >> before the rewrite.

The whole point of the freezer rewrite was preventing userspace from causing
arbitrary and indefinite D states which leads to all sorts of problems. The
tasks in cgroup2 freezer are *not* in uninterruptible sleeps. They are in
task stop loops and thus can be killed, ptraced and so on. It *should*
report S.

Thanks.

-- 
tejun

Re: [PATCH] freezer, sched: report the frozen task stat as 'D'

[Chen Ridong]

On 2024/11/15 0:44, Tejun Heo wrote:
> Hello,
> 
> On Thu, Nov 14, 2024 at 03:47:58PM +0800, Chen Ridong wrote:
>> On 2024/11/13 22:34, Valentin Schneider wrote:
>>> On 11/11/24 13:54, Chen Ridong wrote:
>>>> From: Chen Ridong <chenridong@huawei.com>
>>>>
>>>> Before the commit f5d39b020809 ("freezer,sched: Rewrite core freezer
>>>> logic"), the frozen task stat was reported as 'D' in cgroup v1.
>>>> However, after rewriting core freezer logic, the frozen task stat is
>>>> reported as 'R'. This is confusing, especially when a task with stat of
>>>> 'S' is frozen.
>>>>
>>>> This can be reproduced as bellow step:
>>>> cd /sys/fs/cgroup/freezer/
>>>> mkdir test
>>>> sleep 1000 &
>>>> [1] 739         // task whose stat is 'S'
>>>> echo 739 > test/cgroup.procs
>>>> echo FROZEN > test/freezer.state
>>>> ps -aux | grep 739
>>>> root     739  0.1  0.0   8376  1812 pts/0    R    10:56   0:00 sleep 1000
>>>>
>>>> As shown above, a task whose stat is 'S' was changed to 'R' when it was
>>>> frozen. To solve this issue, simply maintain the same reported state as
>>>> before the rewrite.
> 
> The whole point of the freezer rewrite was preventing userspace from causing
> arbitrary and indefinite D states which leads to all sorts of problems. The
> tasks in cgroup2 freezer are *not* in uninterruptible sleeps. They are in
> task stop loops and thus can be killed, ptraced and so on. It *should*
> report S.
> 
> Thanks.
> 

Hi, Tj, There is no ambiguity what cgroup v2 shows. However, in cgroup
v1, the frozen tasks are reported as 'R'.  Do you think this is reseanable?

Thanks,
Ridong

Re: [PATCH] freezer, sched: report the frozen task stat as 'D'

[Tejun Heo]

Hello,

On Fri, Nov 15, 2024 at 09:14:32AM +0800, Chen Ridong wrote:
> Hi, Tj, There is no ambiguity what cgroup v2 shows. However, in cgroup
> v1, the frozen tasks are reported as 'R'.  Do you think this is reseanable?

Oh, right, pardon my confusion. I thought this was about cgroup2 freezer.
For cgroup1 freezer, yes, D is the closest state for reporting to userspace.

Thanks.

-- 
tejun

Re: [PATCH] freezer, sched: report the frozen task stat as 'D'

[Chen Ridong]

On 2024/11/15 9:28, Tejun Heo wrote:
> Hello,
> 
> On Fri, Nov 15, 2024 at 09:14:32AM +0800, Chen Ridong wrote:
>> Hi, Tj, There is no ambiguity what cgroup v2 shows. However, in cgroup
>> v1, the frozen tasks are reported as 'R'.  Do you think this is reseanable?
> 
> Oh, right, pardon my confusion. I thought this was about cgroup2 freezer.
> For cgroup1 freezer, yes, D is the closest state for reporting to userspace.
> 
> Thanks.
> 

Hello, Peterz, do you have any opinions?

Best regards,
Ridong

Re: [PATCH] freezer, sched: report the frozen task stat as 'D'

[Chen Ridong]

On 2024/11/25 10:14, Chen Ridong wrote:
> 
> 
> On 2024/11/15 9:28, Tejun Heo wrote:
>> Hello,
>>
>> On Fri, Nov 15, 2024 at 09:14:32AM +0800, Chen Ridong wrote:
>>> Hi, Tj, There is no ambiguity what cgroup v2 shows. However, in cgroup
>>> v1, the frozen tasks are reported as 'R'.  Do you think this is reseanable?
>>
>> Oh, right, pardon my confusion. I thought this was about cgroup2 freezer.
>> For cgroup1 freezer, yes, D is the closest state for reporting to userspace.
>>
>> Thanks.
>>
> 
> Hello, Peterz, do you have any opinions?
> 
> Best regards,
> Ridong
> 

Friendly ping.

Does anyone have any opinions?

Re: [PATCH] freezer, sched: report the frozen task stat as 'D'

[Michal Koutný]

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

Hi.

On Wed, Dec 04, 2024 at 09:52:15AM GMT, Chen Ridong <chenridong@huaweicloud.com> wrote:
> Does anyone have any opinions?

When an 'R' task is frozen (v1) and it's shown as (unchanged) 'R' it's
acceptable.
When an 'S' task is equally frozen and its apparent state switches to
'R', I agree it's confusing.

Showing 'D' state in any case of a frozen task (like before that rework)
makes sense too. Please add Fixes: if you need to restore this behavior.

HTH,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] freezer, sched: report the frozen task stat as 'D'

[Chen Ridong]

On 2024/12/12 0:02, Michal Koutný wrote:
> Hi.
> 
> On Wed, Dec 04, 2024 at 09:52:15AM GMT, Chen Ridong <chenridong@huaweicloud.com> wrote:
>> Does anyone have any opinions?
> 
> When an 'R' task is frozen (v1) and it's shown as (unchanged) 'R' it's
> acceptable.
> When an 'S' task is equally frozen and its apparent state switches to
> 'R', I agree it's confusing.
> 
> Showing 'D' state in any case of a frozen task (like before that rework)
> makes sense too. Please add Fixes: if you need to restore this behavior.
> 
> HTH,
> Michal

Thank you, Michal. I will update.

Thanks,
Ridong