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

[Wei Wang <wei.w.wang@intel.com>]

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
> this.
>
> 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.
e.g.
Line 225 at https://github.com/qemu/qemu/blob/master/include/qemu/bitmap.h
(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.


Best,
Wei