diff mbox series

[v2] mm: Fix memcg writeback for rt tasks

Message ID 20230411082248.1020719-1-yizhou.tang@shopee.com (mailing list archive)
State New
Headers show
Series [v2] mm: Fix memcg writeback for rt tasks | expand

Commit Message

Tang Yizhou April 11, 2023, 8:22 a.m. UTC 
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_THROTTLE")
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>
---
v2:
Rewrite the commit message.

 mm/page-writeback.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Kara April 11, 2023, 11:21 a.m. UTC | #1 
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_THROTTLE")
> 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_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
Tang Yizhou April 21, 2023, 2:57 p.m. UTC | #2 
CC Christoph Hellwig and Michal Hocko.


On Tue, Apr 11, 2023 at 7:21 PM 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_THROTTLE")
> > 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_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@suse.com>
> SUSE Labs, CR
diff mbox series

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;
 	}
 	dtc->thresh = thresh;
 	dtc->bg_thresh = bg_thresh;