Date: Wed, 13 Oct 2010 14:30:20 -0700 From: Garrett Cooper <gcooper@FreeBSD.org> To: freebsd-hackers@freebsd.org Subject: [PATCH] Bug with powerof2 macro in sys/param.h Message-ID: <AANLkTi=0PSp2KpFTAHTAoosRYdcewiVAxweH4=ivYTLt@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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:
$ ./test_powerof2
Assertion failed: (!powerof2(0)), function main, file test_powerof2.c, line 7.
Abort trap: 6 (core dumped)
Applying the following patch, everything's correct with the
testcase I've written:
$ ./test_powerof2
$
There are a few different places in the sourcebase where this
macro is used, so I'm a bit wary of the implications of having this
patch be committed because this might break functionality somewhere
critical:
sys/dev/cas/if_cas.c: CTASSERT(powerof2(n) && (n) >= (min) && (n) <= (max))
This is ok.
sys/dev/aic7xxx/aic79xx.c: while (powerof2(sg_prefetch_align) == 0)
This is ok.
sys/dev/md/md.c: if (mdio->md_sectorsize != 0 &&
!powerof2(mdio->md_sectorsize))
This is fixed (attached).
sys/dev/mpt/mpt_raid.c:
powerof2(vol_pg->VolumeSettings.HotSparePool)
This is ok.
sys/dev/mpt/mpt_raid.c:
powerof2(disk_pg->PhysDiskSettings.HotSparePool)
This is ok.
sys/dev/gem/if_gem.c:CTASSERT(powerof2(GEM_NRXDESC) && GEM_NRXDESC >=
32 && GEM_NRXDESC <= 8192);
This is ok.
sys/dev/gem/if_gem.c:CTASSERT(powerof2(GEM_NTXDESC) && GEM_NTXDESC >=
32 && GEM_NTXDESC <= 8192);
This is ok.
sys/dev/pci/pci.c: if (!powerof2(actual))
This looks ok.
sys/dev/cxgb/cxgb_sge.c: while (!powerof2(fl_q_size))
sys/dev/cxgb/cxgb_sge.c: while (!powerof2(jumbo_q_size))
I don't feel comfortable enough to state whether or not these are problems.
sys/dev/hme/if_hme.c:CTASSERT(powerof2(HME_NRXDESC) && HME_NRXDESC >=
32 && HME_NRXDESC <= 256);
This is ok.
sys/x86/x86/local_apic.c: KASSERT(powerof2(divisor), ("lapic:
invalid divisor %u", divisor));
This is ok.
sys/x86/x86/local_apic.c: KASSERT(powerof2(count), ("bad count"));
This could be bad if msix is released and this was called, if I was
reading the code correctly.
sys/x86/x86/local_apic.c: KASSERT(powerof2(align), ("bad align"));
This could be bad if msix is released and this was called, if I was
reading the code correctly.
sys/net/flowtable.c: ft->ft_lock_count =
2*(powerof2(mp_maxid + 1) ? (mp_maxid + 1):
This could be bad.
sys/geom/gate/g_gate.c: if (ggio->gctl_sectorsize > 0 &&
!powerof2(ggio->gctl_sectorsize)) {
This is ok.
sys/geom/stripe/g_stripe.c: if (!powerof2(md->md_stripesize)) {
Not sure.
sys/geom/geom_redboot.c: if (powerof2(cp->provider->mediasize))
Not sure.
sys/i386/i386/i686_mem.c: powerof2((len)) && /* ...
and power of two */ \
This is ok.
sys/i386/i386/k6_mem.c: if (desc->mr_len < 131072 ||
!powerof2(desc->mr_len))
This is ok.
sys/i386/pci/pci_pir.c: if (pci_link->pl_irqmask != 0
&& powerof2(pci_link->pl_irqmask))
This is fixed (attached).
sys/i386/pci/pci_pir.c: if (error &&
!powerof2(pci_link->pl_irqmask)) {
Not sure.
sys/netinet6/ip6_input.c: if
(!powerof2(V_ip6_output_flowtable_size)) {
This is ok.
sys/amd64/amd64/amd64_mem.c: powerof2((len)) && /* ...
and power of two */ \
This is ok.
usr.sbin/mptutil/mpt_config.c:#define powerof2(x) ((((x)-1)&(x))==0)
This is fixed (attached).
usr.sbin/mptutil/mpt_config.c: if ((stripe_size < 512)
|| (!powerof2(stripe_size))) {
This is fixed (attached).
usr.sbin/mfiutil/mfi_config.c:#define powerof2(x) ((((x)-1)&(x))==0)
This is fixed (attached).
Could someone please review these patches and help me commit them?
I'm testing out the patches I can (the amd64 and msi and net ones).
Thanks,
-Garrett
[-- Attachment #2 --]
Index: sys/sys/param.h
===================================================================
--- sys/sys/param.h (revision 211767)
+++ sys/sys/param.h (working copy)
@@ -254,7 +254,7 @@
#define rounddown(x, y) (((x)/(y))*(y))
#define roundup(x, y) ((((x)+((y)-1))/(y))*(y)) /* to any y */
#define roundup2(x, y) (((x)+((y)-1))&(~((y)-1))) /* if y is powers of two */
-#define powerof2(x) ((((x)-1)&(x))==0)
+#define powerof2(x) ((x) != 0 && ((((x)-1)&(x))==0))
/* Macros for min/max. */
#define MIN(a,b) (((a)<(b))?(a):(b))
[-- Attachment #3 --]
#include <sys/param.h>
#include <assert.h>
int
main(void)
{
assert (!powerof2(0));
assert (powerof2(1));
assert (powerof2(2));
assert (!powerof2(3));
assert (!powerof2(5));
assert (powerof2(8));
return (0);
}
[-- Attachment #4 --]
Index: usr.sbin/mfiutil/mfi_config.c
===================================================================
--- usr.sbin/mfiutil/mfi_config.c (revision 211767)
+++ usr.sbin/mfiutil/mfi_config.c (working copy)
@@ -29,6 +29,7 @@
* $FreeBSD$
*/
+#include <sys/param.h>
#include <sys/types.h>
#ifdef DEBUG
#include <sys/sysctl.h>
@@ -52,8 +53,6 @@
static int add_spare(int ac, char **av);
static int remove_spare(int ac, char **av);
-#define powerof2(x) ((((x)-1)&(x))==0)
-
static long
dehumanize(const char *value)
{
Index: usr.sbin/mptutil/mpt_config.c
===================================================================
--- usr.sbin/mptutil/mpt_config.c (revision 211767)
+++ usr.sbin/mptutil/mpt_config.c (working copy)
@@ -50,8 +50,6 @@
static void dump_config(CONFIG_PAGE_RAID_VOL_0 *vol);
#endif
-#define powerof2(x) ((((x)-1)&(x))==0)
-
static long
dehumanize(const char *value)
{
[-- Attachment #5 --]
Index: sys/dev/md/md.c
===================================================================
--- sys/dev/md/md.c (revision 211767)
+++ sys/dev/md/md.c (working copy)
@@ -832,7 +832,7 @@
error = 0;
if (mdio->md_options & ~(MD_AUTOUNIT | MD_COMPRESS | MD_RESERVE))
return (EINVAL);
- if (mdio->md_sectorsize != 0 && !powerof2(mdio->md_sectorsize))
+ if (!powerof2(mdio->md_sectorsize))
return (EINVAL);
/* Compression doesn't make sense if we have reserved space */
if (mdio->md_options & MD_RESERVE)
[-- Attachment #6 --]
Index: sys/i386/pci/pci_pir.c
===================================================================
--- sys/i386/pci/pci_pir.c (revision 213802)
+++ sys/i386/pci/pci_pir.c (working copy)
@@ -526,7 +526,7 @@
* IRQs" set.
*/
if (!PCI_INTERRUPT_VALID(pci_link->pl_irq)) {
- if (pci_link->pl_irqmask != 0 && powerof2(pci_link->pl_irqmask))
+ if (powerof2(pci_link->pl_irqmask))
irq = ffs(pci_link->pl_irqmask) - 1;
else
irq = pci_pir_choose_irq(pci_link,
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTi=0PSp2KpFTAHTAoosRYdcewiVAxweH4=ivYTLt>