Re: [PATCH] Fix get_order()

[David Howells]

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> > +#define ilog2_up(n) ((n) == 1 ? 0 : ilog2((n) - 1) + 1)
> 
> This is wrong. It uses "n" twice, which makes it unsafe as a macro.

Damn.  I missed that.

> Or it could use a "__builtin_constant_p()" (which gcc defines to not have 
> side effects) to allow the multiple use for constant data.

I should have, yes.

> Or we could require that "ilog2(0)" returns -1, and then we could just say
> 
> 	#define ilog2_up(n) (ilog2((n)-1)+1)

I'd rather not do that as the inline assembly variants then have to special
case ilog2(0) rather than just having an undefined result.

> The whole "get_order()" macro also has some serious lack of parenthesis. 
> In general, commit 39d61db0edb34d60b83c5e0d62d0e906578cc707 just was 
> pretty damn bad!

Unfortunately, I can't disagree.

> I'm becoming a bit disgruntled about this whole thing, I have to admit. 
> I'm just not sure the bugs here are worth it. Especially considering that 
> __get_order() has apparently never even tested these things to begin with, 

It was tested...  I've just re-examined my test program and I've realised I've
only tested power-of-2 parameters.  Sigh.

> since nobody but FRV has ever #defined the ARCH_HAS_ILOG2_U?? macros.

Well, that should be CONFIG_ARCH_HAS_ILOG2_U?? macros, and powerpc defines
those too.

>  - buggy

True, for N being a non-power-of-two, unfortunately; and also where evaluating
N has side-effects.

>  - untested

Not true, just that my userspace test program isn't sufficiently exhaustive.

>  - has untrue comments

Unfortunately so.

>  - makes no real sense

Not true.

Various archs (including i386, x86_64, powerpc and frv) have instructions that
can be used to calculate integer log2(N).  The fallback position is to use a
loop:

	size = (size - 1) >> (PAGE_SHIFT - 1);
	order = -1;
	do {
		size >>= 1;
		order++;
	} while (size);

> and I'm inclined to just revert 39d61db0 instead of adding more and more 
> breakage to it, since it's simply not going to help with the fundamental 
> problems!

Probably a good idea.  I'll work on it some more and improve my test program
(which is actually quite simple to do).

David
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/