Re: [PATCH v2] mm: Fix memcg writeback for rt tasks

[Tang Yizhou]

CC Christoph Hellwig and Michal Hocko.


On Tue, Apr 11, 2023 at 7:21 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Tue 11-04-23 16:22:48, yizhou.tang@xxxxxxxxxx wrote:
> > From: Tang Yizhou <yizhou.tang@xxxxxxxxxx>
> >
> > In domain_dirty_limits(), the calculation of the thresh and bg_thresh
> > variable needs to consider whether it's for global dirtypage writeback
> > or memcg dirtypage writeback. However, in the rt_task branch, the
> > accumulation of both variables only considers the global_wb_domain,
> > which seems strange to me.
> >
> > I find the accumulation was introduced in the commit a53eaff8c119 ("MM:
> > increase safety margin provided by PF_LESS_THROTTLE"). IMHO, realtime
> > tasks are given a higher page cache limit because they require higher
> > responsiveness, but we also need to consider whether the writeback of
> > realtime tasks occurs in the global dirtypage writeback or in the memcg
> > dirtypage writeback scenario.
> >
> > Later Neil said he didn't know what was wanted for realtime in the
> > commit message of commit a37b0715ddf3 ("mm/writeback: replace
> > PF_LESS_THROTTLE with PF_LOCAL_THROTTLE"). I guess he made this small
> > mistake since the commit a53eaff8c119 ("MM: increase safety margin
> > provided by PF_LESS_THROTTLE").
> >
> > Fixes: a53eaff8c119 ("MM: increase safety margin provided by PF_LESS_THROTTLE")
> > CC: NeilBrown <neilb@xxxxxxxx>
> > CC: Tejun Heo <tj@xxxxxxxxxx>
> > CC: Fengguang Wu <wufengguang@xxxxxxxxxx>
> > Signed-off-by: Tang Yizhou <yizhou.tang@xxxxxxxxxx>
>
> Thanks for the patch! Was this found just by code inspection or is there
> any practical problem you are trying to fix with this patch?
>
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index 516b1aa247e8..7d92de73360e 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -419,8 +419,8 @@ static void domain_dirty_limits(struct dirty_throttle_control *dtc)
> >               bg_thresh = thresh / 2;
> >       tsk = current;
> >       if (rt_task(tsk)) {
> > -             bg_thresh += bg_thresh / 4 + global_wb_domain.dirty_limit / 32;
> > -             thresh += thresh / 4 + global_wb_domain.dirty_limit / 32;
> > +             bg_thresh += bg_thresh / 4 + dtc_dom(dtc)->dirty_limit / 32;
> > +             thresh += thresh / 4 + dtc_dom(dtc)->dirty_limit / 32;
>
> This makes sense but I'm not 100% sure this does not reintroduce the
> problem a53eaff8c119 was trying to fix. Reading the changelog, it seems the
> extra term you are fixing is there specifically to deal with ratelimiting,
> which is global (and not per-memcg), of calls to balance_dirty_pages() and
> hence using global_wb_domain.dirty_limit is indeed correct. Neil?
>
>                                                                 Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR