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

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] [day] [month] [year] [list]

Date:   Fri, 21 Apr 2023 22:57:22 +0800
From:   Tang Yizhou <yizhou.tang@...pee.com>
To:     Jan Kara <jack@...e.cz>
Cc:     neilb@...e.com, tj@...nel.org, wufengguang@...wei.com,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org, hch@....de,
        mhocko@...e.com, tangyeechou@...il.com, chunguang.xu@...pee.com,
        yue.zhao@...pee.com
Subject: Re: [PATCH v2] mm: Fix memcg writeback for rt tasks

CC Christoph Hellwig and Michal Hocko.


On Tue, Apr 11, 2023 at 7:21 PM Jan Kara <jack@...e.cz> wrote:
>
> On Tue 11-04-23 16:22:48, yizhou.tang@...pee.com wrote:
> > From: Tang Yizhou <yizhou.tang@...pee.com>
> >
> > 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@...e.com>
> > CC: Tejun Heo <tj@...nel.org>
> > CC: Fengguang Wu <wufengguang@...wei.com>
> > Signed-off-by: Tang Yizhou <yizhou.tang@...pee.com>
>
> 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@...e.com>
> SUSE Labs, CR