[PATCH 0/6] use BUG_ON correctly, v2

[PATCH 0/6] use BUG_ON correctly, v2

[Coly Li]

Most of BUG_ON() implementations use unlikely() internally for better
branch prediction results. The following method to use BUG_ON() with
an explicit unlikely() doesn't make things better and is unwelcome:
	BUG_ON(unlikely(condition));
Source code should use BUG_ON() just with condition code.

For arch dependent BUG_ON() implementations, they should use unlikely()
internally if they are able to.

This patch set does two things,
1) Remove all explicit unlikey() where kernel code uses BUG_ON().
2) Fix arch dependent BUG_ON() implementations if they don't use
   unlikely() internally.

The difference between v2 and v1 patch set are,
1) Remove the fix of mm/nommu.c, because it's in mm-tree already.
2) Add unlikely() inside the BUG_ON() implementations of MIPS and PPC.

Signed-off-by: Coly Li <bosong.ly@taobao.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg KH <gregkh@suse.de>
---
 arch/mips/include/asm/bug.h                      |    2 +-
 arch/powerpc/include/asm/bug.h                   |    4 +++-
 drivers/dma/iop-adma.c                           |    6 +++---
 drivers/dma/mv_xor.c                             |    6 +++---
 drivers/dma/ppc4xx/adma.c                        |    8 ++++----
 drivers/scsi/scsi_lib.c                          |    4 ++--
 drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c |   14 +++++++-------
 7 files changed, 23 insertions(+), 21 deletions(-)

[PATCH 1/7] MIPS: add unlikely() to BUG_ON()

[Coly Li]

Current BUG_ON() in arch/mips/include/asm/bug.h does not use unlikely(),
in order to get better branch predict result, source code should call
BUG_ON() with unlikely() explicitly. This is not a suggested method to
use BUG_ON().

This patch adds unlikely() inside BUG_ON implementation on MIPS code,
callers can use BUG_ON without explicit unlikely() now.

I have no usable MIPS hardware to build and test the fix, any test result
of this patch is welcome.

Signed-off-by: Coly Li <bosong.ly@taobao.com>
Cc: David Daney <ddaney@caviumnetworks.com>
Cc: Wang Cong <xiyou.wangcong@gmail.com>
Cc: Yong Zhang <yong.zhang0@gmail.com>
---
 arch/mips/include/asm/bug.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/mips/include/asm/bug.h b/arch/mips/include/asm/bug.h
index 540c98a..6771c07 100644
--- a/arch/mips/include/asm/bug.h
+++ b/arch/mips/include/asm/bug.h
@@ -30,7 +30,7 @@ static inline void  __BUG_ON(unsigned long condition)
 			     : : "r" (condition), "i" (BRK_BUG));
 }
 
-#define BUG_ON(C) __BUG_ON((unsigned long)(C))
+#define BUG_ON(C) __BUG_ON((unsigned long)unlikely(C))
 
 #define HAVE_ARCH_BUG_ON
 
-- 
1.7.3.2

[PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[Coly Li]

Current BUG_ON() arch/powerpc/include/asm/bug.h does not use unlikely(),
in order to get better branch predict result, source code may have to call
BUG_ON() with unlikely() explicitly. This is not a suggested method
to use BUG_ON().

This patch adds unlikely() inside BUG_ON implementation on PPC
code, callers can use BUG_ON without explicit unlikely() now.

I don't have any PPC hardware to compile and test this fix, any feedback
of this patch is welcome.

Signed-off-by: Coly Li <bosong.ly@taobao.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: David Daney <ddaney@caviumnetworks.com>
Cc: Wang Cong <xiyou.wangcong@gmail.com>
Cc: Yong Zhang <yong.zhang0@gmail.com>
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 065c590..10889a6 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -2,6 +2,7 @@
 #define _ASM_POWERPC_BUG_H
 #ifdef __KERNEL__
 
+#include <linux/compiler.h>
 #include <asm/asm-compat.h>
 
 /*
@@ -71,7 +72,7 @@
 	unreachable();						\
 } while (0)
 
-#define BUG_ON(x) do {						\
+#define __BUG_ON(x) do {					\
 	if (__builtin_constant_p(x)) {				\
 		if (x)						\
 			BUG();					\
@@ -85,6 +86,8 @@
 	}							\
 } while (0)
 
+#define BUG_ON(x) __BUG_ON(unlikely(x))
+
 #define __WARN_TAINT(taint) do {				\
 	__asm__ __volatile__(					\
 		"1:	twi 31,0,0\n"				\

[PATCH 3/7] dma: use BUG_ON correctly in iop-adma.c

[Coly Li]

This patch makes BUG_ON() usage correct in drivers/dma/iop-adma.c.

Signed-off-by: Coly Li <bosong.ly@taobao.com>
Cc: Greg KH <gregkh@suse.de>
---
 drivers/dma/iop-adma.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/iop-adma.c b/drivers/dma/iop-adma.c
index c6b01f5..e03f811 100644
--- a/drivers/dma/iop-adma.c
+++ b/drivers/dma/iop-adma.c
@@ -619,7 +619,7 @@ iop_adma_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dma_dest,
 
 	if (unlikely(!len))
 		return NULL;
-	BUG_ON(unlikely(len > IOP_ADMA_MAX_BYTE_COUNT));
+	BUG_ON(len > IOP_ADMA_MAX_BYTE_COUNT);
 
 	dev_dbg(iop_chan->device->common.dev, "%s len: %u\n",
 		__func__, len);
@@ -652,7 +652,7 @@ iop_adma_prep_dma_memset(struct dma_chan *chan, dma_addr_t dma_dest,
 
 	if (unlikely(!len))
 		return NULL;
-	BUG_ON(unlikely(len > IOP_ADMA_MAX_BYTE_COUNT));
+	BUG_ON(len > IOP_ADMA_MAX_BYTE_COUNT);
 
 	dev_dbg(iop_chan->device->common.dev, "%s len: %u\n",
 		__func__, len);
@@ -686,7 +686,7 @@ iop_adma_prep_dma_xor(struct dma_chan *chan, dma_addr_t dma_dest,
 
 	if (unlikely(!len))
 		return NULL;
-	BUG_ON(unlikely(len > IOP_ADMA_XOR_MAX_BYTE_COUNT));
+	BUG_ON(len > IOP_ADMA_XOR_MAX_BYTE_COUNT);
 
 	dev_dbg(iop_chan->device->common.dev,
 		"%s src_cnt: %d len: %u flags: %lx\n",
-- 
1.7.3.2

[PATCH 4/7] dma: use BUG_ON correctly in mv_xor.c

[Coly Li]

This patch makes BUG_ON() usage correct in drivers/dma/mv_xor.c

Signed-off-by: Coly Li <bosong.ly@taobao.com>
Cc: Greg KH <gregkh@suse.de>
---
 drivers/dma/mv_xor.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index a25f5f6..954e334 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -671,7 +671,7 @@ mv_xor_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 	if (unlikely(len < MV_XOR_MIN_BYTE_COUNT))
 		return NULL;
 
-	BUG_ON(unlikely(len > MV_XOR_MAX_BYTE_COUNT));
+	BUG_ON(len > MV_XOR_MAX_BYTE_COUNT);
 
 	spin_lock_bh(&mv_chan->lock);
 	slot_cnt = mv_chan_memcpy_slot_count(len);
@@ -710,7 +710,7 @@ mv_xor_prep_dma_memset(struct dma_chan *chan, dma_addr_t dest, int value,
 	if (unlikely(len < MV_XOR_MIN_BYTE_COUNT))
 		return NULL;
 
-	BUG_ON(unlikely(len > MV_XOR_MAX_BYTE_COUNT));
+	BUG_ON(len > MV_XOR_MAX_BYTE_COUNT);
 
 	spin_lock_bh(&mv_chan->lock);
 	slot_cnt = mv_chan_memset_slot_count(len);
@@ -744,7 +744,7 @@ mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src,
 	if (unlikely(len < MV_XOR_MIN_BYTE_COUNT))
 		return NULL;
 
-	BUG_ON(unlikely(len > MV_XOR_MAX_BYTE_COUNT));
+	BUG_ON(len > MV_XOR_MAX_BYTE_COUNT);
 
 	dev_dbg(mv_chan->device->common.dev,
 		"%s src_cnt: %d len: dest %x %u flags: %ld\n",
-- 
1.7.3.2

[PATCH 5/7] dma: use BUG_ON correctly in ppc4xx/adam.c

[Coly Li]

This patch makes BUG_ON() usage correct in drivers/dma/ppc4xx/adam.c

Signed-off-by: Coly Li <bosong.ly@taobao.com>
Cc: Greg KH <gregkh@suse.de>
---
 drivers/dma/ppc4xx/adma.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
index cef5845..a2b62e9 100644
--- a/drivers/dma/ppc4xx/adma.c
+++ b/drivers/dma/ppc4xx/adma.c
@@ -2313,7 +2313,7 @@ static struct dma_async_tx_descriptor *ppc440spe_adma_prep_dma_memcpy(
 	if (unlikely(!len))
 		return NULL;
 
-	BUG_ON(unlikely(len > PPC440SPE_ADMA_DMA_MAX_BYTE_COUNT));
+	BUG_ON(len > PPC440SPE_ADMA_DMA_MAX_BYTE_COUNT);
 
 	spin_lock_bh(&ppc440spe_chan->lock);
 
@@ -2354,7 +2354,7 @@ static struct dma_async_tx_descriptor *ppc440spe_adma_prep_dma_memset(
 	if (unlikely(!len))
 		return NULL;
 
-	BUG_ON(unlikely(len > PPC440SPE_ADMA_DMA_MAX_BYTE_COUNT));
+	BUG_ON(len > PPC440SPE_ADMA_DMA_MAX_BYTE_COUNT);
 
 	spin_lock_bh(&ppc440spe_chan->lock);
 
@@ -2397,7 +2397,7 @@ static struct dma_async_tx_descriptor *ppc440spe_adma_prep_dma_xor(
 				     dma_dest, dma_src, src_cnt));
 	if (unlikely(!len))
 		return NULL;
-	BUG_ON(unlikely(len > PPC440SPE_ADMA_XOR_MAX_BYTE_COUNT));
+	BUG_ON(len > PPC440SPE_ADMA_XOR_MAX_BYTE_COUNT);
 
 	dev_dbg(ppc440spe_chan->device->common.dev,
 		"ppc440spe adma%d: %s src_cnt: %d len: %u int_en: %d\n",
@@ -2887,7 +2887,7 @@ static struct dma_async_tx_descriptor *ppc440spe_adma_prep_dma_pq(
 	ADMA_LL_DBG(prep_dma_pq_dbg(ppc440spe_chan->device->id,
 				    dst, src, src_cnt));
 	BUG_ON(!len);
-	BUG_ON(unlikely(len > PPC440SPE_ADMA_XOR_MAX_BYTE_COUNT));
+	BUG_ON(len > PPC440SPE_ADMA_XOR_MAX_BYTE_COUNT);
 	BUG_ON(!src_cnt);
 
 	if (src_cnt == 1 && dst[1] == src[0]) {
-- 
1.7.3.2

[PATCH 6/7] wl_cfg80211.c: use BUG_ON correctly

[Coly Li]

This patch removes explicit unlikely() when using BUG_ON() in
wl_cfg80211.c

Signed-off-by: Coly Li <bosong.ly@taobao.com>
Cc: Greg KH <gregkh@suse.de>
---
 drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c b/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c
index 991463f..35124b1 100644
--- a/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c
@@ -673,7 +673,7 @@ wl_dev_iovar_setbuf(struct net_device *dev, s8 * iovar, void *param,
 	s32 iolen;
 
 	iolen = bcm_mkiovar(iovar, param, paramlen, bufptr, buflen);
-	BUG_ON(unlikely(!iolen));
+	BUG_ON(!iolen);
 
 	return wl_dev_ioctl(dev, WLC_SET_VAR, bufptr, iolen);
 }
@@ -685,7 +685,7 @@ wl_dev_iovar_getbuf(struct net_device *dev, s8 * iovar, void *param,
 	s32 iolen;
 
 	iolen = bcm_mkiovar(iovar, param, paramlen, bufptr, buflen);
-	BUG_ON(unlikely(!iolen));
+	BUG_ON(!iolen);
 
 	return wl_dev_ioctl(dev, WLC_GET_VAR, bufptr, buflen);
 }
@@ -704,7 +704,7 @@ wl_run_iscan(struct wl_iscan_ctrl *iscan, struct wlc_ssid *ssid, u16 action)
 	if (unlikely(!params))
 		return -ENOMEM;
 	memset(params, 0, params_size);
-	BUG_ON(unlikely(params_size >= WLC_IOCTL_SMLEN));
+	BUG_ON(params_size >= WLC_IOCTL_SMLEN);
 
 	wl_iscan_prep(&params->params, ssid);
 
@@ -875,7 +875,7 @@ static s32 wl_dev_intvar_set(struct net_device *dev, s8 *name, s32 val)
 
 	val = htod32(val);
 	len = bcm_mkiovar(name, (char *)(&val), sizeof(val), buf, sizeof(buf));
-	BUG_ON(unlikely(!len));
+	BUG_ON(!len);
 
 	err = wl_dev_ioctl(dev, WLC_SET_VAR, buf, len);
 	if (unlikely(err)) {
@@ -899,7 +899,7 @@ wl_dev_intvar_get(struct net_device *dev, s8 *name, s32 *retval)
 	len =
 	    bcm_mkiovar(name, (char *)(&data_null), 0, (char *)(&var),
 			sizeof(var.buf));
-	BUG_ON(unlikely(!len));
+	BUG_ON(!len);
 	err = wl_dev_ioctl(dev, WLC_GET_VAR, &var, len);
 	if (unlikely(err)) {
 		WL_ERR("error (%d)\n", err);
@@ -2436,7 +2436,7 @@ wl_dev_bufvar_set(struct net_device *dev, s8 *name, s8 *buf, s32 len)
 	u32 buflen;
 
 	buflen = bcm_mkiovar(name, buf, len, wl->ioctl_buf, WL_IOCTL_LEN_MAX);
-	BUG_ON(unlikely(!buflen));
+	BUG_ON(!buflen);
 
 	return wl_dev_ioctl(dev, WLC_SET_VAR, wl->ioctl_buf, buflen);
 }
@@ -2450,7 +2450,7 @@ wl_dev_bufvar_get(struct net_device *dev, s8 *name, s8 *buf,
 	s32 err = 0;
 
 	len = bcm_mkiovar(name, NULL, 0, wl->ioctl_buf, WL_IOCTL_LEN_MAX);
-	BUG_ON(unlikely(!len));
+	BUG_ON(!len);
 	err = wl_dev_ioctl(dev, WLC_GET_VAR, (void *)wl->ioctl_buf,
 			WL_IOCTL_LEN_MAX);
 	if (unlikely(err)) {
-- 
1.7.3.2

[PATCH 7/7] scsi_lib.c: use BUG_ON correctly

[Coly Li]

This patch makes BUG_ON() usage correct in drivers/scsi/scsi_lib.c

Signed-off-by: Coly Li <bosong.ly@taobao.com>
Cc: Martin K. Petersen<martin.petersen@oracle.com>
Cc: Greg KH <gregkh@suse.de>
---
 drivers/scsi/scsi_lib.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9045c52..7790a51 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1002,8 +1002,8 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 
 		count = blk_rq_map_integrity_sg(rq->q, rq->bio,
 						prot_sdb->table.sgl);
-		BUG_ON(unlikely(count > ivecs));
-		BUG_ON(unlikely(count > queue_max_integrity_segments(rq->q)));
+		BUG_ON(count > ivecs);
+		BUG_ON(count > queue_max_integrity_segments(rq->q));
 
 		cmd->prot_sdb = prot_sdb;
 		cmd->prot_sdb->table.nents = count;
-- 
1.7.3.2

Re: [PATCH 1/7] MIPS: add unlikely() to BUG_ON()

[David Daney]

Please Cc: linux-mips@linux-mips.org for MIPS patches.

On 01/27/2011 04:12 AM, Coly Li wrote:
> Current BUG_ON() in arch/mips/include/asm/bug.h does not use unlikely(),
> in order to get better branch predict result, source code should call
> BUG_ON() with unlikely() explicitly. This is not a suggested method to
> use BUG_ON().
>
> This patch adds unlikely() inside BUG_ON implementation on MIPS code,
> callers can use BUG_ON without explicit unlikely() now.
>
> I have no usable MIPS hardware to build and test the fix, any test result
> of this patch is welcome.
>
> Signed-off-by: Coly Li<bosong.ly@taobao.com>
> Cc: David Daney<ddaney@caviumnetworks.com>
> Cc: Wang Cong<xiyou.wangcong@gmail.com>
> Cc: Yong Zhang<yong.zhang0@gmail.com>
> ---
>   arch/mips/include/asm/bug.h |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/mips/include/asm/bug.h b/arch/mips/include/asm/bug.h
> index 540c98a..6771c07 100644
> --- a/arch/mips/include/asm/bug.h
> +++ b/arch/mips/include/asm/bug.h
> @@ -30,7 +30,7 @@ static inline void  __BUG_ON(unsigned long condition)
>   			     : : "r" (condition), "i" (BRK_BUG));
>   }
>
> -#define BUG_ON(C) __BUG_ON((unsigned long)(C))
> +#define BUG_ON(C) __BUG_ON((unsigned long)unlikely(C))
>

NAK.

__BUG_ON() expands to a single instruction.  Frobbing about with 
unlikely() will have no effect on the generated code and is thus 
gratuitous code churn.

David Daney

Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[David Daney]

Why not also CC the PPC maintainers as well?  I am not certain, but I 
think they may be reached at:

linuxppc-dev@lists.ozlabs.org


On 01/27/2011 04:12 AM, Coly Li wrote:
> Current BUG_ON() arch/powerpc/include/asm/bug.h does not use unlikely(),
> in order to get better branch predict result, source code may have to call
> BUG_ON() with unlikely() explicitly. This is not a suggested method
> to use BUG_ON().
>
> This patch adds unlikely() inside BUG_ON implementation on PPC
> code, callers can use BUG_ON without explicit unlikely() now.
>
> I don't have any PPC hardware to compile and test this fix, any feedback
> of this patch is welcome.
>
> Signed-off-by: Coly Li<bosong.ly@taobao.com>
> Cc: Jeremy Fitzhardinge<jeremy@goop.org>
> Cc: David Daney<ddaney@caviumnetworks.com>
> Cc: Wang Cong<xiyou.wangcong@gmail.com>
> Cc: Yong Zhang<yong.zhang0@gmail.com>
> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> index 065c590..10889a6 100644
> --- a/arch/powerpc/include/asm/bug.h
> +++ b/arch/powerpc/include/asm/bug.h
> @@ -2,6 +2,7 @@
>   #define _ASM_POWERPC_BUG_H
>   #ifdef __KERNEL__
>
> +#include<linux/compiler.h>
>   #include<asm/asm-compat.h>
>
>   /*
> @@ -71,7 +72,7 @@
>   	unreachable();						\
>   } while (0)
>
> -#define BUG_ON(x) do {						\
> +#define __BUG_ON(x) do {					\
>   	if (__builtin_constant_p(x)) {				\
>   		if (x)						\
>   			BUG();					\
> @@ -85,6 +86,8 @@
>   	}							\
>   } while (0)
>
> +#define BUG_ON(x) __BUG_ON(unlikely(x))
> +

This is the same type of frobbing you were trying to do to MIPS.

I will let the powerpc maintainers weigh in on it, but my opinion is 
that, as with MIPS, BUG_ON() is expanded to a single machine 
instruction, and this unlikely() business will not change the generated 
code in any useful way.  It is thus gratuitous code churn and 
complexification.

David Daney

>   #define __WARN_TAINT(taint) do {				\
>   	__asm__ __volatile__(					\
>   		"1:	twi 31,0,0\n"				\

Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[David Daney]

Why not also CC the PPC maintainers as well?  I am not certain, but I 
think they may be reached at:

linuxppc-dev@lists.ozlabs.org


On 01/27/2011 04:12 AM, Coly Li wrote:
> Current BUG_ON() arch/powerpc/include/asm/bug.h does not use unlikely(),
> in order to get better branch predict result, source code may have to call
> BUG_ON() with unlikely() explicitly. This is not a suggested method
> to use BUG_ON().
>
> This patch adds unlikely() inside BUG_ON implementation on PPC
> code, callers can use BUG_ON without explicit unlikely() now.
>
> I don't have any PPC hardware to compile and test this fix, any feedback
> of this patch is welcome.
>
> Signed-off-by: Coly Li<bosong.ly@taobao.com>
> Cc: Jeremy Fitzhardinge<jeremy@goop.org>
> Cc: David Daney<ddaney@caviumnetworks.com>
> Cc: Wang Cong<xiyou.wangcong@gmail.com>
> Cc: Yong Zhang<yong.zhang0@gmail.com>
> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> index 065c590..10889a6 100644
> --- a/arch/powerpc/include/asm/bug.h
> +++ b/arch/powerpc/include/asm/bug.h
> @@ -2,6 +2,7 @@
>   #define _ASM_POWERPC_BUG_H
>   #ifdef __KERNEL__
>
> +#include<linux/compiler.h>
>   #include<asm/asm-compat.h>
>
>   /*
> @@ -71,7 +72,7 @@
>   	unreachable();						\
>   } while (0)
>
> -#define BUG_ON(x) do {						\
> +#define __BUG_ON(x) do {					\
>   	if (__builtin_constant_p(x)) {				\
>   		if (x)						\
>   			BUG();					\
> @@ -85,6 +86,8 @@
>   	}							\
>   } while (0)
>
> +#define BUG_ON(x) __BUG_ON(unlikely(x))
> +

This is the same type of frobbing you were trying to do to MIPS.

I will let the powerpc maintainers weigh in on it, but my opinion is 
that, as with MIPS, BUG_ON() is expanded to a single machine 
instruction, and this unlikely() business will not change the generated 
code in any useful way.  It is thus gratuitous code churn and 
complexification.

David Daney

>   #define __WARN_TAINT(taint) do {				\
>   	__asm__ __volatile__(					\
>   		"1:	twi 31,0,0\n"				\

Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[Scott Wood]

On Thu, 27 Jan 2011 09:57:39 -0800
David Daney <ddaney@caviumnetworks.com> wrote:

> On 01/27/2011 04:12 AM, Coly Li wrote:
> > diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> > index 065c590..10889a6 100644
> > --- a/arch/powerpc/include/asm/bug.h
> > +++ b/arch/powerpc/include/asm/bug.h
> > @@ -2,6 +2,7 @@
> >   #define _ASM_POWERPC_BUG_H
> >   #ifdef __KERNEL__
> >
> > +#include<linux/compiler.h>
> >   #include<asm/asm-compat.h>
> >
> >   /*
> > @@ -71,7 +72,7 @@
> >   	unreachable();						\
> >   } while (0)
> >
> > -#define BUG_ON(x) do {						\
> > +#define __BUG_ON(x) do {					\
> >   	if (__builtin_constant_p(x)) {				\
> >   		if (x)						\
> >   			BUG();					\
> > @@ -85,6 +86,8 @@
> >   	}							\
> >   } while (0)
> >
> > +#define BUG_ON(x) __BUG_ON(unlikely(x))
> > +
> 
> This is the same type of frobbing you were trying to do to MIPS.
> 
> I will let the powerpc maintainers weigh in on it, but my opinion is 
> that, as with MIPS, BUG_ON() is expanded to a single machine 
> instruction, and this unlikely() business will not change the generated 
> code in any useful way.  It is thus gratuitous code churn and 
> complexification.

What about just doing this:

#define BUG() __builtin_trap()

#define BUG_ON(x) do {	\
	if (x) \
		BUG(); \
} while (0)

GCC should produce better code than forcing it into twnei.  A simple
BUG_ON(x != y) currently generates something like this (GCC 4.3):

xor     r0,r11,r0
addic   r10,r0,-1
subfe   r9,r10,r0
twnei   r9,0

Or this (GCC 4.5):

xor     r0,r11,r0
cntlzw	r0,r0
srwi	r0,r0,5
xori	r0,r0,1
twnei   r0,0

Instead of:

twne	r0,r11

And if GCC doesn't treat code paths that lead to __builtin_trap() as
unlikely (which could make a difference with complex expressions,
even with a conditional trap instruction), that's something that could
and should be fixed in GCC.

The downside is that GCC says, "The mechanism used may vary from
release to release so you should not rely on any particular
implementation."  However, some architectures (sparc, m68k, ia64)
already use __builtin_trap() for this, it seems unlikely that future GCC
versions would switch away from using the trap instruction[1], and there
doesn't seem to be a better-defined way to make GCC generate trap
instructions intelligently.

-Scott

[1] A more likely possibility is that an older compiler just generates a
call to abort() or similar, and later versions implemented trap -- need
to check what the oldest supported GCC does.

Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[Scott Wood]

On Thu, 27 Jan 2011 09:57:39 -0800
David Daney <ddaney@caviumnetworks.com> wrote:

> On 01/27/2011 04:12 AM, Coly Li wrote:
> > diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> > index 065c590..10889a6 100644
> > --- a/arch/powerpc/include/asm/bug.h
> > +++ b/arch/powerpc/include/asm/bug.h
> > @@ -2,6 +2,7 @@
> >   #define _ASM_POWERPC_BUG_H
> >   #ifdef __KERNEL__
> >
> > +#include<linux/compiler.h>
> >   #include<asm/asm-compat.h>
> >
> >   /*
> > @@ -71,7 +72,7 @@
> >   	unreachable();						\
> >   } while (0)
> >
> > -#define BUG_ON(x) do {						\
> > +#define __BUG_ON(x) do {					\
> >   	if (__builtin_constant_p(x)) {				\
> >   		if (x)						\
> >   			BUG();					\
> > @@ -85,6 +86,8 @@
> >   	}							\
> >   } while (0)
> >
> > +#define BUG_ON(x) __BUG_ON(unlikely(x))
> > +
> 
> This is the same type of frobbing you were trying to do to MIPS.
> 
> I will let the powerpc maintainers weigh in on it, but my opinion is 
> that, as with MIPS, BUG_ON() is expanded to a single machine 
> instruction, and this unlikely() business will not change the generated 
> code in any useful way.  It is thus gratuitous code churn and 
> complexification.

What about just doing this:

#define BUG() __builtin_trap()

#define BUG_ON(x) do {	\
	if (x) \
		BUG(); \
} while (0)

GCC should produce better code than forcing it into twnei.  A simple
BUG_ON(x != y) currently generates something like this (GCC 4.3):

xor     r0,r11,r0
addic   r10,r0,-1
subfe   r9,r10,r0
twnei   r9,0

Or this (GCC 4.5):

xor     r0,r11,r0
cntlzw	r0,r0
srwi	r0,r0,5
xori	r0,r0,1
twnei   r0,0

Instead of:

twne	r0,r11

And if GCC doesn't treat code paths that lead to __builtin_trap() as
unlikely (which could make a difference with complex expressions,
even with a conditional trap instruction), that's something that could
and should be fixed in GCC.

The downside is that GCC says, "The mechanism used may vary from
release to release so you should not rely on any particular
implementation."  However, some architectures (sparc, m68k, ia64)
already use __builtin_trap() for this, it seems unlikely that future GCC
versions would switch away from using the trap instruction[1], and there
doesn't seem to be a better-defined way to make GCC generate trap
instructions intelligently.

-Scott

[1] A more likely possibility is that an older compiler just generates a
call to abort() or similar, and later versions implemented trap -- need
to check what the oldest supported GCC does.

Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[David Daney]

On 01/27/2011 12:04 PM, Scott Wood wrote:
> On Thu, 27 Jan 2011 09:57:39 -0800
> David Daney<ddaney@caviumnetworks.com>  wrote:
>
>> On 01/27/2011 04:12 AM, Coly Li wrote:
>>> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
>>> index 065c590..10889a6 100644
>>> --- a/arch/powerpc/include/asm/bug.h
>>> +++ b/arch/powerpc/include/asm/bug.h
>>> @@ -2,6 +2,7 @@
>>>    #define _ASM_POWERPC_BUG_H
>>>    #ifdef __KERNEL__
>>>
>>> +#include<linux/compiler.h>
>>>    #include<asm/asm-compat.h>
>>>
>>>    /*
>>> @@ -71,7 +72,7 @@
>>>    	unreachable();						\
>>>    } while (0)
>>>
>>> -#define BUG_ON(x) do {						\
>>> +#define __BUG_ON(x) do {					\
>>>    	if (__builtin_constant_p(x)) {				\
>>>    		if (x)						\
>>>    			BUG();					\
>>> @@ -85,6 +86,8 @@
>>>    	}							\
>>>    } while (0)
>>>
>>> +#define BUG_ON(x) __BUG_ON(unlikely(x))
>>> +
>>
>> This is the same type of frobbing you were trying to do to MIPS.
>>
>> I will let the powerpc maintainers weigh in on it, but my opinion is
>> that, as with MIPS, BUG_ON() is expanded to a single machine
>> instruction, and this unlikely() business will not change the generated
>> code in any useful way.  It is thus gratuitous code churn and
>> complexification.
>
> What about just doing this:
>
> #define BUG() __builtin_trap()
>
> #define BUG_ON(x) do {	\
> 	if (x) \
> 		BUG(); \
> } while (0)
>
> GCC should produce better code than forcing it into twnei.  A simple
> BUG_ON(x != y) currently generates something like this (GCC 4.3):
>
> xor     r0,r11,r0
> addic   r10,r0,-1
> subfe   r9,r10,r0
> twnei   r9,0
>
> Or this (GCC 4.5):
>
> xor     r0,r11,r0
> cntlzw	r0,r0
> srwi	r0,r0,5
> xori	r0,r0,1
> twnei   r0,0
>
> Instead of:
>
> twne	r0,r11
>
> And if GCC doesn't treat code paths that lead to __builtin_trap() as
> unlikely (which could make a difference with complex expressions,
> even with a conditional trap instruction), that's something that could
> and should be fixed in GCC.
>
> The downside is that GCC says, "The mechanism used may vary from
> release to release so you should not rely on any particular
> implementation."  However, some architectures (sparc, m68k, ia64)
> already use __builtin_trap() for this, it seems unlikely that future GCC
> versions would switch away from using the trap instruction[1], and there
> doesn't seem to be a better-defined way to make GCC generate trap
> instructions intelligently.
>

This is good in theory, however powerpc has this _EMIT_BUG_ENTRY 
business that wouldn't work with __builtin_trap().

David Daney

> -Scott
>
> [1] A more likely possibility is that an older compiler just generates a
> call to abort() or similar, and later versions implemented trap -- need
> to check what the oldest supported GCC does.
>

Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[David Daney]

On 01/27/2011 12:04 PM, Scott Wood wrote:
> On Thu, 27 Jan 2011 09:57:39 -0800
> David Daney<ddaney@caviumnetworks.com>  wrote:
>
>> On 01/27/2011 04:12 AM, Coly Li wrote:
>>> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
>>> index 065c590..10889a6 100644
>>> --- a/arch/powerpc/include/asm/bug.h
>>> +++ b/arch/powerpc/include/asm/bug.h
>>> @@ -2,6 +2,7 @@
>>>    #define _ASM_POWERPC_BUG_H
>>>    #ifdef __KERNEL__
>>>
>>> +#include<linux/compiler.h>
>>>    #include<asm/asm-compat.h>
>>>
>>>    /*
>>> @@ -71,7 +72,7 @@
>>>    	unreachable();						\
>>>    } while (0)
>>>
>>> -#define BUG_ON(x) do {						\
>>> +#define __BUG_ON(x) do {					\
>>>    	if (__builtin_constant_p(x)) {				\
>>>    		if (x)						\
>>>    			BUG();					\
>>> @@ -85,6 +86,8 @@
>>>    	}							\
>>>    } while (0)
>>>
>>> +#define BUG_ON(x) __BUG_ON(unlikely(x))
>>> +
>>
>> This is the same type of frobbing you were trying to do to MIPS.
>>
>> I will let the powerpc maintainers weigh in on it, but my opinion is
>> that, as with MIPS, BUG_ON() is expanded to a single machine
>> instruction, and this unlikely() business will not change the generated
>> code in any useful way.  It is thus gratuitous code churn and
>> complexification.
>
> What about just doing this:
>
> #define BUG() __builtin_trap()
>
> #define BUG_ON(x) do {	\
> 	if (x) \
> 		BUG(); \
> } while (0)
>
> GCC should produce better code than forcing it into twnei.  A simple
> BUG_ON(x != y) currently generates something like this (GCC 4.3):
>
> xor     r0,r11,r0
> addic   r10,r0,-1
> subfe   r9,r10,r0
> twnei   r9,0
>
> Or this (GCC 4.5):
>
> xor     r0,r11,r0
> cntlzw	r0,r0
> srwi	r0,r0,5
> xori	r0,r0,1
> twnei   r0,0
>
> Instead of:
>
> twne	r0,r11
>
> And if GCC doesn't treat code paths that lead to __builtin_trap() as
> unlikely (which could make a difference with complex expressions,
> even with a conditional trap instruction), that's something that could
> and should be fixed in GCC.
>
> The downside is that GCC says, "The mechanism used may vary from
> release to release so you should not rely on any particular
> implementation."  However, some architectures (sparc, m68k, ia64)
> already use __builtin_trap() for this, it seems unlikely that future GCC
> versions would switch away from using the trap instruction[1], and there
> doesn't seem to be a better-defined way to make GCC generate trap
> instructions intelligently.
>

This is good in theory, however powerpc has this _EMIT_BUG_ENTRY 
business that wouldn't work with __builtin_trap().

David Daney

> -Scott
>
> [1] A more likely possibility is that an older compiler just generates a
> call to abort() or similar, and later versions implemented trap -- need
> to check what the oldest supported GCC does.
>

RE: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[David Laight]

 
> > +#define __BUG_ON(x) do {					\
> >   	if (__builtin_constant_p(x)) {				\
> >   		if (x)						\
> >   			BUG();					\
> > @@ -85,6 +86,8 @@
> >   	}							\
> >   } while (0)
> >
> > +#define BUG_ON(x) __BUG_ON(unlikely(x))
> > +

>From my experiments, adding an 'unlikely' at that point isn't
enough for non-trivial conditions - so its presence will
give a false sense the the optimisation is present!
In particular 'if (unlikely(x && y))' needs to be
'if (unlikely(x) && unlikely(y))' in order to avoid
mispredicted branches when 'x' is false.

Also, as (I think) in some of the generated code quoted,
use of __builtin_expect() with a boolean expression can
force some versions of gcc to generate the integer
value of the expression - rather than just selecting the
branch instructions that statically predict the
normal code path.

Sometimes I've also also had to add an asm() statement
that generates no code in order to actually force a
forwards branch (so it has something to place at the
target).

(I've been counting clocks ....)

	David

RE: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[David Laight]

=20
> > +#define __BUG_ON(x) do {					\
> >   	if (__builtin_constant_p(x)) {				\
> >   		if (x)						\
> >   			BUG();					\
> > @@ -85,6 +86,8 @@
> >   	}							\
> >   } while (0)
> >
> > +#define BUG_ON(x) __BUG_ON(unlikely(x))
> > +

>From my experiments, adding an 'unlikely' at that point isn't
enough for non-trivial conditions - so its presence will
give a false sense the the optimisation is present!
In particular 'if (unlikely(x && y))' needs to be
'if (unlikely(x) && unlikely(y))' in order to avoid
mispredicted branches when 'x' is false.

Also, as (I think) in some of the generated code quoted,
use of __builtin_expect() with a boolean expression can
force some versions of gcc to generate the integer
value of the expression - rather than just selecting the
branch instructions that statically predict the
normal code path.

Sometimes I've also also had to add an asm() statement
that generates no code in order to actually force a
forwards branch (so it has something to place at the
target).

(I've been counting clocks ....)

	David

Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[Andreas Schwab]

"David Laight" <David.Laight@ACULAB.COM> writes:

> Also, as (I think) in some of the generated code quoted,
> use of __builtin_expect() with a boolean expression can
> force some versions of gcc to generate the integer
> value of the expression

That's more likely a side effect of the definition of likely/unlikely:
they expand to !!(x).

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[Andreas Schwab]

"David Laight" <David.Laight@ACULAB.COM> writes:

> Also, as (I think) in some of the generated code quoted,
> use of __builtin_expect() with a boolean expression can
> force some versions of gcc to generate the integer
> value of the expression

That's more likely a side effect of the definition of likely/unlikely:
they expand to !!(x).

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: [PATCH 1/7] MIPS: add unlikely() to BUG_ON()

[Coly Li]

On 2011年01月28日 01:50, David Daney Wrote:
> Please Cc: linux-mips@linux-mips.org for MIPS patches.
>
> On 01/27/2011 04:12 AM, Coly Li wrote:
>> Current BUG_ON() in arch/mips/include/asm/bug.h does not use unlikely(),
>> in order to get better branch predict result, source code should call
>> BUG_ON() with unlikely() explicitly. This is not a suggested method to
>> use BUG_ON().
>>
>> This patch adds unlikely() inside BUG_ON implementation on MIPS code,
>> callers can use BUG_ON without explicit unlikely() now.
>>
>> I have no usable MIPS hardware to build and test the fix, any test result
>> of this patch is welcome.
>>
>> Signed-off-by: Coly Li<bosong.ly@taobao.com>
>> Cc: David Daney<ddaney@caviumnetworks.com>
>> Cc: Wang Cong<xiyou.wangcong@gmail.com>
>> Cc: Yong Zhang<yong.zhang0@gmail.com>
>> ---
>> arch/mips/include/asm/bug.h | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/mips/include/asm/bug.h b/arch/mips/include/asm/bug.h
>> index 540c98a..6771c07 100644
>> --- a/arch/mips/include/asm/bug.h
>> +++ b/arch/mips/include/asm/bug.h
>> @@ -30,7 +30,7 @@ static inline void __BUG_ON(unsigned long condition)
>> : : "r" (condition), "i" (BRK_BUG));
>> }
>>
>> -#define BUG_ON(C) __BUG_ON((unsigned long)(C))
>> +#define BUG_ON(C) __BUG_ON((unsigned long)unlikely(C))
>>
>
> NAK.
>
> __BUG_ON() expands to a single instruction. Frobbing about with unlikely() will have no effect on the generated code and
> is thus gratuitous code churn.
>

Since unlikely() in arch implemented BUG_ON() is gratuitous, using unlikely() in kernel code like BUG_ON(unlikely(...)) 
for arch implemented BUG_ON is unwelcome neither.

The NAK makes sense, thanks for your reply.

Coly

-- 
Coly Li

Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[Coly Li]

On 2011年01月28日 18:14, Andreas Schwab Wrote:
> "David Laight"<David.Laight@ACULAB.COM>  writes:
>
>> Also, as (I think) in some of the generated code quoted,
>> use of __builtin_expect() with a boolean expression can
>> force some versions of gcc to generate the integer
>> value of the expression
>
> That's more likely a side effect of the definition of likely/unlikely:
> they expand to !!(x).
>

It seems whether or not using unlikely() inside arch implemented BUG_ON() is arch dependent. Maybe a reasonable method 
to use BUG_ON() is,
1) do not explicitly use unlikely() when using macro BUG_ON().
2) whether or not using unlikely() inside BUG_ON(), it depends on the implementation of BUG_ON(), including arch 
implementation.

So from current feed back, doing "unlikely() optimization" here doesn't make anything better.

Thanks for all of your feed back :-)

-- 
Coly Li

Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()

[Coly Li]

On 2011年01月28日 18:14, Andreas Schwab Wrote:
> "David Laight"<David.Laight@ACULAB.COM>  writes:
>
>> Also, as (I think) in some of the generated code quoted,
>> use of __builtin_expect() with a boolean expression can
>> force some versions of gcc to generate the integer
>> value of the expression
>
> That's more likely a side effect of the definition of likely/unlikely:
> they expand to !!(x).
>

It seems whether or not using unlikely() inside arch implemented BUG_ON() is arch dependent. Maybe a reasonable method 
to use BUG_ON() is,
1) do not explicitly use unlikely() when using macro BUG_ON().
2) whether or not using unlikely() inside BUG_ON(), it depends on the implementation of BUG_ON(), including arch 
implementation.

So from current feed back, doing "unlikely() optimization" here doesn't make anything better.

Thanks for all of your feed back :-)

-- 
Coly Li