Re: acpi throttling: Use this_cpu_has and simplify code

[Tejun Heo]

(cc'ing ACPI ppl and quoting the whole body)

On 12/16/2010 07:16 PM, Christoph Lameter wrote:
> 
> With the this_cpu_xx we no longer need to pass an acpi
> structure to the msr management code. Simplifies code and improves
> performance.
> 
> Signed-off-by: Christoph Lameter <cl@xxxxxxxxx>
> 
> ---
>  drivers/acpi/processor_throttling.c |   32 ++++++++++----------------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/processor_throttling.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/processor_throttling.c	2010-12-16 11:56:57.000000000 -0600
> +++ linux-2.6/drivers/acpi/processor_throttling.c	2010-12-16 12:00:17.000000000 -0600
> @@ -662,20 +662,14 @@ static int acpi_processor_get_throttling
>  }
> 
>  #ifdef CONFIG_X86
> -static int acpi_throttling_rdmsr(struct acpi_processor *pr,
> -					u64 *value)
> +static int acpi_throttling_rdmsr(u64 *value)
>  {
> -	struct cpuinfo_x86 *c;
>  	u64 msr_high, msr_low;
> -	unsigned int cpu;
>  	u64 msr = 0;
>  	int ret = -1;
> 
> -	cpu = pr->id;
> -	c = &cpu_data(cpu);
> -
> -	if ((c->x86_vendor != X86_VENDOR_INTEL) ||
> -		!cpu_has(c, X86_FEATURE_ACPI)) {
> +	if ((this_cpu_read(cpu_info.x86_vendor) != X86_VENDOR_INTEL) ||
> +		!this_cpu_has(X86_FEATURE_ACPI)) {
>  		printk(KERN_ERR PREFIX
>  			"HARDWARE addr space,NOT supported yet\n");
>  	} else {
> @@ -690,18 +684,13 @@ static int acpi_throttling_rdmsr(struct
>  	return ret;
>  }
> 
> -static int acpi_throttling_wrmsr(struct acpi_processor *pr, u64 value)
> +static int acpi_throttling_wrmsr(u64 value)
>  {
> -	struct cpuinfo_x86 *c;
> -	unsigned int cpu;
>  	int ret = -1;
>  	u64 msr;
> 
> -	cpu = pr->id;
> -	c = &cpu_data(cpu);
> -
> -	if ((c->x86_vendor != X86_VENDOR_INTEL) ||
> -		!cpu_has(c, X86_FEATURE_ACPI)) {
> +	if ((this_cpu_read(cpu_info.x86_vendor) != X86_VENDOR_INTEL) ||
> +		!this_cpu_has(X86_FEATURE_ACPI)) {
>  		printk(KERN_ERR PREFIX
>  			"HARDWARE addr space,NOT supported yet\n");
>  	} else {
> @@ -713,15 +702,14 @@ static int acpi_throttling_wrmsr(struct
>  	return ret;
>  }
>  #else
> -static int acpi_throttling_rdmsr(struct acpi_processor *pr,
> -				u64 *value)
> +static int acpi_throttling_rdmsr(u64 *value)
>  {
>  	printk(KERN_ERR PREFIX
>  		"HARDWARE addr space,NOT supported yet\n");
>  	return -1;
>  }
> 
> -static int acpi_throttling_wrmsr(struct acpi_processor *pr, u64 value)
> +static int acpi_throttling_wrmsr(u64 value)
>  {
>  	printk(KERN_ERR PREFIX
>  		"HARDWARE addr space,NOT supported yet\n");
> @@ -753,7 +741,7 @@ static int acpi_read_throttling_status(s
>  		ret = 0;
>  		break;
>  	case ACPI_ADR_SPACE_FIXED_HARDWARE:
> -		ret = acpi_throttling_rdmsr(pr, value);
> +		ret = acpi_throttling_rdmsr(value);
>  		break;
>  	default:
>  		printk(KERN_ERR PREFIX "Unknown addr space %d\n",
> @@ -786,7 +774,7 @@ static int acpi_write_throttling_state(s
>  		ret = 0;
>  		break;
>  	case ACPI_ADR_SPACE_FIXED_HARDWARE:
> -		ret = acpi_throttling_wrmsr(pr, value);
> +		ret = acpi_throttling_wrmsr(value);
>  		break;
>  	default:
>  		printk(KERN_ERR PREFIX "Unknown addr space %d\n",
> 

It's bothersome that these methods don't have any indication that
they're bound to local CPU when they can't be called with @pr for
another CPU as MSRs can only be accessed from local CPU.

In the longer run, it would be nice if there's an indication that this
is only for the local CPU and maybe a WARN_ON_ONCE().  Maybe dropping
@pr and using this_cpu_*() is better for performance too?

Anyways, the above doesn't make the situation any worse, so...

 Acked-by: Tejun Heo <tj@xxxxxxxxxx>

I think this one too fits the x86 tree better.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/