+ mm-rework-calculation-of-bdi_min_ratio-in-bdi_set_min_ratio.patch added to -mm tree

+ mm-rework-calculation-of-bdi_min_ratio-in-bdi_set_min_ratio.patch added to -mm tree

[Andrew Morton]

The patch titled
     Subject: mm: rework calculation of bdi_min_ratio in bdi_set_min_ratio
has been added to the -mm tree.  Its filename is
     mm-rework-calculation-of-bdi_min_ratio-in-bdi_set_min_ratio.patch

This patch should soon appear at
    https://ozlabs.org/~akpm/mmots/broken-out/mm-rework-calculation-of-bdi_min_ratio-in-bdi_set_min_ratio.patch
and later at
    https://ozlabs.org/~akpm/mmotm/broken-out/mm-rework-calculation-of-bdi_min_ratio-in-bdi_set_min_ratio.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***

The -mm tree is included into linux-next and is updated
there every 3-4 working days

------------------------------------------------------
From: Chen Wandun <chenwandun@huawei.com>
Subject: mm: rework calculation of bdi_min_ratio in bdi_set_min_ratio

In function bdi_set_min_ratio, min_ratio is unsigned int, it will
result underflow when setting min_ratio below bdi->min_ratio, it
is confusing. Rework it, no functional change.

Link: https://lkml.kernel.org/r/20220422095159.2858305-1-chenwandun@huawei.com
Signed-off-by: Chen Wandun <chenwandun@huawei.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/page-writeback.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

--- a/mm/page-writeback.c~mm-rework-calculation-of-bdi_min_ratio-in-bdi_set_min_ratio
+++ a/mm/page-writeback.c
@@ -650,18 +650,25 @@ static unsigned int bdi_min_ratio;
 
 int bdi_set_min_ratio(struct backing_dev_info *bdi, unsigned int min_ratio)
 {
+	unsigned int delta;
 	int ret = 0;
 
 	spin_lock_bh(&bdi_lock);
 	if (min_ratio > bdi->max_ratio) {
 		ret = -EINVAL;
 	} else {
-		min_ratio -= bdi->min_ratio;
-		if (bdi_min_ratio + min_ratio < 100) {
-			bdi_min_ratio += min_ratio;
-			bdi->min_ratio += min_ratio;
+		if (min_ratio < bdi->min_ratio) {
+			delta = bdi->min_ratio - min_ratio;
+			bdi_min_ratio -= delta;
+			bdi->min_ratio = min_ratio;
 		} else {
-			ret = -EINVAL;
+			delta = min_ratio - bdi->min_ratio;
+			if (bdi_min_ratio + delta < 100) {
+				bdi_min_ratio += delta;
+				bdi->min_ratio = min_ratio;
+			} else {
+				ret = -EINVAL;
+			}
 		}
 	}
 	spin_unlock_bh(&bdi_lock);
_

Patches currently in -mm which might be from chenwandun@huawei.com are

mm-rework-calculation-of-bdi_min_ratio-in-bdi_set_min_ratio.patch
mm-page_alloc-simplify-update-of-pgdat-in-wake_all_kswapds.patch

Re: + mm-rework-calculation-of-bdi_min_ratio-in-bdi_set_min_ratio.patch added to -mm tree

[Peter Zijlstra]

On Fri, Apr 22, 2022 at 01:54:13PM -0700, Andrew Morton wrote:

> From: Chen Wandun <chenwandun@huawei.com>
> Subject: mm: rework calculation of bdi_min_ratio in bdi_set_min_ratio
> 
> In function bdi_set_min_ratio, min_ratio is unsigned int, it will
> result underflow when setting min_ratio below bdi->min_ratio, it
> is confusing. Rework it, no functional change.
> 
> Link: https://lkml.kernel.org/r/20220422095159.2858305-1-chenwandun@huawei.com
> Signed-off-by: Chen Wandun <chenwandun@huawei.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  mm/page-writeback.c |   17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> --- a/mm/page-writeback.c~mm-rework-calculation-of-bdi_min_ratio-in-bdi_set_min_ratio
> +++ a/mm/page-writeback.c
> @@ -650,18 +650,25 @@ static unsigned int bdi_min_ratio;
>  
>  int bdi_set_min_ratio(struct backing_dev_info *bdi, unsigned int min_ratio)
>  {
> +	unsigned int delta;
>  	int ret = 0;
>  
>  	spin_lock_bh(&bdi_lock);
>  	if (min_ratio > bdi->max_ratio) {
>  		ret = -EINVAL;
>  	} else {

> -		min_ratio -= bdi->min_ratio;
> -		if (bdi_min_ratio + min_ratio < 100) {
> -			bdi_min_ratio += min_ratio;
> -			bdi->min_ratio += min_ratio;
>  		} else {
> -			ret = -EINVAL;

I really don't see the why we're doing this, code gets to be worse, for
no actual gain.

> +		if (min_ratio < bdi->min_ratio) {
> +			delta = bdi->min_ratio - min_ratio;
> +			bdi_min_ratio -= delta;
> +			bdi->min_ratio = min_ratio;
>  		} else {
> +			delta = min_ratio - bdi->min_ratio;
> +			if (bdi_min_ratio + delta < 100) {
> +				bdi_min_ratio += delta;
> +				bdi->min_ratio = min_ratio;
> +			} else {
> +				ret = -EINVAL;
> +			}
>  		}

>  	}

Re: + mm-rework-calculation-of-bdi_min_ratio-in-bdi_set_min_ratio.patch added to -mm tree

[Andrew Morton]

On Mon, 25 Apr 2022 10:37:23 +0200 Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Apr 22, 2022 at 01:54:13PM -0700, Andrew Morton wrote:
> 
> > From: Chen Wandun <chenwandun@huawei.com>
> > Subject: mm: rework calculation of bdi_min_ratio in bdi_set_min_ratio
> > 
> > In function bdi_set_min_ratio, min_ratio is unsigned int, it will
> > result underflow when setting min_ratio below bdi->min_ratio, it
> > is confusing. Rework it, no functional change.
> > 
>
> ...
>
> > 
> > --- a/mm/page-writeback.c~mm-rework-calculation-of-bdi_min_ratio-in-bdi_set_min_ratio
> > +++ a/mm/page-writeback.c
> > @@ -650,18 +650,25 @@ static unsigned int bdi_min_ratio;
> >  
> >  int bdi_set_min_ratio(struct backing_dev_info *bdi, unsigned int min_ratio)
> >  {
> > +	unsigned int delta;
> >  	int ret = 0;
> >  
> >  	spin_lock_bh(&bdi_lock);
> >  	if (min_ratio > bdi->max_ratio) {
> >  		ret = -EINVAL;
> >  	} else {
> 
> > -		min_ratio -= bdi->min_ratio;
> > -		if (bdi_min_ratio + min_ratio < 100) {
> > -			bdi_min_ratio += min_ratio;
> > -			bdi->min_ratio += min_ratio;
> >  		} else {
> > -			ret = -EINVAL;
> 
> I really don't see the why we're doing this, code gets to be worse, for
> no actual gain.

Oh.  I found the resulting code to be considerably more understandable.
I mean, how does the old code work?  afaict it treats `unsigned
min_ratio' as a signed value(!) and relies upon an underflowed unsigned
to be a large number (> 100) when avoiding underflowing global
bdi_min_ratio.  Or something like that.  Also, modifying an incoming
arg's value is kinda rude.

And there's no change in the object file's .text size.

> > +		if (min_ratio < bdi->min_ratio) {
> > +			delta = bdi->min_ratio - min_ratio;
> > +			bdi_min_ratio -= delta;
> > +			bdi->min_ratio = min_ratio;
> >  		} else {
> > +			delta = min_ratio - bdi->min_ratio;
> > +			if (bdi_min_ratio + delta < 100) {
> > +				bdi_min_ratio += delta;
> > +				bdi->min_ratio = min_ratio;
> > +			} else {
> > +				ret = -EINVAL;
> > +			}
> >  		}
> 
> >  	}