Re: [PATCH] linux/bitmap.h: fix BITMAP_LAST_WORD_MASK

From: Wei Wang
Date: Tue Aug 07 2018 - 07:18:36 EST

On 08/07/2018 06:26 PM, Andy Shevchenko wrote:
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"?
Let's check again, if bits == 0, bits % whatever == 0 as well, thus,
no execution. When bits < BITS_PER_LONG, the execution is fine (k = 0
and still 0).
When bits >= BITS_PER_LONG, but not aligned, k goes from 0 to lim and
last word is exactly the partially filled one. No problem here. Las
case if bits % BITS_PER_LONG == 0, but hey, we have a guard against

So, where is the problem exactly?

There is no problem here. All the discussions here are about if it is better to
1) put this guard to the macro or 2) have each callers to do it.

If we go with 2) as we discussed before, then do we need to add notes above the macro to people who will use/port this macro.

OTOH, for nbits=0, there _is_ no last word (since there are no words at
all), so by the time you want to apply the result of
BITMAP_LAST_WORD_MASK(0) to anything, you already have a bug, probably
either having read or being about to write into bitmap[0], which you
cannot do. Please check that user-space port and see if there are bugs
of that kind.
Yes, some callers there don't check for nbits=0, that's why I think it is
better to offload that check to the macro. The macro itself can be robust to
handle all the cases.
Where they are? Can't you point out?

"there", I meant that user-space port, not in the kernel.
Line 225 at
(there are a couple of other places)

nbits=64, means all the 64 bits need to mask

The two are different cases, I'm not sure why we let the macro to return the
same value.
The point is macro mustn't be called when nbits==0.

Yes, I fully agree with that point, but it seems Rasmus NAK-ed that point.

To conclude the point of the discussion: with the current macro, there is no doc saying 0 can't be given to this macro and "BITMAP_LAST_WORD_MASK(0)=0xffffffff" doesn't seem correct to me.