linux-kernel - Re: [PATCH RESEND 2/2] writeback: Fix calculations in trace_balance_dirty

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]

Message-ID: <rcfl3znyagtikvvzobic4hfuwzdjtrzwh3cuy4f6vbuq3emehl@2zx2bs75mszo>
Date: Tue, 4 Mar 2025 15:35:52 +0100
From: Jan Kara <jack@...e.cz>
To: Tang Yizhou <yizhou.tang@...pee.com>
Cc: tj@...nel.org, jack@...e.cz, brauner@...nel.org, willy@...radead.org, 
	akpm@...ux-foundation.org, rostedt@...dmis.org, mhiramat@...nel.org, ast@...nel.org, 
	linux-mm@...ck.org, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND 2/2] writeback: Fix calculations in
 trace_balance_dirty_pages() for cgwb

On Mon 03-03-25 18:06:17, Tang Yizhou wrote:
> From: Tang Yizhou <yizhou.tang@...pee.com>
> 
> In the commit dcc25ae76eb7 ("writeback: move global_dirty_limit into
> wb_domain") of the cgroup writeback backpressure propagation patchset,
> Tejun made some adaptations to trace_balance_dirty_pages() for cgroup
> writeback. However, this adaptation was incomplete and Tejun missed
> further adaptation in the subsequent patches.
> 
> In the cgroup writeback scenario, if sdtc in balance_dirty_pages() is
> assigned to mdtc, then upon entering trace_balance_dirty_pages(),
> __entry->limit should be assigned based on the dirty_limit of the
> corresponding memcg's wb_domain, rather than global_wb_domain.
> 
> To address this issue and simplify the implementation, introduce a 'limit'
> field in struct dirty_throttle_control to store the hard_limit value
> computed in wb_position_ratio() by calling hard_dirty_limit(). This field
> will then be used in trace_balance_dirty_pages() to assign the value to
> __entry->limit.
> 
> Fixes: dcc25ae76eb7 ("writeback: move global_dirty_limit into wb_domain")
> Signed-off-by: Tang Yizhou <yizhou.tang@...pee.com>

In principle this looks fine but one nit below:

> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> index 32095928365c..58bda3347914 100644
> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -326,6 +326,7 @@ struct dirty_throttle_control {
>  	unsigned long		dirty;		/* file_dirty + write + nfs */
>  	unsigned long		thresh;		/* dirty threshold */
>  	unsigned long		bg_thresh;	/* dirty background threshold */
> +	unsigned long		limit;		/* hard dirty limit */
				^^^ I'd call this dirty_limit to not invent
a new name for the same thing. I've noticed the tracepoint has 'limit' as
well but that is the outlier that should be modified if anything. Also I'd
modify the comment to /* smoothed dirty limit */ to better explain what
this is about.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR