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

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 18 Sep 2025 11:03:18 +0800 Julian Sun <sunjunchao@bytedance.com> wrote:

> Hi,
> Thanks for your review and comments.
> 
> On Thu, Sep 18, 2025 at 6:22 AM Andrew Morton <akpm@linux-foundation.org> 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.
> 
> It also delays the release of other resources and the update of
> vmstats and vmevents statistics for the parent cgroup.

This is new - such considerations weren't described in the changelog. 
How much of a problem are these things?

> But we can put
> the wb_wait_for_completion() after the release of these resources.

Can we move these actions into the writeback completion path which
seems to be the most accurate way to do them?

> >
> > > 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.
> 
> Emm.. What is the definition of 'undesirably' here?

Seems the intent here is mainly to prevent the warning.  If that
warning wasn't coming out, would we bother making these changes?  If
no, just kill the warning.

> >
> > 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().
> >
> > Another approach might be to set some flag in the task_struct
> > instructing the hung task detector to ignore this thread.
> 
> To me, this feels kind of like a workaround rather than a real fix.

I don't see why.  It appears that the kworker's intended role is to
wait for writeback completion then to finish things up.  Which it does
just fine, except there's this pesky warning we get.  Therefore: kill
the pesky warning,

> And these approaches are beyond the scope of memcg,

So expand the scope?  If hung-task doesn't have a way to suppress
inappropriate warnings then add it and use it.

> I'm not sure how
> Tejun thinks about it since he mentioned before wanting to keep the
> modifications inside the memcg. Given the fact we already have an
> existing solution and code, and the scope of impact is confined to
> memcg, I prefer to use the existing solution.

mmm...  sunk cost fallacy!  Let's just opt for the best solution,
regardless of cost.