[PATCH] Fix order_base_2(0)

[PATCH] Fix order_base_2(0)

[David Howells]

The order_base_2() function is either wrongly documented or wrongly
implemented.  In the preceding comment, it says that:

	ob2(0) = 0

but this is not valid as roundup_pow_of_two()'s documentation asserts that:

	* - the result is undefined when n == 0

Fix order_base_2(n) to actually return 0 when n == 0.

However, should we just instead define that the result of order_base_2(0) is
undefined?

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Robert P. J. Day <rpjday@crashcourse.ca>
cc: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/log2.h |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/include/linux/log2.h b/include/linux/log2.h
index fd7ff3d..d6463d8 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -202,7 +202,6 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
  *  ob2(5) = 3
  *  ... and so on.
  */
-
-#define order_base_2(n) ilog2(roundup_pow_of_two(n))
+#define order_base_2(n) ((n) == 0 ? 0 : ilog2(roundup_pow_of_two(n)))
 
 #endif /* _LINUX_LOG2_H */

Re: [PATCH] Fix order_base_2(0)

[Linus Torvalds]

On Wed, Dec 14, 2011 at 3:40 AM, David Howells <dhowells@redhat.com> wrote:
> The order_base_2() function is either wrongly documented or wrongly
> implemented.  In the preceding comment, it says that:
>
>        ob2(0) = 0

Let's just remove that comment. That's just crazy math and makes no
sense. Why would anybody do ilog2() on zero and expect anything valid?

At least "-1" would make a tiny amount as sense as an error return or
"underflow" or whatever. But returning 0 is just wrong. That's
ilog2(1), not 0.

Does anybody actually *want* order_base_2(0)?

                      Linus

Re: [PATCH] Fix order_base_2(0)

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Does anybody actually *want* order_base_2(0)?

There aren't actually that many users:

| arch/mips/include/asm/mach-powertv/ioremap.h:#define _IOR_OFFSET_WIDTH(n)    (1 << order_base_2(n))

Doesn't look like it should ever be 0.  In fact, this looks like a case for
using roundup_pow_of_two() directly.

| arch/powerpc/platforms/pseries/iommu.c:       len = order_base_2(max_addr);

Not sure.  Doesn't look likely, but can memory_hotplug_max() be 0 if hotplug
is not supported?

| arch/x86/kvm/x86.c:   return hash_32(gfn & 0xffffffff, order_base_2(ASYNC_PF_PER_VCPU));

Constant 64.  Can't be 0.

| fs/ext4/indirect.c:   blk_bits = order_base_2(lblock);

lblock *could* perhaps be 0.  Being negative is checked for, but not for it
being 0.

| fs/ext4/mballoc.c:    int blocksize_bits = order_base_2(size);
| fs/jbd2/journal.c:    int i = order_base_2(size) - 10;

Can't be 0.

| fs/jbd2/journal.c:    int i = order_base_2(size) - 10;

Hmmm...  Can jbd2_alloc() be given a 0 size?  (Or jbd2_free() for that matter)

| mm/percpu-km.c:       pages = alloc_pages(GFP_KERNEL, order_base_2(nr_pages));
| mm/percpu-km.c:               __free_pages(chunk->data, order_base_2(nr_pages));

Can pcpu_group_sizes[0] be less than PAGE_SIZE?

I can't tell from a cursory inspection of pcpu_setup_first_chunk().

In fact, pcpu_create_chunk() could do order_base_2() then subtract PAGE_SHIFT
rather than shifting and then taking the log.

David

Re: [PATCH] Fix order_base_2(0)

[Milton Miller]

David Howells wrote:
> Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > Does anybody actually *want* order_base_2(0)?
> 
> There aren't actually that many users:
..
 
> | arch/powerpc/platforms/pseries/iommu.c: len = order_base_2(max_addr);
> 
> Not sure. Doesn't look likely, but can memory_hotplug_max() be 0 if hotplug
> is not supported?

arch/powerpc/include/asm/mmzone.h has the declaration or #define, and
arch/powerpc/mm/numa.c has the definiton when not defined.   All have
memblock_end_of_DRAM() as the minimum value, so it will never be 0.

(The usage is to get the number of bits needed to cover all possible
memory that could be allocated to the OS to size the iommu table).

milton