[PATCH] Bug with powerof2 macro in sys/param.h

[John Baldwin]

On Thursday, October 14, 2010 7:58:32 am Andriy Gapon wrote:
> on 14/10/2010 00:30 Garrett Cooper said the following:
> >     I was talking to someone today about this macro, and he noted that
> > the algorithm is incorrect -- it fails the base case with ((x) == 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 input (many
> places in the do not allow that).

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:       while (!powerof2(fl_q_size))
sys/dev/cxgb/cxgb_sge.c:       while (!powerof2(jumbo_q_size))

These are fine, will not be zero.

sys/x86/x86/local_apic.c:      KASSERT(powerof2(count), ("bad count"));
sys/x86/x86/local_apic.c:      KASSERT(powerof2(align), ("bad align"));

These are fine.  No code allocates zero IDT vectors.  We never allocate IDT
vectors for unallocated MSI or MSI-X IRQs.

sys/net/flowtable.c:           ft->ft_lock_count =
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.

sys/i386/pci/pci_pir.c:                if (error &&
!powerof2(pci_link->pl_irqmask)) {

This fine.  Earlier 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_irqmask
is non-zero.  In practice pl_irqmask is never zero anyway.

I suspect the GEOM ones are also generally safe.

-- 
John Baldwin