Re: [External] Re: [PATCH v2 2/2] memcg: Don't trigger hung task warnings when memcg is releasing resources.

[Julian Sun <sunjunchao@bytedance.com>]

On 9/24/25 2:32 PM, Peter Zijlstra wrote:
> On Wed, Sep 24, 2025 at 11:41:00AM +0800, Julian Sun wrote:
>> Hung task warning in mem_cgroup_css_free() is undesirable and
>> unnecessary since the behavior of waiting for a long time is
>> expected.
>>
>> Use touch_hung_task_detector() to eliminate the possible
>> hung task warning.
>>
>> Signed-off-by: Julian Sun <sunjunchao@bytedance.com>
> 
> Still hate this. It is not tied to progress. If progress really stalls,
> no warning will be given.

Hi, peter

Thanks for your review and comments.

I did take a look at your solution provided yesterday, and get your 
point. However AFAICS it can't resolve the unexpected warnings here. 
Because it only works after we reach the finish_writeback_work(), and 
the key point here is, it *already* takes a long time before we reach 
finish_writeback_work(), and there is true progress before finish the 
writeback work that hung task detector still can not know.

If we want to make the hung task detector to known the progress of 
writeback work, we need to add some code within do_writepages(): after 
each finish of a_ops->writepages(), we need to make detector to known 
there's progress. Something like this:

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 3e248d1c3969..49572a83c47b 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2635,6 +2635,10 @@ int do_writepages(struct address_space *mapping, 
struct writeback_control *wbc)
                 else
                         /* deal with chardevs and other special files */
                         ret = 0;
+               /* Make hung task detector to known there's progress. */
+               if (force_wake)
+                       wake_up_all(waitq);
+
                 if (ret != -ENOMEM || wbc->sync_mode != WB_SYNC_ALL)
                         break;

which has a big impact on current code - I don't want to introduce this.

Yes, the behavior in this patch does have the possibility to paper cover 
the real warnings, and what I want to argue is that the essence of this 
patch is the same as the current touch_nmi_watchdog() and 
touch_softlockup_watchdog() - these functions are used only in specific 
scenarios we known and only affect a single event. And there seems no 
report that touch_nmi/softlockup_watchdog() will paper cover the real 
warnings (do we?).

Correct me if there's anything I'm missing or misunderstanding.


> 
>>   mm/memcontrol.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 8dd7fbed5a94..fc73a56372a4 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -63,6 +63,7 @@
>>   #include <linux/seq_buf.h>
>>   #include <linux/sched/isolation.h>
>>   #include <linux/kmemleak.h>
>> +#include <linux/nmi.h>
>>   #include "internal.h"
>>   #include <net/sock.h>
>>   #include <net/ip.h>
>> @@ -3912,8 +3913,15 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
>>   	int __maybe_unused i;
>>   
>>   #ifdef CONFIG_CGROUP_WRITEBACK
>> -	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++)
>> +	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) {
>> +		/*
>> +		 * We don't want the hung task detector to report warnings
>> +		 * here since there's nothing wrong if the writeback work
>> +		 * lasts for a long time.
>> +		 */
>> +		touch_hung_task_detector(current);
>>   		wb_wait_for_completion(&memcg->cgwb_frn[i].done);
>> +	}
>>   #endif
>>   	if (cgroup_subsys_on_dfl(memory_cgrp_subsys) && !cgroup_memory_nosocket)
>>   		static_branch_dec(&memcg_sockets_enabled_key);
>> -- 
>> 2.39.5
>>

Thanks,
-- 
Julian Sun <sunjunchao@bytedance.com>