Linus Torvalds <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
