[PATCH] remove powerpc bitops infavor of existing generic bitops

[PATCH] remove powerpc bitops infavor of existing generic bitops

[Jon Mason]

There already exists a big endian safe bitops implementation in
lib/find_next_bit.c.  The code in it is 90%+ common with the powerpc
specific version, so the powerpc version is redundant.  This patch
makes the necessary changes to use the generic bitops in powerpc, and
removes the powerpc specific version.

Thanks,
Jon

Signed-off-by: Jon Mason <jdmason@us.ibm.com>

diff -r bfccde0f7221 arch/powerpc/Kconfig
--- a/arch/powerpc/Kconfig	Sat May 13 08:42:09 2006 +0700
+++ b/arch/powerpc/Kconfig	Mon May 15 17:30:46 2006 +0000
@@ -42,6 +42,10 @@ config GENERIC_HWEIGHT
 	default y
 
 config GENERIC_CALIBRATE_DELAY
+	bool
+	default y
+
+config GENERIC_FIND_NEXT_BIT
 	bool
 	default y
 
diff -r bfccde0f7221 arch/powerpc/lib/Makefile
--- a/arch/powerpc/lib/Makefile	Sat May 13 08:42:09 2006 +0700
+++ b/arch/powerpc/lib/Makefile	Mon May 15 17:30:46 2006 +0000
@@ -7,7 +7,6 @@ obj-$(CONFIG_PPC32)	+= div64.o copy_32.o
 obj-$(CONFIG_PPC32)	+= div64.o copy_32.o checksum_32.o
 endif
 
-obj-y			+= bitops.o
 obj-$(CONFIG_PPC64)	+= checksum_64.o copypage_64.o copyuser_64.o \
 			   memcpy_64.o usercopy_64.o mem_64.o string.o \
 			   strcase.o
diff -r bfccde0f7221 include/asm-powerpc/bitops.h
--- a/include/asm-powerpc/bitops.h	Sat May 13 08:42:09 2006 +0700
+++ b/include/asm-powerpc/bitops.h	Mon May 15 17:30:46 2006 +0000
@@ -288,8 +288,8 @@ static __inline__ int test_le_bit(unsign
 #define __test_and_clear_le_bit(nr, addr) \
 	__test_and_clear_bit((nr) ^ BITOP_LE_SWIZZLE, (addr))
 
-#define find_first_zero_le_bit(addr, size) find_next_zero_le_bit((addr), (size), 0)
-unsigned long find_next_zero_le_bit(const unsigned long *addr,
+#define find_first_zero_le_bit(addr, size) generic_find_next_zero_le_bit((addr), (size), 0)
+unsigned long generic_find_next_zero_le_bit(const unsigned long *addr,
 				    unsigned long size, unsigned long offset);
 
 /* Bitmap functions for the ext2 filesystem */
@@ -309,7 +309,7 @@ unsigned long find_next_zero_le_bit(cons
 #define ext2_find_first_zero_bit(addr, size) \
 	find_first_zero_le_bit((unsigned long*)addr, size)
 #define ext2_find_next_zero_bit(addr, size, off) \
-	find_next_zero_le_bit((unsigned long*)addr, size, off)
+	generic_find_next_zero_le_bit((unsigned long*)addr, size, off)
 
 /* Bitmap functions for the minix filesystem.  */
 
diff -r bfccde0f7221 arch/powerpc/lib/bitops.c
--- a/arch/powerpc/lib/bitops.c	Sat May 13 08:42:09 2006 +0700
+++ /dev/null	Thu Jan 01 00:00:00 1970 +0000
@@ -1,150 +0,0 @@
-#include <linux/types.h>
-#include <linux/module.h>
-#include <asm/byteorder.h>
-#include <asm/bitops.h>
-
-/**
- * find_next_bit - find the next set bit in a memory region
- * @addr: The address to base the search on
- * @offset: The bitnumber to start searching at
- * @size: The maximum size to search
- */
-unsigned long find_next_bit(const unsigned long *addr, unsigned long size,
-			    unsigned long offset)
-{
-	const unsigned long *p = addr + BITOP_WORD(offset);
-	unsigned long result = offset & ~(BITS_PER_LONG-1);
-	unsigned long tmp;
-
-	if (offset >= size)
-		return size;
-	size -= result;
-	offset %= BITS_PER_LONG;
-	if (offset) {
-		tmp = *(p++);
-		tmp &= (~0UL << offset);
-		if (size < BITS_PER_LONG)
-			goto found_first;
-		if (tmp)
-			goto found_middle;
-		size -= BITS_PER_LONG;
-		result += BITS_PER_LONG;
-	}
-	while (size & ~(BITS_PER_LONG-1)) {
-		if ((tmp = *(p++)))
-			goto found_middle;
-		result += BITS_PER_LONG;
-		size -= BITS_PER_LONG;
-	}
-	if (!size)
-		return result;
-	tmp = *p;
-
-found_first:
-	tmp &= (~0UL >> (BITS_PER_LONG - size));
-	if (tmp == 0UL)		/* Are any bits set? */
-		return result + size;	/* Nope. */
-found_middle:
-	return result + __ffs(tmp);
-}
-EXPORT_SYMBOL(find_next_bit);
-
-/*
- * This implementation of find_{first,next}_zero_bit was stolen from
- * Linus' asm-alpha/bitops.h.
- */
-unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
-				 unsigned long offset)
-{
-	const unsigned long *p = addr + BITOP_WORD(offset);
-	unsigned long result = offset & ~(BITS_PER_LONG-1);
-	unsigned long tmp;
-
-	if (offset >= size)
-		return size;
-	size -= result;
-	offset %= BITS_PER_LONG;
-	if (offset) {
-		tmp = *(p++);
-		tmp |= ~0UL >> (BITS_PER_LONG - offset);
-		if (size < BITS_PER_LONG)
-			goto found_first;
-		if (~tmp)
-			goto found_middle;
-		size -= BITS_PER_LONG;
-		result += BITS_PER_LONG;
-	}
-	while (size & ~(BITS_PER_LONG-1)) {
-		if (~(tmp = *(p++)))
-			goto found_middle;
-		result += BITS_PER_LONG;
-		size -= BITS_PER_LONG;
-	}
-	if (!size)
-		return result;
-	tmp = *p;
-
-found_first:
-	tmp |= ~0UL << size;
-	if (tmp == ~0UL)	/* Are any bits zero? */
-		return result + size;	/* Nope. */
-found_middle:
-	return result + ffz(tmp);
-}
-EXPORT_SYMBOL(find_next_zero_bit);
-
-static inline unsigned int ext2_ilog2(unsigned int x)
-{
-	int lz;
-
-	asm("cntlzw %0,%1": "=r"(lz):"r"(x));
-	return 31 - lz;
-}
-
-static inline unsigned int ext2_ffz(unsigned int x)
-{
-	u32 rc;
-	if ((x = ~x) == 0)
-		return 32;
-	rc = ext2_ilog2(x & -x);
-	return rc;
-}
-
-unsigned long find_next_zero_le_bit(const unsigned long *addr,
-				    unsigned long size, unsigned long offset)
-{
-	const unsigned int *p = ((const unsigned int *)addr) + (offset >> 5);
-	unsigned int result = offset & ~31;
-	unsigned int tmp;
-
-	if (offset >= size)
-		return size;
-	size -= result;
-	offset &= 31;
-	if (offset) {
-		tmp = cpu_to_le32p(p++);
-		tmp |= ~0U >> (32 - offset);	/* bug or feature ? */
-		if (size < 32)
-			goto found_first;
-		if (tmp != ~0)
-			goto found_middle;
-		size -= 32;
-		result += 32;
-	}
-	while (size >= 32) {
-		if ((tmp = cpu_to_le32p(p++)) != ~0)
-			goto found_middle;
-		result += 32;
-		size -= 32;
-	}
-	if (!size)
-		return result;
-	tmp = cpu_to_le32p(p);
-found_first:
-	tmp |= ~0 << size;
-	if (tmp == ~0)		/* Are any bits zero? */
-		return result + size;	/* Nope. */
-found_middle:
-	return result + ext2_ffz(tmp);
-}
-EXPORT_SYMBOL(find_next_zero_le_bit);

Re: [PATCH] remove powerpc bitops infavor of existing generic bitops

[jschopp]

> There already exists a big endian safe bitops implementation in
> lib/find_next_bit.c.  The code in it is 90%+ common with the powerpc
> specific version, so the powerpc version is redundant.  This patch
> makes the necessary changes to use the generic bitops in powerpc, and
> removes the powerpc specific version.

I like generic as much as the next guy, but I'm also a big fan of fast bitops.  And the 
function below is fast.  You'll have to explain to me how the generic code is going to 
find the first zero as fast without explicit calls to ppc assembly.

> -static inline unsigned int ext2_ilog2(unsigned int x)
> -{
> -	int lz;
> -
> -	asm("cntlzw %0,%1": "=r"(lz):"r"(x));
> -	return 31 - lz;
> -}

Re: [PATCH] remove powerpc bitops infavor of existing generic bitops

[Jon Mason]

On Mon, May 15, 2006 at 02:56:13PM -0500, jschopp wrote:
> >There already exists a big endian safe bitops implementation in
> >lib/find_next_bit.c.  The code in it is 90%+ common with the powerpc
> >specific version, so the powerpc version is redundant.  This patch
> >makes the necessary changes to use the generic bitops in powerpc, and
> >removes the powerpc specific version.
> 
> I like generic as much as the next guy, but I'm also a big fan of fast 
> bitops.  And the function below is fast.  You'll have to explain to me how 
> the generic code is going to find the first zero as fast without explicit 
> calls to ppc assembly.
> 
> >-static inline unsigned int ext2_ilog2(unsigned int x)
> >-{
> >-	int lz;
> >-
> >-	asm("cntlzw %0,%1": "=r"(lz):"r"(x));
> >-	return 31 - lz;
> >-}

Ah but here's the trick, there is the same explicit call to ppc
assembly.  The only function in the file removed is this one you pointed
out, and the only caller of this function is ext2_ffz.  And the only
user of ext2_ffz is find_next_zero_le_bit.  

Now the generic code is very similar to the file removed (`diff -Narup
arch/powerpc/lib/bitops.c lib/find_next_bit.c` to see for yourself).  In
the same place where ext2_ffz is called, ffz is called in the generic
code.  Now if we look at the definition of ffz in
include/asm-powerpc/bitops.h, we see it calls __ilog2 of that same file.
__ilog2 is defined as: 

static __inline__ int __ilog2(unsigned long x)
{
        int lz;

        asm (PPC_CNTLZL "%0,%1" : "=r" (lz) : "r" (x));
        return BITS_PER_LONG - 1 - lz;
}

So, its really the same code :)

Thanks,
Jon

Re: [PATCH] remove powerpc bitops infavor of existing generic bitops

[jschopp]

> Ah but here's the trick, there is the same explicit call to ppc
> assembly.  The only function in the file removed is this one you pointed
> out, and the only caller of this function is ext2_ffz.  And the only
> user of ext2_ffz is find_next_zero_le_bit.  
> 
> Now the generic code is very similar to the file removed (`diff -Narup
> arch/powerpc/lib/bitops.c lib/find_next_bit.c` to see for yourself).  In
> the same place where ext2_ffz is called, ffz is called in the generic
> code.  Now if we look at the definition of ffz in
> include/asm-powerpc/bitops.h, we see it calls __ilog2 of that same file.
> __ilog2 is defined as: 
> 
> static __inline__ int __ilog2(unsigned long x)
> {
>         int lz;
> 
>         asm (PPC_CNTLZL "%0,%1" : "=r" (lz) : "r" (x));
>         return BITS_PER_LONG - 1 - lz;
> }
> 
> So, its really the same code :)

Good explination.  I'm convinced, so for what it's worth:

Acked-by: Joel Schopp <jschopp@austin.ibm.com>

Re: [PATCH] remove powerpc bitops infavor of existing generic bitops

[Paul Mackerras]

Jon Mason writes:

> There already exists a big endian safe bitops implementation in
> lib/find_next_bit.c.  The code in it is 90%+ common with the powerpc
> specific version, so the powerpc version is redundant.  This patch
> makes the necessary changes to use the generic bitops in powerpc, and
> removes the powerpc specific version.

This patch breaks ARCH=ppc builds.  Please resubmit with the necessary
changes to arch/ppc/Kconfig as well.

Paul.

Re: [PATCH] remove powerpc bitops infavor of existing generic bitops

[Jon Mason]

On Fri, May 19, 2006 at 03:00:41PM +1000, Paul Mackerras wrote:
> Jon Mason writes:
> 
> > There already exists a big endian safe bitops implementation in
> > lib/find_next_bit.c.  The code in it is 90%+ common with the powerpc
> > specific version, so the powerpc version is redundant.  This patch
> > makes the necessary changes to use the generic bitops in powerpc, and
> > removes the powerpc specific version.
> 
> This patch breaks ARCH=ppc builds.  Please resubmit with the necessary
> changes to arch/ppc/Kconfig as well.
> 
> Paul.

Sorry.  Here is the patch with the necessary correction.

Thanks,
Jon

Signed-off-by: Jon Mason <jdmason@us.ibm.com>

diff -r a89fd2f124df arch/powerpc/Kconfig
--- a/arch/powerpc/Kconfig	Fri May 19 16:17:18 2006 +0700
+++ b/arch/powerpc/Kconfig	Fri May 19 15:34:39 2006 -0500
@@ -42,6 +42,10 @@ config GENERIC_HWEIGHT
 	default y
 
 config GENERIC_CALIBRATE_DELAY
+	bool
+	default y
+
+config GENERIC_FIND_NEXT_BIT
 	bool
 	default y
 
diff -r a89fd2f124df arch/powerpc/lib/Makefile
--- a/arch/powerpc/lib/Makefile	Fri May 19 16:17:18 2006 +0700
+++ b/arch/powerpc/lib/Makefile	Fri May 19 15:34:39 2006 -0500
@@ -7,7 +7,6 @@ obj-$(CONFIG_PPC32)	+= div64.o copy_32.o
 obj-$(CONFIG_PPC32)	+= div64.o copy_32.o checksum_32.o
 endif
 
-obj-y			+= bitops.o
 obj-$(CONFIG_PPC64)	+= checksum_64.o copypage_64.o copyuser_64.o \
 			   memcpy_64.o usercopy_64.o mem_64.o string.o \
 			   strcase.o
diff -r a89fd2f124df arch/ppc/Kconfig
--- a/arch/ppc/Kconfig	Fri May 19 16:17:18 2006 +0700
+++ b/arch/ppc/Kconfig	Fri May 19 15:34:39 2006 -0500
@@ -37,6 +37,10 @@ config PPC32
 
 # All PPCs use generic nvram driver through ppc_md
 config GENERIC_NVRAM
+	bool
+	default y
+
+config GENERIC_FIND_NEXT_BIT
 	bool
 	default y
 
diff -r a89fd2f124df include/asm-powerpc/bitops.h
--- a/include/asm-powerpc/bitops.h	Fri May 19 16:17:18 2006 +0700
+++ b/include/asm-powerpc/bitops.h	Fri May 19 15:34:39 2006 -0500
@@ -288,8 +288,8 @@ static __inline__ int test_le_bit(unsign
 #define __test_and_clear_le_bit(nr, addr) \
 	__test_and_clear_bit((nr) ^ BITOP_LE_SWIZZLE, (addr))
 
-#define find_first_zero_le_bit(addr, size) find_next_zero_le_bit((addr), (size), 0)
-unsigned long find_next_zero_le_bit(const unsigned long *addr,
+#define find_first_zero_le_bit(addr, size) generic_find_next_zero_le_bit((addr), (size), 0)
+unsigned long generic_find_next_zero_le_bit(const unsigned long *addr,
 				    unsigned long size, unsigned long offset);
 
 /* Bitmap functions for the ext2 filesystem */
@@ -309,7 +309,7 @@ unsigned long find_next_zero_le_bit(cons
 #define ext2_find_first_zero_bit(addr, size) \
 	find_first_zero_le_bit((unsigned long*)addr, size)
 #define ext2_find_next_zero_bit(addr, size, off) \
-	find_next_zero_le_bit((unsigned long*)addr, size, off)
+	generic_find_next_zero_le_bit((unsigned long*)addr, size, off)
 
 /* Bitmap functions for the minix filesystem.  */
 
diff -r a89fd2f124df arch/powerpc/lib/bitops.c
--- a/arch/powerpc/lib/bitops.c	Fri May 19 16:17:18 2006 +0700
+++ /dev/null	Thu Jan  1 00:00:00 1970 +0000
@@ -1,150 +0,0 @@
-#include <linux/types.h>
-#include <linux/module.h>
-#include <asm/byteorder.h>
-#include <asm/bitops.h>
-
-/**
- * find_next_bit - find the next set bit in a memory region
- * @addr: The address to base the search on
- * @offset: The bitnumber to start searching at
- * @size: The maximum size to search
- */
-unsigned long find_next_bit(const unsigned long *addr, unsigned long size,
-			    unsigned long offset)
-{
-	const unsigned long *p = addr + BITOP_WORD(offset);
-	unsigned long result = offset & ~(BITS_PER_LONG-1);
-	unsigned long tmp;
-
-	if (offset >= size)
-		return size;
-	size -= result;
-	offset %= BITS_PER_LONG;
-	if (offset) {
-		tmp = *(p++);
-		tmp &= (~0UL << offset);
-		if (size < BITS_PER_LONG)
-			goto found_first;
-		if (tmp)
-			goto found_middle;
-		size -= BITS_PER_LONG;
-		result += BITS_PER_LONG;
-	}
-	while (size & ~(BITS_PER_LONG-1)) {
-		if ((tmp = *(p++)))
-			goto found_middle;
-		result += BITS_PER_LONG;
-		size -= BITS_PER_LONG;
-	}
-	if (!size)
-		return result;
-	tmp = *p;
-
-found_first:
-	tmp &= (~0UL >> (BITS_PER_LONG - size));
-	if (tmp == 0UL)		/* Are any bits set? */
-		return result + size;	/* Nope. */
-found_middle:
-	return result + __ffs(tmp);
-}
-EXPORT_SYMBOL(find_next_bit);
-
-/*
- * This implementation of find_{first,next}_zero_bit was stolen from
- * Linus' asm-alpha/bitops.h.
- */
-unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
-				 unsigned long offset)
-{
-	const unsigned long *p = addr + BITOP_WORD(offset);
-	unsigned long result = offset & ~(BITS_PER_LONG-1);
-	unsigned long tmp;
-
-	if (offset >= size)
-		return size;
-	size -= result;
-	offset %= BITS_PER_LONG;
-	if (offset) {
-		tmp = *(p++);
-		tmp |= ~0UL >> (BITS_PER_LONG - offset);
-		if (size < BITS_PER_LONG)
-			goto found_first;
-		if (~tmp)
-			goto found_middle;
-		size -= BITS_PER_LONG;
-		result += BITS_PER_LONG;
-	}
-	while (size & ~(BITS_PER_LONG-1)) {
-		if (~(tmp = *(p++)))
-			goto found_middle;
-		result += BITS_PER_LONG;
-		size -= BITS_PER_LONG;
-	}
-	if (!size)
-		return result;
-	tmp = *p;
-
-found_first:
-	tmp |= ~0UL << size;
-	if (tmp == ~0UL)	/* Are any bits zero? */
-		return result + size;	/* Nope. */
-found_middle:
-	return result + ffz(tmp);
-}
-EXPORT_SYMBOL(find_next_zero_bit);
-
-static inline unsigned int ext2_ilog2(unsigned int x)
-{
-	int lz;
-
-	asm("cntlzw %0,%1": "=r"(lz):"r"(x));
-	return 31 - lz;
-}
-
-static inline unsigned int ext2_ffz(unsigned int x)
-{
-	u32 rc;
-	if ((x = ~x) == 0)
-		return 32;
-	rc = ext2_ilog2(x & -x);
-	return rc;
-}
-
-unsigned long find_next_zero_le_bit(const unsigned long *addr,
-				    unsigned long size, unsigned long offset)
-{
-	const unsigned int *p = ((const unsigned int *)addr) + (offset >> 5);
-	unsigned int result = offset & ~31;
-	unsigned int tmp;
-
-	if (offset >= size)
-		return size;
-	size -= result;
-	offset &= 31;
-	if (offset) {
-		tmp = cpu_to_le32p(p++);
-		tmp |= ~0U >> (32 - offset);	/* bug or feature ? */
-		if (size < 32)
-			goto found_first;
-		if (tmp != ~0)
-			goto found_middle;
-		size -= 32;
-		result += 32;
-	}
-	while (size >= 32) {
-		if ((tmp = cpu_to_le32p(p++)) != ~0)
-			goto found_middle;
-		result += 32;
-		size -= 32;
-	}
-	if (!size)
-		return result;
-	tmp = cpu_to_le32p(p);
-found_first:
-	tmp |= ~0 << size;
-	if (tmp == ~0)		/* Are any bits zero? */
-		return result + size;	/* Nope. */
-found_middle:
-	return result + ext2_ffz(tmp);
-}
-EXPORT_SYMBOL(find_next_zero_le_bit);