Re: [PATCH v6] memcg: Don't wait writeback completion when release memcg.

[Lance Yang <lance.yang@linux.dev>]

Hey Julian,

On 2025/9/18 06:21, Andrew Morton wrote:
> On Thu, 18 Sep 2025 05:29:59 +0800 Julian Sun <sunjunchao@bytedance.com> wrote:
> 
>> Recently, we encountered the following hung task:
>>
>> INFO: task kworker/4:1:1334558 blocked for more than 1720 seconds.
>> [Wed Jul 30 17:47:45 2025] Workqueue: cgroup_destroy css_free_rwork_fn
>>
>> ...
>>
>> The direct cause is that memcg spends a long time waiting for dirty page
>> writeback of foreign memcgs during release.
>>
>> The root causes are:
>>      a. The wb may have multiple writeback tasks, containing millions
>>         of dirty pages, as shown below:
>>
>>>>> for work in list_for_each_entry("struct wb_writeback_work", \
>> 				    wb.work_list.address_of_(), "list"):
>> ...     print(work.nr_pages, work.reason, hex(work))
>> ...
>> 900628  WB_REASON_FOREIGN_FLUSH 0xffff969e8d956b40
>> 1116521 WB_REASON_FOREIGN_FLUSH 0xffff9698332a9540
>>
>> ...
>>
> 
> I don't think it's particularly harmful that a dedicated worker thread
> has to wait for a long time in this fashion.  It doesn't have anything
> else to do (does it?) and a blocked kernel thread is cheap.

Looking at wb_wait_for_completion(), one could introduce a new helper that
internally uses wait_event_timeout() in a loop. Something like:

void wb_wait_for_completion_with_timeout(struct wb_completion *done)
{
	atomic_dec(&done->cnt);
	while (atomic_read(&done->cnt))
         	wait_event_timeout(*done->waitq, !atomic_read(&done->cnt), 
timeout);
}

With this, the detector should no longer complain for that specific case :)

> 
>> 3085016 WB_REASON_FOREIGN_FLUSH 0xffff969f0455e000
>> 3035712 WB_REASON_FOREIGN_FLUSH 0xffff969d9bbf4b00
>>
>>      b. The writeback might severely throttled by wbt, with a speed
>>         possibly less than 100kb/s, leading to a very long writeback time.
>>
>> ...
>>
>>   include/linux/memcontrol.h | 14 +++++++++-
>>   mm/memcontrol.c            | 57 ++++++++++++++++++++++++++++++++------
>>   2 files changed, 62 insertions(+), 9 deletions(-)
> 
> Seems we're adding a bunch of tricky code to fix a non-problem which
> the hung-task detector undesirably reports.
> 
> Would a better fix be to simply suppress the warning?
> 
> I don't think we presently have a touch_hung_task_detector() (do we?)
> but it's presumably pretty simple.  And maybe
> touch_softlockup_watchdog) should be taught to call that
> touch_hung_task_dectector().

Yes, introducing a touch_hung_task_detector() and having other tasks
periodically call it for the blocked worker seems like a much cleaner
approach to suppress the warning, IMHO.

> 
> Another approach might be to set some flag in the task_struct
> instructing the hung task detector to ignore this thread.

Alternatively, the idea of setting a flag for the worker to explicitly
ignore certain tasks by the detector is also interesting, especially if the
worker's blocked state is expected and benign ;)

Well, happy to help explore either of these paths if you'd like to go 
further
with them ;P

Cheers,
Lance