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

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 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