| Message ID | 20240327152358.2368467-14-aleksander.lobakin@intel.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ice: add PFCP filter support | expand |
Hi Alexander, On 27/03/2024 3:23 pm, Alexander Lobakin wrote: > Now that we have generic bitmap_read() and bitmap_write(), which are > inline and try to take care of non-bound-crossing and aligned cases > to keep them optimized, collapse bitmap_{get,set}_value8() into > simple wrappers around the former ones. > bloat-o-meter shows no difference in vmlinux and -2 bytes for > gpio-pca953x.ko, which says the optimization didn't suffer due to > that change. The converted helpers have the value width embedded > and always compile-time constant and that helps a lot. This change appears to have introduced a build failure for me on arm64 (with GCC 9.4.0 from Ubuntu 20.04.02) - reverting b44759705f7d makes these errors go away again: In file included from drivers/gpio/gpio-pca953x.c:12: drivers/gpio/gpio-pca953x.c: In function ‘pca953x_probe’: ./include/linux/bitmap.h:799:17: error: array subscript [1, 1024] is outside array bounds of ‘long unsigned int[1]’ [-Werror=array-bounds] 799 | map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits); | ^~ In file included from ./include/linux/atomic.h:5, from drivers/gpio/gpio-pca953x.c:11: drivers/gpio/gpio-pca953x.c:1015:17: note: while referencing ‘val’ 1015 | DECLARE_BITMAP(val, MAX_LINE); | ^~~ ./include/linux/types.h:11:16: note: in definition of macro ‘DECLARE_BITMAP’ 11 | unsigned long name[BITS_TO_LONGS(bits)] | ^~~~ In file included from drivers/gpio/gpio-pca953x.c:12: ./include/linux/bitmap.h:800:17: error: array subscript [1, 1024] is outside array bounds of ‘long unsigned int[1]’ [-Werror=array-bounds] 800 | map[index + 1] |= (value >> space); | ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~ In file included from ./include/linux/atomic.h:5, from drivers/gpio/gpio-pca953x.c:11: drivers/gpio/gpio-pca953x.c:1015:17: note: while referencing ‘val’ 1015 | DECLARE_BITMAP(val, MAX_LINE); | ^~~ ./include/linux/types.h:11:16: note: in definition of macro ‘DECLARE_BITMAP’ 11 | unsigned long name[BITS_TO_LONGS(bits)] | ^~~~ I've not dug further since I don't have any interest in the pca953x driver - it just happened to be enabled in my config, so for now I've turned it off. However I couldn't obviously see any other reports of this, so here it is. Thanks, Robin.
On Wed, May 29, 2024 at 04:12:25PM +0100, Robin Murphy wrote: > Hi Alexander, > > On 27/03/2024 3:23 pm, Alexander Lobakin wrote: > > Now that we have generic bitmap_read() and bitmap_write(), which are > > inline and try to take care of non-bound-crossing and aligned cases > > to keep them optimized, collapse bitmap_{get,set}_value8() into > > simple wrappers around the former ones. > > bloat-o-meter shows no difference in vmlinux and -2 bytes for > > gpio-pca953x.ko, which says the optimization didn't suffer due to > > that change. The converted helpers have the value width embedded > > and always compile-time constant and that helps a lot. > > This change appears to have introduced a build failure for me on arm64 > (with GCC 9.4.0 from Ubuntu 20.04.02) - reverting b44759705f7d makes > these errors go away again: > > In file included from drivers/gpio/gpio-pca953x.c:12: > drivers/gpio/gpio-pca953x.c: In function ‘pca953x_probe’: > ./include/linux/bitmap.h:799:17: error: array subscript [1, 1024] is outside array bounds of ‘long unsigned int[1]’ [-Werror=array-bounds] > 799 | map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits); > | ^~ > In file included from ./include/linux/atomic.h:5, > from drivers/gpio/gpio-pca953x.c:11: > drivers/gpio/gpio-pca953x.c:1015:17: note: while referencing ‘val’ > 1015 | DECLARE_BITMAP(val, MAX_LINE); > | ^~~ > ./include/linux/types.h:11:16: note: in definition of macro ‘DECLARE_BITMAP’ > 11 | unsigned long name[BITS_TO_LONGS(bits)] > | ^~~~ > In file included from drivers/gpio/gpio-pca953x.c:12: > ./include/linux/bitmap.h:800:17: error: array subscript [1, 1024] is outside array bounds of ‘long unsigned int[1]’ [-Werror=array-bounds] > 800 | map[index + 1] |= (value >> space); > | ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~ > In file included from ./include/linux/atomic.h:5, > from drivers/gpio/gpio-pca953x.c:11: > drivers/gpio/gpio-pca953x.c:1015:17: note: while referencing ‘val’ > 1015 | DECLARE_BITMAP(val, MAX_LINE); > | ^~~ > ./include/linux/types.h:11:16: note: in definition of macro ‘DECLARE_BITMAP’ > 11 | unsigned long name[BITS_TO_LONGS(bits)] > | ^~~~ > > I've not dug further since I don't have any interest in the pca953x > driver - it just happened to be enabled in my config, so for now I've > turned it off. However I couldn't obviously see any other reports of > this, so here it is. It's a compiler false-positive. The straightforward fix is to disable the warning For gcc9+, and it's in Andrew Morton's tree alrady. but there's some discussion ongoing on how it should be mitigated properlu: https://lore.kernel.org/all/0ab2702f-8245-4f02-beb7-dcc7d79d5416@app.fastmail.com/T/ Thanks, YUry
On 30/05/2024 6:11 pm, Yury Norov wrote: > On Wed, May 29, 2024 at 04:12:25PM +0100, Robin Murphy wrote: >> Hi Alexander, >> >> On 27/03/2024 3:23 pm, Alexander Lobakin wrote: >>> Now that we have generic bitmap_read() and bitmap_write(), which are >>> inline and try to take care of non-bound-crossing and aligned cases >>> to keep them optimized, collapse bitmap_{get,set}_value8() into >>> simple wrappers around the former ones. >>> bloat-o-meter shows no difference in vmlinux and -2 bytes for >>> gpio-pca953x.ko, which says the optimization didn't suffer due to >>> that change. The converted helpers have the value width embedded >>> and always compile-time constant and that helps a lot. >> >> This change appears to have introduced a build failure for me on arm64 >> (with GCC 9.4.0 from Ubuntu 20.04.02) - reverting b44759705f7d makes >> these errors go away again: >> >> In file included from drivers/gpio/gpio-pca953x.c:12: >> drivers/gpio/gpio-pca953x.c: In function ‘pca953x_probe’: >> ./include/linux/bitmap.h:799:17: error: array subscript [1, 1024] is outside array bounds of ‘long unsigned int[1]’ [-Werror=array-bounds] >> 799 | map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits); >> | ^~ >> In file included from ./include/linux/atomic.h:5, >> from drivers/gpio/gpio-pca953x.c:11: >> drivers/gpio/gpio-pca953x.c:1015:17: note: while referencing ‘val’ >> 1015 | DECLARE_BITMAP(val, MAX_LINE); >> | ^~~ >> ./include/linux/types.h:11:16: note: in definition of macro ‘DECLARE_BITMAP’ >> 11 | unsigned long name[BITS_TO_LONGS(bits)] >> | ^~~~ >> In file included from drivers/gpio/gpio-pca953x.c:12: >> ./include/linux/bitmap.h:800:17: error: array subscript [1, 1024] is outside array bounds of ‘long unsigned int[1]’ [-Werror=array-bounds] >> 800 | map[index + 1] |= (value >> space); >> | ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~ >> In file included from ./include/linux/atomic.h:5, >> from drivers/gpio/gpio-pca953x.c:11: >> drivers/gpio/gpio-pca953x.c:1015:17: note: while referencing ‘val’ >> 1015 | DECLARE_BITMAP(val, MAX_LINE); >> | ^~~ >> ./include/linux/types.h:11:16: note: in definition of macro ‘DECLARE_BITMAP’ >> 11 | unsigned long name[BITS_TO_LONGS(bits)] >> | ^~~~ >> >> I've not dug further since I don't have any interest in the pca953x >> driver - it just happened to be enabled in my config, so for now I've >> turned it off. However I couldn't obviously see any other reports of >> this, so here it is. > > It's a compiler false-positive. The straightforward fix is to disable the warning > For gcc9+, and it's in Andrew Morton's tree alrady. but there's some discussion > ongoing on how it should be mitigated properlu: > > https://lore.kernel.org/all/0ab2702f-8245-4f02-beb7-dcc7d79d5416@app.fastmail.com/T/ Ah, great! Guess I really should have scrolled further down my lore search results - I assumed I was looking for any other reports of a recent regression in mainline, not ones from 6 months ago :) Cheers, Robin.
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h index 363e0b184a45..8c4768c44a01 100644 --- a/include/linux/bitmap.h +++ b/include/linux/bitmap.h @@ -727,39 +727,6 @@ static inline void bitmap_from_u64(unsigned long *dst, u64 mask) bitmap_from_arr64(dst, &mask, 64); } -/** - * bitmap_get_value8 - get an 8-bit value within a memory region - * @map: address to the bitmap memory region - * @start: bit offset of the 8-bit value; must be a multiple of 8 - * - * Returns the 8-bit value located at the @start bit offset within the @src - * memory region. - */ -static inline unsigned long bitmap_get_value8(const unsigned long *map, - unsigned long start) -{ - const size_t index = BIT_WORD(start); - const unsigned long offset = start % BITS_PER_LONG; - - return (map[index] >> offset) & 0xFF; -} - -/** - * bitmap_set_value8 - set an 8-bit value within a memory region - * @map: address to the bitmap memory region - * @value: the 8-bit value; values wider than 8 bits may clobber bitmap - * @start: bit offset of the 8-bit value; must be a multiple of 8 - */ -static inline void bitmap_set_value8(unsigned long *map, unsigned long value, - unsigned long start) -{ - const size_t index = BIT_WORD(start); - const unsigned long offset = start % BITS_PER_LONG; - - map[index] &= ~(0xFFUL << offset); - map[index] |= value << offset; -} - /** * bitmap_read - read a value of n-bits from the memory region * @map: address to the bitmap memory region @@ -833,6 +800,11 @@ static inline void bitmap_write(unsigned long *map, unsigned long value, map[index + 1] |= (value >> space); } +#define bitmap_get_value8(map, start) \ + bitmap_read(map, start, BITS_PER_BYTE) +#define bitmap_set_value8(map, value, start) \ + bitmap_write(map, value, start, BITS_PER_BYTE) + #endif /* __ASSEMBLY__ */ #endif /* __LINUX_BITMAP_H */
Now that we have generic bitmap_read() and bitmap_write(), which are inline and try to take care of non-bound-crossing and aligned cases to keep them optimized, collapse bitmap_{get,set}_value8() into simple wrappers around the former ones. bloat-o-meter shows no difference in vmlinux and -2 bytes for gpio-pca953x.ko, which says the optimization didn't suffer due to that change. The converted helpers have the value width embedded and always compile-time constant and that helps a lot. Suggested-by: Yury Norov <yury.norov@gmail.com> Signed-off-by: Yury Norov <yury.norov@gmail.com> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> --- include/linux/bitmap.h | 38 +++++--------------------------------- 1 file changed, 5 insertions(+), 33 deletions(-)