| Message ID | 20230717113709.328671-2-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement MTE tag compression for swapped pages | expand |
On Mon, Jul 17, 2023 at 05:31:44PM +0300, Andy Shevchenko wrote: > On Mon, Jul 17, 2023 at 05:29:12PM +0300, Andy Shevchenko wrote: > > On Mon, Jul 17, 2023 at 04:14:57PM +0200, Alexander Potapenko wrote: > > ... > > > > > > + map[index] &= ~(GENMASK(nbits + offset - 1, offset)); > > > > > > > > I remember that this construction may bring horrible code on some architectures > > > > with some version(s) of the compiler (*). > > > > > > Wow, even the trunk Clang and GCC seem to generate better code for > > > your version of this line: https://godbolt.org/z/36Kqxhe6j > > > > Wow, indeed! Perhaps time to report to clang and GCC people. I believe the root > > cause is that in the original version compiler can't prove that l is constant > > for GENMASK(). > > > > > > To fix that I found an easy refactoring: > > > > > > > > map[index] &= ~(GENMASK(nbits, 0) << offset)); > > nbits - 1 it should be, btw. In any case it seems the code is still better. Yep. Alexander, for the next round can you please show disassembly for the functions in case of compile-time and run-time defined start and nbits? Thanks, Yury
On Mon, Jul 17, 2023 at 5:03 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Jul 17, 2023 at 04:53:51PM +0200, Alexander Potapenko wrote: > > ... > > > > > I remember that this construction may bring horrible code on some architectures > > > > with some version(s) of the compiler (*). > > > > > > Wow, even the trunk Clang and GCC seem to generate better code for > > > your version of this line: https://godbolt.org/z/36Kqxhe6j > > > > Oh wait. > > First, my Godbolt reproducer is incorrect, it is using sizeof(unsigned > > long) instead of 8 * sizeof(unsigned long) for BITS_PER_LONG. > > Still slightly better. And note, that the same GENMASK() is used at the > beginning of the function. Compiler actually might cache it. I think that the compiler might consider this transformation invalid because "GENMASK(nbits - 1, 0) << offset" and "GENMASK(nbits + offset - 1, offset)" have different values in the case "nbits + offset" exceed 64. Restricting the domain of bitmap_set_value() might result in better code, but it is easier to stick to your version, which prevents the overflow. > > > > To fix that I found an easy refactoring: > > > > > > > > map[index] &= ~(GENMASK(nbits, 0) << offset)); > > > > > > > > Second, the line above should probably be: > > map[index] &= ~(GENMASK(nbits - 1, 0) << offset)); > > > > , right? > > Yes. This "nbits vs. nbits - 1" was not detected by test_set_get_value(), so I'll add an extra test case for it.
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h index 03644237e1efb..4559366084988 100644 --- a/include/linux/bitmap.h +++ b/include/linux/bitmap.h @@ -76,7 +76,11 @@ struct device; * bitmap_to_arr32(buf, src, nbits) Copy nbits from buf to u32[] dst * bitmap_to_arr64(buf, src, nbits) Copy nbits from buf to u64[] dst * bitmap_get_value8(map, start) Get 8bit value from map at start + * bitmap_get_value(map, start, nbits) Get bit value of size 'nbits' + * from map at start * bitmap_set_value8(map, value, start) Set 8bit value to map at start + * bitmap_set_value(map, value, start, nbits) Set bit value of size 'nbits' + * of map at start * * Note, bitmap_zero() and bitmap_fill() operate over the region of * unsigned longs, that is, bits behind bitmap till the unsigned long @@ -583,6 +587,31 @@ static inline unsigned long bitmap_get_value8(const unsigned long *map, return (map[index] >> offset) & 0xFF; } +/** + * bitmap_get_value - get a value of n-bits from the memory region + * @map: address to the bitmap memory region + * @start: bit offset of the n-bit value + * @nbits: size of value in bits + * + * Returns value of nbits located at the @start bit offset within the @map + * memory region. + */ +static inline unsigned long bitmap_get_value(const unsigned long *map, + unsigned long start, + unsigned long nbits) +{ + const size_t index = BIT_WORD(start); + const unsigned long offset = start % BITS_PER_LONG; + const unsigned long space = BITS_PER_LONG - offset; + unsigned long value_low, value_high; + + if (space >= nbits) + return (map[index] >> offset) & GENMASK(nbits - 1, 0); + value_low = map[index] & BITMAP_FIRST_WORD_MASK(start); + value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits); + return (value_low >> offset) | (value_high << space); +} + /** * bitmap_set_value8 - set an 8-bit value within a memory region * @map: address to the bitmap memory region @@ -599,6 +628,34 @@ static inline void bitmap_set_value8(unsigned long *map, unsigned long value, map[index] |= value << offset; } +/** + * bitmap_set_value - set n-bit value within a memory region + * @map: address to the bitmap memory region + * @value: value of nbits + * @start: bit offset of the n-bit value + * @nbits: size of value in bits + */ +static inline void bitmap_set_value(unsigned long *map, + unsigned long value, + unsigned long start, unsigned long nbits) +{ + const size_t index = BIT_WORD(start); + const unsigned long offset = start % BITS_PER_LONG; + const unsigned long space = BITS_PER_LONG - offset; + + value &= GENMASK(nbits - 1, 0); + + if (space >= nbits) { + map[index] &= ~(GENMASK(nbits + offset - 1, offset)); + map[index] |= value << offset; + return; + } + map[index] &= ~BITMAP_FIRST_WORD_MASK(start); + map[index] |= value << offset; + map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits); + map[index + 1] |= (value >> space); +} + #endif /* __ASSEMBLY__ */ #endif /* __LINUX_BITMAP_H */