Re: acpi throttling: Use this_cpu_has and simplify code

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@linux.com>
> 
> ---
>  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@kernel.org>

I think this one too fits the x86 tree better.

Thanks.

-- 
tejun

Re: acpi throttling: Use this_cpu_has and simplify code

[ykzhao]

On Sat, 2010-12-18 at 23:50 +0800, Tejun Heo wrote:
> (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@linux.com>
> > 
> > ---
> >  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.

The above function may be called under the scenario with irq disabled.
In such case if the corresponding MRS is accessed by using remote
method, it will complain the oops. Maybe it will be safer to firstly
switch to the local CPU and then read/write the MRS register related
with the throttling.

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

It is also ok to me that drops the pr input argument of
acpi_throttling_rdmsr/wrmsr as it is already switched to the local cpu.

> 
> Anyways, the above doesn't make the situation any worse, so...
> 
>  Acked-by: Tejun Heo <tj@kernel.org>
> 
> I think this one too fits the x86 tree better.
> 
> Thanks.
> 

Re: acpi throttling: Use this_cpu_has and simplify code

[Christoph Lameter]

On Sat, 18 Dec 2010, Tejun Heo wrote:

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

Right.

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

That is precisely the difficulty with many other functions that take
pointers to cpu_info. If we can establish that they are called *only* with
pointers to the current cpu then we can improve the function by dropping
the parameter and using this_cpu ops to access the cpu_info fields.

Using this_cpu ops instead of access to a field of a struct pointed to by
an array avoids an addition of pointers and replacest with a operation
that essentially takes a global address with a segment prefix. It reduces
register pressure and avoids the additions of pointers but it will encode
the address in the instruction.