lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y08rqtdiTDbIm0EJ@yury-laptop> Date: Tue, 18 Oct 2022 15:41:46 -0700 From: Yury Norov <yury.norov@...il.com> To: Alexander Lobakin <alexandr.lobakin@...el.com> Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Rasmus Villemoes <linux@...musvillemoes.dk>, Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>, Maciej Fijalkowski <maciej.fijalkowski@...el.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v2 net-next 1/6] bitmap: try to optimize arr32 <-> bitmap on 64-bit LEs On Tue, Oct 18, 2022 at 04:00:22PM +0200, Alexander Lobakin wrote: > Unlike bitmap_{from,to}_arr64(), when there can be no out-of-bounds > accesses (due to u64 always being no shorter than unsigned long), > it can't be guaranteed with arr32s due to that on 64-bit platforms: > > bits BITS_TO_U32 * sizeof(u32) BITS_TO_LONGS * sizeof(long) > 1-32 4 8 > 33-64 8 8 > 95-96 12 16 > 97-128 16 16 > > and so on. > That is why bitmap_{from,to}_arr32() are always defined there as > externs. But quite often @nbits is a compile-time constant, which > means we could suggest whether it can be inlined or not at > compile-time basing on the number of bits (above). > > So, try to determine that at compile time and, in case of both > containers having the same size in bytes, resolve it to > bitmap_copy_clear_tail() on Little Endian. No changes here for > Big Endian or when the number of bits *really* is variable. You're saying 'try to optimize', but don't show any numbers. What's the target for your optimization? Can you demonstrate how it performs in test or in real work? > Signed-off-by: Alexander Lobakin <alexandr.lobakin@...el.com> > --- > include/linux/bitmap.h | 51 ++++++++++++++++++++++++++++++------------ > lib/bitmap.c | 12 +++++----- > 2 files changed, 43 insertions(+), 20 deletions(-) > > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h > index 7d6d73b78147..79d12e0f748b 100644 > --- a/include/linux/bitmap.h > +++ b/include/linux/bitmap.h > @@ -283,24 +283,47 @@ static inline void bitmap_copy_clear_tail(unsigned long *dst, > * On 32-bit systems bitmaps are represented as u32 arrays internally. On LE64 > * 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 > - * u32. But in LE64 case, typecast in bitmap_copy_clear_tail() may lead > - * to out-of-bound access. To avoid that, both LE and BE variants of 64-bit > - * architectures are not using bitmap_copy_clear_tail(). > + * u32. But in LE64 case, typecast in bitmap_copy_clear_tail() may lead to > + * out-of-bound access. To avoid that, LE variant of 64-bit architectures uses > + * bitmap_copy_clear_tail() only when @bitmap and @buf containers have the same > + * size in memory (known at compile time), and 64-bit BEs never use it. > */ > -#if BITS_PER_LONG == 64 > -void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, > - unsigned int nbits); > -void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, > - unsigned int nbits); > +#if BITS_PER_LONG == 32 > +#define bitmap_arr32_compat(nbits) true > +#elif defined(__LITTLE_ENDIAN) > +#define bitmap_arr32_compat(nbits) \ 'Compat' is reserved for a compatibility layer between kernel and user spaces running different ABIs. Can you pick some other word? > + (__builtin_constant_p(nbits) && \ > + BITS_TO_U32(nbits) * sizeof(u32) == \ > + BITS_TO_LONGS(nbits) * sizeof(long)) I think it should be: round_up(nbits, 32) == round_up(nbits, 64) > #else > -#define bitmap_from_arr32(bitmap, buf, nbits) \ > - bitmap_copy_clear_tail((unsigned long *) (bitmap), \ > - (const unsigned long *) (buf), (nbits)) > -#define bitmap_to_arr32(buf, bitmap, nbits) \ > - bitmap_copy_clear_tail((unsigned long *) (buf), \ > - (const unsigned long *) (bitmap), (nbits)) Can you keep this part untouched? I'd like to have a clear meaning - on 32-bit arch, bitmap_to_arr32 is a simple copy. > +#define bitmap_arr32_compat(nbits) false > #endif > > +void __bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned int nbits); > +void __bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits); > + > +static inline void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, > + unsigned int nbits) > +{ > + const unsigned long *src = (const unsigned long *)buf; > + > + if (bitmap_arr32_compat(nbits)) > + bitmap_copy_clear_tail(bitmap, src, nbits); > + else > + __bitmap_from_arr32(bitmap, buf, nbits); If you would really want to optimize it, I'd suggest something like this: #ifdef __LITTLE_ENDIAN /*copy as many full 64-bit words as we can */ bitmap_copy(bitmap, src, round_down(nbits, BITS_PER_LONG)); /* now copy part of last word per-byte */ ... #else __bitmap_from_arr32(bitmap, buf, nbits); #endif This should be better because it uses fast bitmap_copy() regardless the number of bits. Assuming bitmap_copy() is significantly faster than bitmap_from_arr(), people will be surprised by the difference of speed of copying, say, 2048 and 2049-bit bitmaps. Right? But unless we'll see real numbers, it's questionable to me if that's worth the effort. Thanks, Yury
Powered by blists - more mailing lists