[PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Dmitry Torokhov]

From: Andrei Warkentin <andreiw@vmware.com>

1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.
It does in case the argument is a variable but in case it's a constant
it behaves incorrectly and returns 0. Probably nobody ever did it so
this was never noticed, however drivers/net/vmxnet3 with latest GCC does
and breaks on unicpu systems.

This is similar to Rolf's patch to roundup_pow_of_two(1).

Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
Reviewed-by: Jesper Juhl <jj@chaosbits.net>
Reviewed-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
Cc: stable@kernel.org
Signed-off-by: Dmitry Torokhov <dtor@vmware.com>
---
 include/linux/log2.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/log2.h b/include/linux/log2.h
index 25b8086..ccda848 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -185,7 +185,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 #define rounddown_pow_of_two(n)			\
 (						\
 	__builtin_constant_p(n) ? (		\
-		(n == 1) ? 0 :			\
+		(n == 1) ? 1 :			\
 		(1UL << ilog2(n))) :		\
 	__rounddown_pow_of_two(n)		\
  )
-- 
1.7.4.1

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Linus Torvalds]

On Mon, Dec 12, 2011 at 10:02 AM, Dmitry Torokhov <dtor@vmware.com> wrote:
> From: Andrei Warkentin <andreiw@vmware.com>
>
> 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.
> It does in case the argument is a variable but in case it's a constant
> it behaves incorrectly and returns 0. Probably nobody ever did it so
> this was never noticed, however drivers/net/vmxnet3 with latest GCC does
> and breaks on unicpu systems.
>
> This is similar to Rolf's patch to roundup_pow_of_two(1).

Umm. I already applied this patch, but then I started looking at it
more, and asked myself:

 - Why is that "n == 1" test there AT ALL?

Afaik, that whole test is just plain stupid. It seems to have been
copied from the "roundup()" case (where it exists due to the "-1/+1"
hackery that breaks ilog2()) without any thought about the actual math
of the function at all.

I think the *real* fix is to just remove that incorrect line, no?

It's a bit sad that we apparently have several reviewers for this
trivial patch, and nobody reacted to the math just not making any
sense.

                                      Linus

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Dmitry Torokhov]

On Mon, Dec 12, 2011 at 03:50:11PM -0800, Linus Torvalds wrote:
> On Mon, Dec 12, 2011 at 10:02 AM, Dmitry Torokhov <dtor@vmware.com> wrote:
> > From: Andrei Warkentin <andreiw@vmware.com>
> >
> > 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.
> > It does in case the argument is a variable but in case it's a constant
> > it behaves incorrectly and returns 0. Probably nobody ever did it so
> > this was never noticed, however drivers/net/vmxnet3 with latest GCC does
> > and breaks on unicpu systems.
> >
> > This is similar to Rolf's patch to roundup_pow_of_two(1).
> 
> Umm. I already applied this patch, but then I started looking at it
> more, and asked myself:
> 
>  - Why is that "n == 1" test there AT ALL?
> 
> Afaik, that whole test is just plain stupid. It seems to have been
> copied from the "roundup()" case (where it exists due to the "-1/+1"
> hackery that breaks ilog2()) without any thought about the actual math
> of the function at all.
> 
> I think the *real* fix is to just remove that incorrect line, no?

Yes, you are right, special-casing for 1 is not necessary in rounddown
case.

Thanks,
Dmitry

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Peter Zijlstra]

On Mon, 2011-12-12 at 10:02 -0800, Dmitry Torokhov wrote:
> 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.

x^0 = 1
x^1 = x

Seems to me 0 was the right answer, no?

Re: [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Peter Zijlstra]

On Tue, 2011-12-13 at 21:00 +0100, Peter Zijlstra wrote:
> On Mon, 2011-12-12 at 10:02 -0800, Dmitry Torokhov wrote:
> > 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.
> 
> x^0 = 1
> x^1 = x
> 
> Seems to me 0 was the right answer, no?

n/m head-up-arse, missed that this wasn't actually a log variant.

RE: [Pv-drivers] [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Bhavesh Davda]

> On Mon, 2011-12-12 at 10:02 -0800, Dmitry Torokhov wrote:
> > 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.
> 
> x^0 = 1
> x^1 = x
> 
> Seems to me 0 was the right answer, no?

Eh? The answer should be the power-of-two (1), not the exponent (0).

RE: [Pv-drivers] [PATCH] include/log2.h: Fix rounddown_pow_of_two(1)

[Peter Zijlstra]

On Tue, 2011-12-13 at 12:06 -0800, Bhavesh Davda wrote:
> > On Mon, 2011-12-12 at 10:02 -0800, Dmitry Torokhov wrote:
> > > 1 is a power of two, therefore rounddown_pow_of_two(1) should return 1.
> > 
> > x^0 = 1
> > x^1 = x
> > 
> > Seems to me 0 was the right answer, no?
> 
> Eh? The answer should be the power-of-two (1), not the exponent (0).

Yeah, just noticed that, see my other reply.