linux-kernel - Re: acpi throttling: Use this_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4D0CD840.2050100@gmail.com>
Date:	Sat, 18 Dec 2010 16:50:24 +0100
From:	Tejun Heo <htejun@...il.com>
To:	Christoph Lameter <cl@...ux.com>
CC:	"H. Peter Anvin" <hpa@...or.com>,
	Eric Dumazet <eric.dumazet@...il.com>,
	akpm@...ux-foundation.org, Pekka Enberg <penberg@...helsinki.fi>,
	linux-kernel@...r.kernel.org,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	rui.zhang@...el.com, lenb@...nel.org, linux-acpi@...r.kernel.org
Subject: Re: acpi throttling: Use this_cpu_has and simplify code

(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@...ux.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@...nel.org>

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/