Re: [PATCH 1/3] mm/page-writeback.c: Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL

[Jan Kara]

On Wed 02-10-24 21:00:02, Tang Yizhou wrote:
> From: Tang Yizhou <yizhou.tang@xxxxxxxxxx>
> 
> The name of the BANDWIDTH_INTERVAL macro is misleading, as it is not
> only used in the bandwidth update functions wb_update_bandwidth() and
> __wb_update_bandwidth(), but also in the dirty limit update function
> domain_update_dirty_limit().
> 
> Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL to make things clear.
> 
> This patche doesn't introduce any behavioral changes.
> 
> Signed-off-by: Tang Yizhou <yizhou.tang@xxxxxxxxxx>

Umm, I agree BANDWIDTH_INTERVAL may be confusing but UPDATE_INTERVAL does
not seem much better to be honest. I actually have hard time coming up with
a more descriptive name so what if we settled on updating the comment only
instead of renaming to something not much better?

								Honza

> ---
>  mm/page-writeback.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index fcd4c1439cb9..a848e7f0719d 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -54,9 +54,9 @@
>  #define DIRTY_POLL_THRESH	(128 >> (PAGE_SHIFT - 10))
>  
>  /*
> - * Estimate write bandwidth at 200ms intervals.
> + * Estimate write bandwidth or update dirty limit at 200ms intervals.
>   */
> -#define BANDWIDTH_INTERVAL	max(HZ/5, 1)
> +#define UPDATE_INTERVAL		max(HZ/5, 1)
>  
>  #define RATELIMIT_CALC_SHIFT	10
>  
> @@ -1331,11 +1331,11 @@ static void domain_update_dirty_limit(struct dirty_throttle_control *dtc,
>  	/*
>  	 * check locklessly first to optimize away locking for the most time
>  	 */
> -	if (time_before(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL))
> +	if (time_before(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL))
>  		return;
>  
>  	spin_lock(&dom->lock);
> -	if (time_after_eq(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL)) {
> +	if (time_after_eq(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL)) {
>  		update_dirty_limit(dtc);
>  		dom->dirty_limit_tstamp = now;
>  	}
> @@ -1928,7 +1928,7 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
>  		wb->dirty_exceeded = gdtc->dirty_exceeded ||
>  				     (mdtc && mdtc->dirty_exceeded);
>  		if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> -					   BANDWIDTH_INTERVAL))
> +					   UPDATE_INTERVAL))
>  			__wb_update_bandwidth(gdtc, mdtc, true);
>  
>  		/* throttle according to the chosen dtc */
> @@ -2705,7 +2705,7 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
>  	 * writeback bandwidth is updated once in a while.
>  	 */
>  	if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> -				   BANDWIDTH_INTERVAL))
> +				   UPDATE_INTERVAL))
>  		wb_update_bandwidth(wb);
>  	return ret;
>  }
> @@ -3057,14 +3057,14 @@ static void wb_inode_writeback_end(struct bdi_writeback *wb)
>  	atomic_dec(&wb->writeback_inodes);
>  	/*
>  	 * Make sure estimate of writeback throughput gets updated after
> -	 * writeback completed. We delay the update by BANDWIDTH_INTERVAL
> +	 * writeback completed. We delay the update by UPDATE_INTERVAL
>  	 * (which is the interval other bandwidth updates use for batching) so
>  	 * that if multiple inodes end writeback at a similar time, they get
>  	 * batched into one bandwidth update.
>  	 */
>  	spin_lock_irqsave(&wb->work_lock, flags);
>  	if (test_bit(WB_registered, &wb->state))
> -		queue_delayed_work(bdi_wq, &wb->bw_dwork, BANDWIDTH_INTERVAL);
> +		queue_delayed_work(bdi_wq, &wb->bw_dwork, UPDATE_INTERVAL);
>  	spin_unlock_irqrestore(&wb->work_lock, flags);
>  }
>  
> -- 
> 2.25.1
> 
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR