Re: [PATCH for-4.8 V2 08/10] powerpc: use the jump label for cpu_has_feature

[Kevin Hao <haokexin@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2765 bytes --]

On Mon, Jul 25, 2016 at 04:28:49PM +1000, Nicholas Piggin wrote:
> On Sat, 23 Jul 2016 14:42:41 +0530
> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> wrote:
> 
> > From: Kevin Hao <haokexin@gmail.com>
> > 
> > The cpu features are fixed once the probe of cpu features are done.
> > And the function cpu_has_feature() does be used in some hot path.
> > The checking of the cpu features for each time of invoking of
> > cpu_has_feature() seems suboptimal. This tries to reduce this
> > overhead of this check by using jump label.
> > 
> > The generated assemble code of the following c program:
> > 	if (cpu_has_feature(CPU_FTR_XXX))
> > 		xxx()
> > 
> > Before:
> > 	lis     r9,-16230
> > 	lwz     r9,12324(r9)
> > 	lwz     r9,12(r9)
> > 	andi.   r10,r9,512
> > 	beqlr-
> > 
> > After:
> > 	nop	if CPU_FTR_XXX is enabled
> > 	b xxx	if CPU_FTR_XXX is not enabled
> > 
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/include/asm/cpufeatures.h | 21 +++++++++++++++++++++
> >  arch/powerpc/include/asm/cputable.h    |  8 ++++++++
> >  arch/powerpc/kernel/cputable.c         | 20 ++++++++++++++++++++
> >  arch/powerpc/lib/feature-fixups.c      |  1 +
> >  4 files changed, 50 insertions(+)
> > 
> > diff --git a/arch/powerpc/include/asm/cpufeatures.h
> > b/arch/powerpc/include/asm/cpufeatures.h index
> > bfa6cb8f5629..4a4a0b898463 100644 ---
> > a/arch/powerpc/include/asm/cpufeatures.h +++
> > b/arch/powerpc/include/asm/cpufeatures.h @@ -13,10 +13,31 @@ static
> > inline bool __cpu_has_feature(unsigned long feature)
> > return !!(CPU_FTRS_POSSIBLE & cur_cpu_spec->cpu_features & feature); }
> >  
> > +#ifdef CONFIG_JUMP_LABEL
> > +#include <linux/jump_label.h>
> > +
> > +extern struct static_key_true cpu_feat_keys[MAX_CPU_FEATURES];
> > +
> > +static __always_inline bool cpu_has_feature(unsigned long feature)
> > +{
> > +	int i;
> > +
> > +	if (CPU_FTRS_ALWAYS & feature)
> > +		return true;
> > +
> > +	if (!(CPU_FTRS_POSSIBLE & feature))
> > +		return false;
> > +
> > +	i = __builtin_ctzl(feature);
> > +	return static_branch_likely(&cpu_feat_keys[i]);
> > +}
> 
> Is feature ever not-constant, or could it ever be, I wonder? We could
> do a build time check to ensure it is always constant?

In the current code, all the using of this function are passing a constant
argument. But yes, due to the implementation of jump label, we should add
a check here to ensure that a constant is passed to this function. Something
likes this:

	if (!__builtin_constant_p(feature))
		return __cpu_has_feature(feature);

We need the same change for the mmu_has_feature().

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]