Re: [PATCH 2/5] powernow-k8: Add core performance boost support

[Thomas Renninger <trenn@suse.de>]

On Monday 22 March 2010 19:38:38 Borislav Petkov wrote:
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> Starting with F10h, revE, AMD processors add support for a dynamic
> core boosting feature called Core Performance Boost. When a specific
> condition is present, a subset of the cores on a system are boosted
> beyond their P0 operating frequency to speed up the performance of
> single-threaded applications.
> 
> In the normal case, the system comes out of reset with core boosting
> enabled. This patch adds a sysfs knob with which core boosting can be
> switched on or off for benchmarking purposes.
> 
> While at it, cleanup the driver init codepath and update copyrights.
> 
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  arch/x86/kernel/cpu/cpufreq/powernow-k8.c |  115 ++++++++++++++++++++++++++---
>  arch/x86/kernel/cpu/cpufreq/powernow-k8.h |    3 +-
>  2 files changed, 106 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> index d360b56..90fda2c 100644
> --- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> +++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> @@ -1,6 +1,5 @@
> -
>  /*
> - *   (c) 2003-2006 Advanced Micro Devices, Inc.
> + *   (c) 2003-2010 Advanced Micro Devices, Inc.
>   *  Your use of this code is subject to the terms and conditions of the
>   *  GNU general public license version 2. See "COPYING" or
>   *  http://www.gnu.org/licenses/gpl.html
> @@ -54,6 +53,10 @@ static DEFINE_PER_CPU(struct powernow_k8_data *, powernow_data);
>  
>  static int cpu_family = CPU_OPTERON;
>  
> +/* core performance boost */
> +static bool cpb_capable, cpb_disabled;
Whatabout using a cpufeature (arch/x86/include/asm/cpufeature.h)
instead of cpb_capable. Then people could see this feature in 
/proc/cpuinfo and other code parts could check for it easily if
needed later.
It could already be set in arch/x86/kernel/cpu/amd.c and
powernow-k8 could use cpu_has(cpu, X86_FEATURE_CPB);

Instead of cpb_disabled, I'd use cpb_enabled. Checking for
!cpb_disabled whether it's enabled, is ugly to read.

> +static struct msr __percpu *msrs;
> +
>  #ifndef CONFIG_SMP
>  static inline const struct cpumask *cpu_core_mask(int cpu)
>  {
> @@ -1393,8 +1396,69 @@ out:
>  	return khz;
>  }
>  
> +static void _cpb_toggle_msrs(bool t)
> +{
> +	int cpu;
> +
> +	rdmsr_on_cpus(cpu_online_mask, MSR_K7_HWCR, msrs);
> +
> +	for_each_cpu(cpu, cpu_online_mask) {
> +		struct msr *reg = per_cpu_ptr(msrs, cpu);
> +		if (t)
> +			reg->l &= ~BIT(25);
> +		else
> +			reg->l |= BIT(25);
> +	}
> +	wrmsr_on_cpus(cpu_online_mask, MSR_K7_HWCR, msrs);
> +}
> +
> +/*
> + * Switch on/off core performance boosting.
> + *
> + * 0=disable
> + * 1=enable.
> + */
> +static void cpb_toggle(bool t)
> +{
> +	if (!cpb_capable)
> +		return;
> +
> +	if (t && cpb_disabled) {
> +		cpb_disabled = false;
cpb_enabled = true
is better.
> +		_cpb_toggle_msrs(t);
> +		printk(KERN_INFO PFX "Core Boosting enabled.\n");
Always printk on every toggle?
That should not happen often and a user might want to get noticed if
an app does this behind his back -> should be fine w/ or w/o, just not 
sure whether it's intended.

...

     Thomas