| Message ID | 20220428205116.861003-3-yury.norov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bitmap: fix conversion from/to fix-sized arrays | expand |
On Thu, Apr 28, 2022 at 01:51:13PM -0700, Yury Norov wrote: > Manipulating 64-bit arrays with bitmap functions is potentially dangerous > because on 32-bit BE machines the order of halfwords doesn't match. > Another issue is that compiler may throw a warning about out-of-boundary > access. > > This patch adds bitmap_{from,to}_arr64 functions in addition to existing > bitmap_{from,to}_arr32. ... > + bitmap_copy_clear_tail((unsigned long *) (bitmap), \ > + (const unsigned long *) (buf), (nbits)) Drop spaces after castings. Besides that it might be placed on a single line. ... > + bitmap_copy_clear_tail((unsigned long *) (buf), \ > + (const unsigned long *) (bitmap), (nbits)) Ditto. ... > +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits) > +{ > + const unsigned long *end = bitmap + BITS_TO_LONGS(nbits); > + > + while (bitmap < end) { > + *buf = *bitmap++; > + if (bitmap < end) > + *buf |= (u64)(*bitmap++) << 32; > + buf++; > + } > > + /* Clear tail bits in last element of array beyond nbits. */ > + if (nbits % 64) > + buf[-1] &= GENMASK_ULL(nbits, 0); Hmm... if nbits is > 0 and < 64, wouldn't be this problematic, since end == bitmap? Or did I miss something? > +}
On Fri, Apr 29, 2022 at 03:59:25PM +0300, Andy Shevchenko wrote: > On Thu, Apr 28, 2022 at 01:51:13PM -0700, Yury Norov wrote: > > Manipulating 64-bit arrays with bitmap functions is potentially dangerous > > because on 32-bit BE machines the order of halfwords doesn't match. > > Another issue is that compiler may throw a warning about out-of-boundary > > access. > > > > This patch adds bitmap_{from,to}_arr64 functions in addition to existing > > bitmap_{from,to}_arr32. > > ... > > > + bitmap_copy_clear_tail((unsigned long *) (bitmap), \ > > + (const unsigned long *) (buf), (nbits)) > > Drop spaces after castings. Besides that it might be placed on a single line. > > ... OK > > > + bitmap_copy_clear_tail((unsigned long *) (buf), \ > > + (const unsigned long *) (bitmap), (nbits)) > > Ditto. > > ... > > > +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits) > > +{ > > + const unsigned long *end = bitmap + BITS_TO_LONGS(nbits); > > + > > + while (bitmap < end) { > > + *buf = *bitmap++; > > + if (bitmap < end) > > + *buf |= (u64)(*bitmap++) << 32; > > + buf++; > > + } > > > > + /* Clear tail bits in last element of array beyond nbits. */ > > + if (nbits % 64) > > + buf[-1] &= GENMASK_ULL(nbits, 0); > > Hmm... if nbits is > 0 and < 64, wouldn't be this problematic, since > end == bitmap? Or did I miss something? BITS_TO_LONGS(0) == 0 BITS_TO_LONGS(1..32) == 1 BITS_TO_LONGS(33..64) == 2 The only potential problem with buf[-1] is nbits == 0, but fortunately (0 % 64) == 0, and it doesn't happen. Thanks, Yury > > +} > > -- > With Best Regards, > Andy Shevchenko >
On Fri, Apr 29, 2022 at 08:45:35AM -0700, Yury Norov wrote: > On Fri, Apr 29, 2022 at 03:59:25PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 28, 2022 at 01:51:13PM -0700, Yury Norov wrote: > > > Manipulating 64-bit arrays with bitmap functions is potentially dangerous > > > because on 32-bit BE machines the order of halfwords doesn't match. > > > Another issue is that compiler may throw a warning about out-of-boundary > > > access. > > > > > > This patch adds bitmap_{from,to}_arr64 functions in addition to existing > > > bitmap_{from,to}_arr32. > > > > ... > > > > > + bitmap_copy_clear_tail((unsigned long *) (bitmap), \ > > > + (const unsigned long *) (buf), (nbits)) > > > > Drop spaces after castings. Besides that it might be placed on a single line. > > > > ... > > OK > > > > > > + bitmap_copy_clear_tail((unsigned long *) (buf), \ > > > + (const unsigned long *) (bitmap), (nbits)) > > > > Ditto. > > > > ... > > > > > +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits) > > > +{ > > > + const unsigned long *end = bitmap + BITS_TO_LONGS(nbits); > > > + > > > + while (bitmap < end) { > > > + *buf = *bitmap++; > > > + if (bitmap < end) > > > + *buf |= (u64)(*bitmap++) << 32; > > > + buf++; > > > + } > > > > > > + /* Clear tail bits in last element of array beyond nbits. */ > > > + if (nbits % 64) > > > + buf[-1] &= GENMASK_ULL(nbits, 0); > > > > Hmm... if nbits is > 0 and < 64, wouldn't be this problematic, since > > end == bitmap? Or did I miss something? > > BITS_TO_LONGS(0) == 0 > BITS_TO_LONGS(1..32) == 1 > BITS_TO_LONGS(33..64) == 2 > > The only potential problem with buf[-1] is nbits == 0, but fortunately > (0 % 64) == 0, and it doesn't happen. > > Thanks, > Yury Are there any other concerns? If no, I'll fix formatting and append it to bitmap-for-next. Thanks, Yury
On Mon, May 02, 2022 at 01:06:56PM -0700, Yury Norov wrote: > On Fri, Apr 29, 2022 at 08:45:35AM -0700, Yury Norov wrote: > > On Fri, Apr 29, 2022 at 03:59:25PM +0300, Andy Shevchenko wrote: > > > On Thu, Apr 28, 2022 at 01:51:13PM -0700, Yury Norov wrote: ... > > > > +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits) > > > > +{ > > > > + const unsigned long *end = bitmap + BITS_TO_LONGS(nbits); > > > > + > > > > + while (bitmap < end) { > > > > + *buf = *bitmap++; > > > > + if (bitmap < end) > > > > + *buf |= (u64)(*bitmap++) << 32; > > > > + buf++; > > > > + } > > > > > > > > + /* Clear tail bits in last element of array beyond nbits. */ in the last > > > > + if (nbits % 64) > > > > + buf[-1] &= GENMASK_ULL(nbits, 0); > > > > > > Hmm... if nbits is > 0 and < 64, wouldn't be this problematic, since > > > end == bitmap? Or did I miss something? > > > > BITS_TO_LONGS(0) == 0 > > BITS_TO_LONGS(1..32) == 1 > > BITS_TO_LONGS(33..64) == 2 > > > > The only potential problem with buf[-1] is nbits == 0, but fortunately > > (0 % 64) == 0, and it doesn't happen. I see, perhaps adding a small comment would be nice to have to explain that -1 index is safe. > Are there any other concerns? If no, I'll fix formatting and append it to > bitmap-for-next. Nope.
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h index dbdf1685debf..57f1c74239d5 100644 --- a/include/linux/bitmap.h +++ b/include/linux/bitmap.h @@ -292,6 +292,24 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, (const unsigned long *) (bitmap), (nbits)) #endif +/* + * On 64-bit systems bitmaps are represented as u64 arrays internally. On LE32 + * machines the order of hi and lo parts of numbers match the bitmap structure. + * In both cases conversion is not needed when copying data from/to arrays of + * u64. + */ +#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN) +void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits); +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits); +#else +#define bitmap_from_arr64(bitmap, buf, nbits) \ + bitmap_copy_clear_tail((unsigned long *) (bitmap), \ + (const unsigned long *) (buf), (nbits)) +#define bitmap_to_arr64(buf, bitmap, nbits) \ + bitmap_copy_clear_tail((unsigned long *) (buf), \ + (const unsigned long *) (bitmap), (nbits)) +#endif + static inline int bitmap_and(unsigned long *dst, const unsigned long *src1, const unsigned long *src2, unsigned int nbits) { @@ -601,10 +619,7 @@ static inline void bitmap_next_set_region(unsigned long *bitmap, */ static inline void bitmap_from_u64(unsigned long *dst, u64 mask) { - dst[0] = mask & ULONG_MAX; - - if (sizeof(mask) > sizeof(unsigned long)) - dst[1] = mask >> 32; + bitmap_from_arr64(dst, &mask, 64); } /** diff --git a/lib/bitmap.c b/lib/bitmap.c index d9a4480af5b9..027b63a655fd 100644 --- a/lib/bitmap.c +++ b/lib/bitmap.c @@ -1533,5 +1533,53 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits) buf[halfwords - 1] &= (u32) (UINT_MAX >> ((-nbits) & 31)); } EXPORT_SYMBOL(bitmap_to_arr32); +#endif + +#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN) +/** + * bitmap_from_arr64 - copy the contents of u64 array of bits to bitmap + * @bitmap: array of unsigned longs, the destination bitmap + * @buf: array of u64 (in host byte order), the source bitmap + * @nbits: number of bits in @bitmap + */ +void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits) +{ + int n; + + for (n = nbits; n > 0; n -= 64) { + u64 val = *buf++; + + *bitmap++ = val; + if (n > 32) + *bitmap++ = val >> 32; + } + + /* Clear tail bits in last word beyond nbits. */ + if (nbits % BITS_PER_LONG) + bitmap[-1] &= BITMAP_LAST_WORD_MASK(nbits); +} +EXPORT_SYMBOL(bitmap_from_arr64); + +/** + * bitmap_to_arr64 - copy the contents of bitmap to a u64 array of bits + * @buf: array of u64 (in host byte order), the dest bitmap + * @bitmap: array of unsigned longs, the source bitmap + * @nbits: number of bits in @bitmap + */ +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits) +{ + const unsigned long *end = bitmap + BITS_TO_LONGS(nbits); + + while (bitmap < end) { + *buf = *bitmap++; + if (bitmap < end) + *buf |= (u64)(*bitmap++) << 32; + buf++; + } + /* Clear tail bits in last element of array beyond nbits. */ + if (nbits % 64) + buf[-1] &= GENMASK_ULL(nbits, 0); +} +EXPORT_SYMBOL(bitmap_to_arr64); #endif
Manipulating 64-bit arrays with bitmap functions is potentially dangerous because on 32-bit BE machines the order of halfwords doesn't match. Another issue is that compiler may throw a warning about out-of-boundary access. This patch adds bitmap_{from,to}_arr64 functions in addition to existing bitmap_{from,to}_arr32. Signed-off-by: Yury Norov <yury.norov@gmail.com> --- include/linux/bitmap.h | 23 ++++++++++++++++---- lib/bitmap.c | 48 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-)