On 08/07/2018 03:03 PM, Wei Wang wrote:
On 08/07/2018 07:30 AM, Rasmus Villemoes wrote:
On 2018-07-26 12:15, Wei Wang wrote:
On 07/26/2018 05:37 PM, Yury Norov wrote:
On Thu, Jul 26, 2018 at 04:07:51PM +0800, Wei Wang wrote:
The existing BITMAP_LAST_WORD_MASK macro returns 0xffffffff if
nbits is
0. This patch changes the macro to return 0 when there is no bit
needs to
be masked.
I think this is intentional behavour. Previous version did return ~0UL
explicitly in this case. See patch 89c1e79eb3023 (linux/bitmap.h:
improve
BITMAP_{LAST,FIRST}_WORD_MASK) from Rasmus.
Yes, I saw that. But it seems confusing for the corner case that
nbits=0
(no bits to mask), the macro returns with all the bits set.
Introducing conditional branch would affect performance. All existing
code checks nbits for 0 before handling last word where needed
explicitly. So I think we'd better change nothing here.
I think that didn't save the conditional branch essentially, because
it's just moved from inside this macro to the caller as you mentioned.
If callers missed the check for some reason and passed 0 to the macro,
they will get something unexpected.
Current callers like __bitmap_weight, __bitmap_equal, and others,
they have
if (bits % BITS_PER_LONG)
w += hweight_long(bitmap[k] & BITMAP_LAST_WORD_MASK(bits));
we could remove the "if" check by "w += hweight_long(bitmap[k] &
BITMAP_LAST_WORD_MASK(bits % BITS_PER_LONG));" the branch is the same.
Absolutely not! That would access bitmap[lim] (the final value of the k
variable) despite that word not being part of the bitmap.
Probably it's more clear to post the entire function here for a
discussion:
int __bitmap_weight(const unsigned long *bitmap, unsigned int bits)
{
unsigned int k, lim = bits/BITS_PER_LONG;
int w = 0;
for (k = 0; k < lim; k++)
w += hweight_long(bitmap[k]);
if (bits % BITS_PER_LONG)
==> w += hweight_long(bitmap[k] &
BITMAP_LAST_WORD_MASK(bits));
return w;
}
When the execution reaches "==>", isn't "k=lim"?
And accessing to bitmap[lim] which does not exist should be a case
considered by the caller rather than the macro. For example, with
"BITMAP_LAST_WORD_MASK(bits) & bitmap[k]", making
BITMAP_LAST_WORD_MASK(0) be 0 will not be a problem.
Anyway, my point is that we could make the macro itself robust.
Best,
Wei
