Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230411082248.1020719-1-yizhou.tang@shopee.com> <20230411112130.7zswqnyifgjhshif@quack3>
In-Reply-To: <20230411112130.7zswqnyifgjhshif@quack3>
From:   Tang Yizhou <yizhou.tang@shopee.com>
Date:   Fri, 21 Apr 2023 22:57:22 +0800
Message-ID: <CACuPKx=+g_bhLKH1d9M4WOUVyRRKGj+sP0q4td1gRE9ghtVYog@mail.gmail.com>
Subject: Re: [PATCH v2] mm: Fix memcg writeback for rt tasks
To:     Jan Kara <jack@suse.cz>
Cc:     neilb@suse.com, tj@kernel.org, wufengguang@huawei.com,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org, hch@lst.de,
        mhocko@suse.com, tangyeechou@gmail.com, chunguang.xu@shopee.com,
        yue.zhao@shopee.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

CC Christoph Hellwig and Michal Hocko.


On Tue, Apr 11, 2023 at 7:21=E2=80=AFPM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 11-04-23 16:22:48, yizhou.tang@shopee.com wrote:
> > From: Tang Yizhou <yizhou.tang@shopee.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_TH=
ROTTLE")
> > CC: NeilBrown <neilb@suse.com>
> > CC: Tejun Heo <tj@kernel.org>
> > CC: Fengguang Wu <wufengguang@huawei.com>
> > Signed-off-by: Tang Yizhou <yizhou.tang@shopee.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_thrott=
le_control *dtc)
> >               bg_thresh =3D thresh / 2;
> >       tsk =3D current;
> >       if (rt_task(tsk)) {
> > -             bg_thresh +=3D bg_thresh / 4 + global_wb_domain.dirty_lim=
it / 32;
> > -             thresh +=3D thresh / 4 + global_wb_domain.dirty_limit / 3=
2;
> > +             bg_thresh +=3D bg_thresh / 4 + dtc_dom(dtc)->dirty_limit =
/ 32;
> > +             thresh +=3D 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 t=
he
> 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() an=
d
> hence using global_wb_domain.dirty_limit is indeed correct. Neil?
>
>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR