On Fri, Oct 16, 2020 at 04:23:05AM +0530, Syed Nayyar Waris wrote: > On Tue, Oct 6, 2020 at 4:56 PM Andy Shevchenko > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > On Tue, Oct 06, 2020 at 02:52:16PM +0530, Syed Nayyar Waris wrote: ... > > > + return (map[index] >> offset) & GENMASK(nbits - 1, 0); > > > > Have you considered to use rather BIT{_ULL}(nbits) - 1? > > It maybe better for code generation. > > Yes I have considered using BIT{_ULL} in earlier versions of patchset. > It has a problem: > > This macro when used in both bitmap_get_value and > bitmap_set_value functions, it will give unexpected results when nbits or clump > size is BITS_PER_LONG (32 or 64 depending on arch). > > Actually when nbits (clump size) is 64 (BITS_PER_LONG is 64, for example), > (BIT(nbits) - 1) > gives a value of zero and when this zero is ANDed with any value, it > makes it full zero. This is unexpected, and incorrect calculation occurs. > > What actually happens is in the macro expansion of BIT(64), that is 1 > << 64, the '1' overflows from leftmost bit position (most significant > bit) and re-enters at the rightmost bit position (least significant > bit), therefore 1 << 64 becomes '0x1', and when another '1' is > subtracted from this, the final result becomes 0. > > This is undefined behavior in the C standard (section 6.5.7 in the N1124) I see, indeed, for 64/32 it is like this. ... > Yes I have incorporated your suggestion to use the '<<' operator. Thank You. One side note, consider the use round_up() vs. roundup(). I don't remember which one is optimized to divisor being power of 2. -- With Best Regards, Andy Shevchenko