Re: [PATCH 05/32] locking/lockdep: Prepare valid_state() to handle plain masks

[Frederic Weisbecker]

On Tue, Feb 12, 2019 at 09:45:52AM -0800, Linus Torvalds wrote:
> On Tue, Feb 12, 2019 at 9:14 AM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
> >
> > +
> > +       while (vectors) {
> > +               long fs = __ffs64(vectors) + 1;
> > +
> > +               vectors >>= fs;
> 
> This is wrong.
> 
> If "vectors" only has the high hit set, you end up with "fs" having
> the value "64".
> 
> And then "vectors >>= fs" is undefined and won't actually do anything
> at all on x86.

Oh! ok didn't know that...

> 
> In general, avoid "ffs()", and the stupid pattern of "__ffs(x)+1".
> 
> Bit numbering starts at 0. "ffs()" is wrong. And you never *ever* just
> add one to a bit number in order to shift by one more bit, exactly
> because of overflow issues.
> 
> So it may look inefficient, but the correct thing to do is
> 
>     long bit = __ffs64(vectors);
>     vectors >>= fs;
>     vectors >>= 1;
> 
> because that actually works.

I see, perhaps I should use for_each_set_bit() that should take care about those
details?

Thanks.