[PATCH] blk-iocost: Avoid using clamp() on inuse in __propagate_weights()

[PATCH] blk-iocost: Avoid using clamp() on inuse in __propagate_weights()

[Nathan Chancellor]

After a recent change to clamp() and its variants [1] that increases the
coverage of the check that high is greater than low because it can be
done through inlining, certain build configurations (such as s390
defconfig) fail to build with clang with:

  block/blk-iocost.c:1101:11: error: call to '__compiletime_assert_557' declared with 'error' attribute: clamp() low limit 1 greater than high limit active
   1101 |                 inuse = clamp_t(u32, inuse, 1, active);
        |                         ^
  include/linux/minmax.h:218:36: note: expanded from macro 'clamp_t'
    218 | #define clamp_t(type, val, lo, hi) __careful_clamp(type, val, lo, hi)
        |                                    ^
  include/linux/minmax.h:195:2: note: expanded from macro '__careful_clamp'
    195 |         __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
        |         ^
  include/linux/minmax.h:188:2: note: expanded from macro '__clamp_once'
    188 |         BUILD_BUG_ON_MSG(statically_true(ulo > uhi),                            \
        |         ^

__propagate_weights() is called with an active value of zero in
ioc_check_iocgs(), which results in the high value being less than the
low value, which is undefined because the value returned depends on the
order of the comparisons.

The purpose of this expression is to ensure inuse is not more than
active and at least 1. This could be written more simply with a ternary
expression that uses min(inuse, active) as the condition so that the
value of that condition can be used if it is not zero and one if it is.
Do this conversion to resolve the error and add a comment to deter
people from turning this back into clamp().

Link: https://lore.kernel.org/r/34d53778977747f19cce2abb287bb3e6@AcuMS.aculab.com/ [1]
Suggested-by: David Laight <david.laight@aculab.com>
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Closes: https://lore.kernel.org/llvm/CA+G9fYsD7mw13wredcZn0L-KBA3yeoVSTuxnss-AEWMN3ha0cA@mail.gmail.com/
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202412120322.3GfVe3vF-lkp@intel.com/
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 block/blk-iocost.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 384aa15e8260bd4d83b00f1c03efb87829950327..a5894ec9696e7e8c1011cbda1d849562e1732d31 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1098,7 +1098,14 @@ static void __propagate_weights(struct ioc_gq *iocg, u32 active, u32 inuse,
 		inuse = DIV64_U64_ROUND_UP(active * iocg->child_inuse_sum,
 					   iocg->child_active_sum);
 	} else {
-		inuse = clamp_t(u32, inuse, 1, active);
+		/*
+		 * It may be tempting to turn this into a clamp expression with
+		 * a lower limit of 1 but active may be 0, which cannot be used
+		 * as an upper limit in that situation. This expression allows
+		 * active to clamp inuse unless it is 0, in which case inuse
+		 * becomes 1.
+		 */
+		inuse = min(inuse, active) ?: 1;
 	}
 
 	iocg->last_inuse = iocg->inuse;

---
base-commit: 3e42dc9229c5950e84b1ed705f94ed75ed208228
change-id: 20241212-blk-iocost-fix-clamp-error-5816c028b245

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

Re: [PATCH] blk-iocost: Avoid using clamp() on inuse in __propagate_weights()

[Tejun Heo]

On Thu, Dec 12, 2024 at 10:13:29AM -0700, Nathan Chancellor wrote:
> After a recent change to clamp() and its variants [1] that increases the
> coverage of the check that high is greater than low because it can be
> done through inlining, certain build configurations (such as s390
> defconfig) fail to build with clang with:
> 
>   block/blk-iocost.c:1101:11: error: call to '__compiletime_assert_557' declared with 'error' attribute: clamp() low limit 1 greater than high limit active
>    1101 |                 inuse = clamp_t(u32, inuse, 1, active);
>         |                         ^
>   include/linux/minmax.h:218:36: note: expanded from macro 'clamp_t'
>     218 | #define clamp_t(type, val, lo, hi) __careful_clamp(type, val, lo, hi)
>         |                                    ^
>   include/linux/minmax.h:195:2: note: expanded from macro '__careful_clamp'
>     195 |         __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
>         |         ^
>   include/linux/minmax.h:188:2: note: expanded from macro '__clamp_once'
>     188 |         BUILD_BUG_ON_MSG(statically_true(ulo > uhi),                            \
>         |         ^
> 
> __propagate_weights() is called with an active value of zero in
> ioc_check_iocgs(), which results in the high value being less than the
> low value, which is undefined because the value returned depends on the
> order of the comparisons.
> 
> The purpose of this expression is to ensure inuse is not more than
> active and at least 1. This could be written more simply with a ternary
> expression that uses min(inuse, active) as the condition so that the
> value of that condition can be used if it is not zero and one if it is.
> Do this conversion to resolve the error and add a comment to deter
> people from turning this back into clamp().
> 
> Link: https://lore.kernel.org/r/34d53778977747f19cce2abb287bb3e6@AcuMS.aculab.com/ [1]
> Suggested-by: David Laight <david.laight@aculab.com>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Closes: https://lore.kernel.org/llvm/CA+G9fYsD7mw13wredcZn0L-KBA3yeoVSTuxnss-AEWMN3ha0cA@mail.gmail.com/
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202412120322.3GfVe3vF-lkp@intel.com/
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

Acked-by: Tejun Heo <tj@kernel.org>

This likely deserves:

Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost")
Cc: stable@vger.kernel.org # v5.4+

Thanks.

-- 
tejun

RE: [PATCH] blk-iocost: Avoid using clamp() on inuse in __propagate_weights()

[David Laight]

From: Tejun Heo <tj@kernel.org>
> Sent: 12 December 2024 17:19
> 
> On Thu, Dec 12, 2024 at 10:13:29AM -0700, Nathan Chancellor wrote:
> > After a recent change to clamp() and its variants [1] that increases the
> > coverage of the check that high is greater than low because it can be
> > done through inlining, certain build configurations (such as s390
> > defconfig) fail to build with clang with:
> >
...
> > __propagate_weights() is called with an active value of zero in
> > ioc_check_iocgs(), which results in the high value being less than the
> > low value, which is undefined because the value returned depends on the
> > order of the comparisons.
> >
> > The purpose of this expression is to ensure inuse is not more than
> > active and at least 1. This could be written more simply with a ternary
> > expression that uses min(inuse, active) as the condition so that the
> > value of that condition can be used if it is not zero and one if it is.
> > Do this conversion to resolve the error and add a comment to deter
> > people from turning this back into clamp().
> >
> > Link: https://lore.kernel.org/r/34d53778977747f19cce2abb287bb3e6@AcuMS.aculab.com/ [1]
> > Suggested-by: David Laight <david.laight@aculab.com>
> > Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> > Closes: https://lore.kernel.org/llvm/CA+G9fYsD7mw13wredcZn0L-KBA3yeoVSTuxnss-
> AEWMN3ha0cA@mail.gmail.com/
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202412120322.3GfVe3vF-lkp@intel.com/
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> This likely deserves:
> 
> Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost")
> Cc: stable@vger.kernel.org # v5.4+

Especially since the old defn was:

#define __clamp(val, lo, hi)	\
	((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))

so:
-		inuse = clamp_t(u32, inuse, 1, active);

is zero if active is zero.

	David

> 
> Thanks.
> 
> --
> tejun

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] blk-iocost: Avoid using clamp() on inuse in __propagate_weights()

[Nathan Chancellor]

On Thu, Dec 12, 2024 at 07:19:23AM -1000, Tejun Heo wrote:
> On Thu, Dec 12, 2024 at 10:13:29AM -0700, Nathan Chancellor wrote:
> > After a recent change to clamp() and its variants [1] that increases the
> > coverage of the check that high is greater than low because it can be
> > done through inlining, certain build configurations (such as s390
> > defconfig) fail to build with clang with:
> > 
> >   block/blk-iocost.c:1101:11: error: call to '__compiletime_assert_557' declared with 'error' attribute: clamp() low limit 1 greater than high limit active
> >    1101 |                 inuse = clamp_t(u32, inuse, 1, active);
> >         |                         ^
> >   include/linux/minmax.h:218:36: note: expanded from macro 'clamp_t'
> >     218 | #define clamp_t(type, val, lo, hi) __careful_clamp(type, val, lo, hi)
> >         |                                    ^
> >   include/linux/minmax.h:195:2: note: expanded from macro '__careful_clamp'
> >     195 |         __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
> >         |         ^
> >   include/linux/minmax.h:188:2: note: expanded from macro '__clamp_once'
> >     188 |         BUILD_BUG_ON_MSG(statically_true(ulo > uhi),                            \
> >         |         ^
> > 
> > __propagate_weights() is called with an active value of zero in
> > ioc_check_iocgs(), which results in the high value being less than the
> > low value, which is undefined because the value returned depends on the
> > order of the comparisons.
> > 
> > The purpose of this expression is to ensure inuse is not more than
> > active and at least 1. This could be written more simply with a ternary
> > expression that uses min(inuse, active) as the condition so that the
> > value of that condition can be used if it is not zero and one if it is.
> > Do this conversion to resolve the error and add a comment to deter
> > people from turning this back into clamp().
> > 
> > Link: https://lore.kernel.org/r/34d53778977747f19cce2abb287bb3e6@AcuMS.aculab.com/ [1]
> > Suggested-by: David Laight <david.laight@aculab.com>
> > Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> > Closes: https://lore.kernel.org/llvm/CA+G9fYsD7mw13wredcZn0L-KBA3yeoVSTuxnss-AEWMN3ha0cA@mail.gmail.com/
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202412120322.3GfVe3vF-lkp@intel.com/
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> 
> Acked-by: Tejun Heo <tj@kernel.org>

Thanks for the quick response!

> This likely deserves:
> 
> Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost")
> Cc: stable@vger.kernel.org # v5.4+

Thanks, I was wondering if I should have provided those. I'll carry them
forward on any future revisions, as I assume Jens can pick those up with
your tag if this is good enough.

> 
> -- 
> tejun
> 

Re: [PATCH] blk-iocost: Avoid using clamp() on inuse in __propagate_weights()

[Jens Axboe]

On Thu, 12 Dec 2024 10:13:29 -0700, Nathan Chancellor wrote:
> After a recent change to clamp() and its variants [1] that increases the
> coverage of the check that high is greater than low because it can be
> done through inlining, certain build configurations (such as s390
> defconfig) fail to build with clang with:
> 
>   block/blk-iocost.c:1101:11: error: call to '__compiletime_assert_557' declared with 'error' attribute: clamp() low limit 1 greater than high limit active
>    1101 |                 inuse = clamp_t(u32, inuse, 1, active);
>         |                         ^
>   include/linux/minmax.h:218:36: note: expanded from macro 'clamp_t'
>     218 | #define clamp_t(type, val, lo, hi) __careful_clamp(type, val, lo, hi)
>         |                                    ^
>   include/linux/minmax.h:195:2: note: expanded from macro '__careful_clamp'
>     195 |         __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
>         |         ^
>   include/linux/minmax.h:188:2: note: expanded from macro '__clamp_once'
>     188 |         BUILD_BUG_ON_MSG(statically_true(ulo > uhi),                            \
>         |         ^
> 
> [...]

Applied, thanks!

[1/1] blk-iocost: Avoid using clamp() on inuse in __propagate_weights()
      commit: 57e420c84f9ab55ba4c5e2ae9c5f6c8e1ea834d2

Best regards,
-- 
Jens Axboe