| Message ID | 20241212-blk-iocost-fix-clamp-error-v1-1-b925491bc7d3@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | blk-iocost: Avoid using clamp() on inuse in __propagate_weights() | expand |
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.
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)
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 >
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,
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;
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(-) --- base-commit: 3e42dc9229c5950e84b1ed705f94ed75ed208228 change-id: 20241212-blk-iocost-fix-clamp-error-5816c028b245 Best regards,