Factor out balance_wb_limits to remove repeated code

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/page-writeback.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 0f1f3e179be2..d1d385373c5b 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -1783,6 +1783,17 @@ static inline void wb_dirty_exceeded(struct dirty_throttle_control *dtc,
 		((dtc->dirty > dtc->thresh) || strictlimit);
 }
 
+static void balance_wb_limits(struct dirty_throttle_control *dtc,
+			      bool strictlimit)
+{
+	wb_dirty_freerun(dtc, strictlimit);
+	if (dtc->freerun)
+		return;
+
+	wb_dirty_exceeded(dtc, strictlimit);
+	wb_position_ratio(dtc);
+}
+
 /*
  * balance_dirty_pages() must be called by processes which are generating dirty
  * data.  It looks at the number of dirty pages in the machine and will force
@@ -1869,12 +1880,9 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
 		 * Calculate global domain's pos_ratio and select the
 		 * global dtc by default.
 		 */
-		wb_dirty_freerun(gdtc, strictlimit);
+		balance_wb_limits(gdtc, strictlimit);
 		if (gdtc->freerun)
 			goto free_running;
-
-		wb_dirty_exceeded(gdtc, strictlimit);
-		wb_position_ratio(gdtc);
 		sdtc = gdtc;
 
 		if (mdtc) {
@@ -1884,12 +1892,9 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
 			 * both global and memcg domains.  Choose the one
 			 * w/ lower pos_ratio.
 			 */
-			wb_dirty_freerun(mdtc, strictlimit);
+			balance_wb_limits(mdtc, strictlimit);
 			if (mdtc->freerun)
 				goto free_running;
-
-			wb_dirty_exceeded(mdtc, strictlimit);
-			wb_position_ratio(mdtc);
 			if (mdtc->pos_ratio < gdtc->pos_ratio)
 				sdtc = mdtc;
 		}
-- 
2.30.0

Hello,

On Tue, May 14, 2024 at 08:52:54PM +0800, Kemeng Shi wrote:
> +static void balance_wb_limits(struct dirty_throttle_control *dtc,
> +			      bool strictlimit)
> +{
> +	wb_dirty_freerun(dtc, strictlimit);
> +	if (dtc->freerun)
> +		return;
> +
> +	wb_dirty_exceeded(dtc, strictlimit);
> +	wb_position_ratio(dtc);
> +}
...
> @@ -1869,12 +1880,9 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
>  		 * Calculate global domain's pos_ratio and select the
>  		 * global dtc by default.
>  		 */
> -		wb_dirty_freerun(gdtc, strictlimit);
> +		balance_wb_limits(gdtc, strictlimit);
>  		if (gdtc->freerun)
>  			goto free_running;
> -
> -		wb_dirty_exceeded(gdtc, strictlimit);
> -		wb_position_ratio(gdtc);
>  		sdtc = gdtc;

Isn't this a bit nasty? The helper skips updating states because it knows
the caller is not going to use them? I'm not sure the slight code reduction
justifies the added subtlety.

Thanks.

-- 
tejun

Hello,
on 5/31/2024 2:33 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, May 14, 2024 at 08:52:54PM +0800, Kemeng Shi wrote:
>> +static void balance_wb_limits(struct dirty_throttle_control *dtc,
>> +			      bool strictlimit)
>> +{
>> +	wb_dirty_freerun(dtc, strictlimit);
>> +	if (dtc->freerun)
>> +		return;
>> +
>> +	wb_dirty_exceeded(dtc, strictlimit);
>> +	wb_position_ratio(dtc);
>> +}
> ...
>> @@ -1869,12 +1880,9 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
>>  		 * Calculate global domain's pos_ratio and select the
>>  		 * global dtc by default.
>>  		 */
>> -		wb_dirty_freerun(gdtc, strictlimit);
>> +		balance_wb_limits(gdtc, strictlimit);
>>  		if (gdtc->freerun)
>>  			goto free_running;
>> -
>> -		wb_dirty_exceeded(gdtc, strictlimit);
>> -		wb_position_ratio(gdtc);
>>  		sdtc = gdtc;
> 
> Isn't this a bit nasty? The helper skips updating states because it knows
> the caller is not going to use them? I'm not sure the slight code reduction
> justifies the added subtlety.

It's a general rule that wb should not be limited if the wb is in freerun state.
So I think it's intuitive to obey the rule in both balance_wb_limits and it's
caller in which case balance_wb_limits and it's caller should stop to do anything
when freerun state of wb is first seen.
But no insistant on this...

Thanks.
> 
> Thanks.
>

Hello,

On Mon, Jun 03, 2024 at 02:39:18PM +0800, Kemeng Shi wrote:
> > Isn't this a bit nasty? The helper skips updating states because it knows
> > the caller is not going to use them? I'm not sure the slight code reduction
> > justifies the added subtlety.
> 
> It's a general rule that wb should not be limited if the wb is in freerun state.
> So I think it's intuitive to obey the rule in both balance_wb_limits and it's
> caller in which case balance_wb_limits and it's caller should stop to do anything
> when freerun state of wb is first seen.
> But no insistant on this...

Hmm... can you at least add comments pointing out that if freerun, the
limits fields are invalid?

Thanks.

-- 
tejun

on 6/4/2024 2:09 AM, Tejun Heo wrote:
> Hello,
> 
> On Mon, Jun 03, 2024 at 02:39:18PM +0800, Kemeng Shi wrote:
>>> Isn't this a bit nasty? The helper skips updating states because it knows
>>> the caller is not going to use them? I'm not sure the slight code reduction
>>> justifies the added subtlety.
>>
>> It's a general rule that wb should not be limited if the wb is in freerun state.
>> So I think it's intuitive to obey the rule in both balance_wb_limits and it's
>> caller in which case balance_wb_limits and it's caller should stop to do anything
>> when freerun state of wb is first seen.
>> But no insistant on this...
> 
> Hmm... can you at least add comments pointing out that if freerun, the
> limits fields are invalid?
Sure, will add it in next version. Thanks
> 
> Thanks.
>