<br><tt><font size=2>Roland Dreier <rdreier@cisco.com> wrote on 01/30/2008
03:05:04 PM:<br>
> Looks almost identical to what I came up with (below), except:<br>
> <br>
>  > +       shift = __ffs((unsigned long) ((mask
& ((1ull<<31)-1)) | (1ull<<31)));<br>
> <br>
> I think there's no reason to mask off the top bit of mask if we're<br>
> going to set it immediately;</font></tt>
<br>
<br><tt><font size=2>I was actually masking off the top 33 bits before
setting bit 31.  I agree that it's unnecessary, but I wanted it to
be clear that the possible truncation from 64 to 32 bits (on 32-bit target
machines) is okay.  I'd have left off the masking and the cast if
the kernel provided something like __ffsll() or __ffs64() that would take
64-bit arguments even on 32-bit machines, but it doesn't as far as I can
tell.  You've changed mask itself into an unsigned long:</font></tt>
<br>
<br><tt><font size=2>> -   u64 mask;<br>
> +   unsigned long mask;</font></tt>
<br>
<br><tt><font size=2>so the truncation (on 32-bit machines) is now happening
as the mask is constructed.  In this case the truncation is okay,
because ultimately you only care about the low-order bits anyway, but implicit
truncations always raise a red flag for me.</font></tt>
<br>
<br><tt><font size=2>In any case, I think your patch does the job and does
it efficiently.</font></tt>
<br>
<br><tt><font size=2>- Bryan</font></tt>
<br>
<br>