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;
Date:   Tue, 11 Apr 2023 13:21:30 +0200
From:   Jan Kara <jack@suse.cz>
To:     yizhou.tang@shopee.com
Cc:     neilb@suse.com, tj@kernel.org, wufengguang@huawei.com,
        jack@suse.cz, linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        tangyeechou@gmail.com, chunguang.xu@shopee.com, yue.zhao@shopee.com
Subject: Re: [PATCH v2] mm: Fix memcg writeback for rt tasks
Message-ID: <20230411112130.7zswqnyifgjhshif@quack3>
References: <20230411082248.1020719-1-yizhou.tang@shopee.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230411082248.1020719-1-yizhou.tang@shopee.com>
Precedence: bulk

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