Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 20 Jan 2019 00:01:40 +0000
From:   Will Deacon <will.deacon@arm.com>
To:     Florian La Roche <florian.laroche@googlemail.com>
Cc:     linux-kernel@vger.kernel.org, Crt Mori <cmo@melexis.com>,
        Joe Perches <joe@perches.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Peter Zijlstra <peterz@infradead.org>,
        Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: fix int_sqrt() for very large numbers
Message-ID: <20190120000138.GI26876@brain-police>
References: <20190119151450.26879-1-Florian.LaRoche@googlemail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190119151450.26879-1-Florian.LaRoche@googlemail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Jan 19, 2019 at 04:14:50PM +0100, Florian La Roche wrote:
> If an input number x for int_sqrt() has the highest bit set, then
> __ffs(x) is 64. (1UL << 64) is an overflow and breaks the algorithm.

This is confusing, because the patch doesn't go near an __ffs().

> Just subtracting 1 is an even better guess for the initial
> value of m and that's what also used to be done in earlier
> versions of this code.
> 
> best regards,
> 
> Florian La Roche
> 
> Signed-off-by: Florian La Roche <Florian.LaRoche@googlemail.com>
> ---
>  lib/int_sqrt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
> index 14436f4ca6bd..ea00e84dc272 100644
> --- a/lib/int_sqrt.c
> +++ b/lib/int_sqrt.c
> @@ -23,7 +23,7 @@ unsigned long int_sqrt(unsigned long x)
>  	if (x <= 1)
>  		return x;
>  
> -	m = 1UL << (__fls(x) & ~1UL);
> +	m = 1UL << ((__fls(x) - 1) & ~1UL);

I think this one is fine, because __fls() gives you back 0-63 (or
undefined, but the previous <= 1 check handles that case).

>  	while (m != 0) {
>  		b = y + m;
>  		y >>= 1;
> @@ -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);

This just looks like a copy-paste error because there isn't an __fls64().
But I think your suggestion here is ok, given the previous check against
ULONG_MAX.

Will