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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4f78d752-52ab-493d-8bf5-f12dc4f554c8@huaweicloud.com>
Date: Thu, 14 Nov 2024 15:47:58 +0800
From: Chen Ridong <chenridong@...weicloud.com>
To: Valentin Schneider <vschneid@...hat.com>,
 Chen Ridong <chenridong@...weicloud.com>, mingo@...hat.com,
 peterz@...radead.org, juri.lelli@...hat.com, vincent.guittot@...aro.org,
 dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
 mgorman@...e.de, Tejun Heo <tj@...nel.org>,
 mkoutny@...e.com >> Michal Koutný <mkoutny@...e.com>
Cc: linux-kernel@...r.kernel.org, wangweiyang2@...wei.com,
 cgroups@...r.kernel.org
Subject: Re: [PATCH] freezer, sched: report the frozen task stat as 'D'



On 2024/11/13 22:34, Valentin Schneider wrote:
> On 11/11/24 13:54, Chen Ridong wrote:
>> From: Chen Ridong <chenridong@...wei.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@...wei.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
> 
>