Date: Thu, 14 Oct 2010 20:49:23 -0700 From: Garrett Cooper <gcooper@FreeBSD.org> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-hackers@freebsd.org, Andriy Gapon <avg@icyb.net.ua> Subject: Re: [PATCH] Bug with powerof2 macro in sys/param.h Message-ID: <AANLkTineQfE12Q4sh6QgKapJGJ-S7UHDfiwRasw5nQVG@mail.gmail.com> In-Reply-To: <201010140937.51953.jhb@freebsd.org> References: <AANLkTi=0PSp2KpFTAHTAoosRYdcewiVAxweH4=ivYTLt@mail.gmail.com> <4CB6F068.1070406@icyb.net.ua> <201010140937.51953.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Oct 14, 2010 at 6:37 AM, John Baldwin <jhb@freebsd.org> wrote:
> On Thursday, October 14, 2010 7:58:32 am Andriy Gapon wrote:
>> on 14/10/2010 00:30 Garrett Cooper said the following:
>> > =A0 =A0 I was talking to someone today about this macro, and he noted =
that
>> > the algorithm is incorrect -- it fails the base case with ((x) =3D=3D =
0 --
>> > which makes sense because 2^(x) cannot equal 0 (mathematically
>> > impossible, unless you consider the limit as x goes to negative
>> > infinity as log (0) / log(2) is undefined). I tested out his claim and
>> > he was right:
>>
>> That's kind of obvious given the code.
>> I think that this might be an intentional optimization.
>> I guess that it doesn't really make sense to apply powerof2 to zero and =
the users
>> of the macro should do the check on their own if they expect zero as inp=
ut (many
>> places in the do not allow that).
But the point is that this could be micro-optimizing things
incorrectly. I'm running simple iteration tests to see what the
performance is like, but the runtime is going to take a while to
produce stable results.
Mathematically there is a conflict with the definition of the macro,
so it might confuse folks who pay attention to the math as opposed to
the details (if you want I'll gladly add a comment around the macro in
a patch to note the caveats of using powerof2).
> I agree, the current macro is this way on purpose (and straight out of
> "Hacker's Delight").
>
> Of the existing calls you weren't sure of:
>
> sys/dev/cxgb/cxgb_sge.c: =A0 =A0 =A0 while (!powerof2(fl_q_size))
> sys/dev/cxgb/cxgb_sge.c: =A0 =A0 =A0 while (!powerof2(jumbo_q_size))
>
> These are fine, will not be zero.
Good to know.
> sys/x86/x86/local_apic.c: =A0 =A0 =A0KASSERT(powerof2(count), ("bad count=
"));
> sys/x86/x86/local_apic.c: =A0 =A0 =A0KASSERT(powerof2(align), ("bad align=
"));
>
> These are fine. =A0No code allocates zero IDT vectors. =A0We never alloca=
te IDT
> vectors for unallocated MSI or MSI-X IRQs.
Excellent.
> sys/net/flowtable.c: =A0 =A0 =A0 =A0 =A0 ft->ft_lock_count =3D
> 2*(powerof2(mp_maxid + 1) ? (mp_maxid + 1):
>
> Clearly, 'mp_maxid + 1' will not be zero (barring a bizarre overflow case
> which will not happen until we support 2^32 CPUs), so this is fine.
But that should be caught by the mp_machdep code, correct?
> sys/i386/pci/pci_pir.c: =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (error &&
> !powerof2(pci_link->pl_irqmask)) {
>
> This fine. =A0Earlier in the function if pl_irqmask is zero, then all of =
the
> pci_pir_choose_irq() calls will fail, so this is only invoked if pl_irqma=
sk
> is non-zero. =A0In practice pl_irqmask is never zero anyway.
Ok.
> I suspect the GEOM ones are also generally safe.
Well, that is the overall feeling I was getting, but I'm not sure that
they're ok 100% of the time.
What about the other patches? The mfiutil and mptutil ones at least
get the two beforementioned tools in sync with sys/param.h at least,
so I see some degree of value in the patches (even if they're just
cleanup).
Thanks,
-Garrett
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTineQfE12Q4sh6QgKapJGJ-S7UHDfiwRasw5nQVG>