From: Crt Mori <cmo@melexis.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Florian La Roche <florian.laroche@googlemail.com>,
Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
Joe Perches <joe@perches.com>,
Davidlohr Bueso <dave@stgolabs.net>,
Will Deacon <will.deacon@arm.com>,
Peter Zijlstra <peterz@infradead.org>
Subject: Re: fix int_sqrt() for very large numbers
Date: Mon, 21 Jan 2019 21:25:00 +0100 [thread overview]
Message-ID: <CAKv63uu+-gF_zci8NrRDB4g7ujkbz7dPnAEAjY6_ZVXB9NOWGA@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=wjruxUdKWjn-PvZMOPn3pGO5yTcbc_J4hc4TrS0P15vaw@mail.gmail.com>
On Mon, 21 Jan 2019 at 01:20, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, Jan 20, 2019 at 4:15 AM Florian La Roche
> <florian.laroche@googlemail.com> wrote:
> >
> > @@ -52,7 +52,7 @@ u32 int_sqrt64(u64 x)
> > if (x <= ULONG_MAX)
> > return int_sqrt((unsigned long) x);
> >
> > - m = 1ULL << (fls64(x) & ~1ULL);
> > + m = 1ULL << ((fls64(x) - 1) & ~1ULL);
>
> I've applied this part of the patch as commit fbfaf851902c ("fix
> int_sqrt64() for very large numbers") with slightly edited commit
> log.
>
Thanks for the patch - I its indeed my copy-paste error, because
__fls64 does not exist on 32bit CPU, but the fls64 is not "equal"
replacement. I am very sorry for the bug.
> I still think there are some oddities in here in the types. I
> mentioned the caller that unnecessarily does the int_sqrt64() twice,
> even though the outer one doesn't actually take a 64-bit value.
>
True. This is oddity is originating from time where mlx90632 used its
own function for int_sqrt64 on 32bit.
> But in the very line above, there's another type oddity: the "& ~1ULL"
> is entirely the wrong type. The shift *count* shouldn't be an unsigned
> long long, so that type doesn't make much sense. It should be just a
> ~1, or even just "62".
>
This was also inline with above copy-paste and variable expansion to
force the 64bit everywhere. I will prepare a patch to clean this line
to ~1, question is why does the int_sqrt is having UL if this should
just be "62". I was thinking because we want to cast to the type
before we shift.
> But I didn't actually start micro-editing the patch, and just did that
> one-liner off-by-one fix.
>
> Linus
next prev parent reply other threads:[~2019-01-21 20:25 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-19 15:14 fix int_sqrt() for very large numbers Florian La Roche
2019-01-20 0:01 ` Will Deacon
2019-01-20 3:48 ` Linus Torvalds
2019-01-20 5:03 ` Florian La Roche
2019-01-20 5:11 ` Linus Torvalds
2019-01-20 8:31 ` Crt Mori
2019-01-20 9:29 ` Crt Mori
2019-01-20 10:00 ` Linus Torvalds
2019-01-21 0:20 ` Linus Torvalds
2019-01-21 20:25 ` Crt Mori [this message]
2019-02-02 6:11 ` [LKP] 32bd07585d: kernel_selftests.lib.prime_numbers.sh.fail kernel test robot
2019-02-02 13:10 ` Florian La Roche
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKv63uu+-gF_zci8NrRDB4g7ujkbz7dPnAEAjY6_ZVXB9NOWGA@mail.gmail.com \
--to=cmo@melexis.com \
--cc=dave@stgolabs.net \
--cc=florian.laroche@googlemail.com \
--cc=joe@perches.com \
--cc=linux-kernel@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=torvalds@linux-foundation.org \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).