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
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))
test_powerof2.c
Description: Binary data
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)
{
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)
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,
_______________________________________________ [email protected] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[email protected]"
