Messages in this thread | | | Date | Wed, 13 Feb 2019 16:16:49 +0100 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 05/32] locking/lockdep: Prepare valid_state() to handle plain masks |
| |
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.
| |