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

Frederic Weisbecker
2019-02-13T15:16:54Z

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@kernel.org> 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.