On Mon, Nov 09, 2020 at 10:15:29PM +0530, Syed Nayyar Waris wrote: > On Mon, Nov 09, 2020 at 03:41:53PM +0100, Arnd Bergmann wrote: > > On Mon, Nov 9, 2020 at 2:41 PM William Breathitt Gray > > <vilhelm.gray@xxxxxxxxx> wrote: > > > On Mon, Nov 09, 2020 at 06:04:11PM +0530, Syed Nayyar Waris wrote: > > > > > > One of my concerns is that we're incurring the latency two additional > > > conditional checks just to suppress a compiler warning about a case that > > > wouldn't occur in the actual use of bitmap_set_value(). I'm hoping > > > there's a way for us to suppress these warnings without adding onto the > > > latency of this function; given that bitmap_set_value() is intended to > > > be used in loops, conditionals here could significantly increase latency > > > in drivers. > > > > At least for this caller, the size check would be a compile-time > > constant that can be eliminated. > > > > > I wonder if array_index_nospec() might have the side effect of > > > suppressing these warnings for us. For example, would this work: > > > > > > static inline void bitmap_set_value(unsigned long *map, > > > unsigned long value, > > > unsigned long start, unsigned long nbits) > > > { > > > const unsigned long offset = start % BITS_PER_LONG; > > > const unsigned long ceiling = round_up(start + 1, BITS_PER_LONG); > > > const unsigned long space = ceiling - start; > > > size_t index = BIT_WORD(start); > > > > > > value &= GENMASK(nbits - 1, 0); > > > > > > if (space >= nbits) { > > > index = array_index_nospec(index, index + 1); > > > > > > map[index] &= ~(GENMASK(nbits - 1, 0) << offset); > > > map[index] |= value << offset; > > > } else { > > > index = array_index_nospec(index, index + 2); > > > > > > map[index + 0] &= ~BITMAP_FIRST_WORD_MASK(start); > > > map[index + 0] |= value << offset; > > > map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits); > > > map[index + 1] |= value >> space; > > > } > > > } > > > > > > Or is this going to produce the same warning because we're not using an > > > explicit check against the map array size? > > > > https://godbolt.org/z/fxnsG9 > > > > It still warns about the 'map[index + 1]' access: from all I can tell, > > gcc mainly complains because it cannot rule out that 'space < nbits', > > and then it knows the size of 'DECLARE_BITMAP(old, 64)' and finds > > that if 'index + 0' is correct, then 'index + 1' overflows that array. > > > > Arnd > > Hi Arnd, > > As suggested by William, sharing another solution to suppress the > compiler warning. Please let me know your views on the below fix. Thanks. > > If its alright, I shall submit a (new) v13 patchset soon. Let me know. > > @@ -1,5 +1,5 @@ > static inline void bitmap_set_value(unsigned long *map, > - unsigned long value, > + unsigned long value, const size_t length, > unsigned long start, unsigned long nbits) > { > const size_t index = BIT_WORD(start); > @@ -15,6 +15,10 @@ static inline void bitmap_set_value(unsigned long *map, > } else { > map[index + 0] &= ~BITMAP_FIRST_WORD_MASK(start); > map[index + 0] |= value << offset; > + > + if (index + 1 >= length) > + __builtin_unreachable(); > + > map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits); > map[index + 1] |= value >> space; > } Hi Syed, Let's rename 'length' to 'nbits' as Arnd suggested, and rename 'nbits' to value_width. William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature