Cmpxchg and xchg operations

Use cmpxchg instead of xchg to realize this_cpu_xchg.

xchg will cause LOCK overhead since LOCK is always implied but cmpxchg will not.

Baselines:

xchg() = 18 cycles (no segment prefix, LOCK semantics)
__this_cpu_xchg = 1 cycle

(simulated using this_cpu_read/write, two prefixes. Looks like the cpu can use loop optimization to get rid of most of the overhead)

Cycles before:

this_cpu_xchg = 37 cycles (segment prefix and LOCK (implied by xchg))

After:

this_cpu_xchg = 11 cycle (using cmpxchg without lock semantics)

Signed-off-by Christoph Lameter <[email protected]>

---
 arch/x86/include/asm/percpu.h |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Index: linux-2.6/arch/x86/include/asm/percpu.h
===================================================================

arch/x86/include/asm/percpu.h +15 -6

@@ -213,8 +213,9 @@ do {									\
 })
 
 /*
- * Beware: xchg on x86 has an implied lock prefix. There will be the cost of
- * full lock semantics even though they are not needed.
+ * xchg is implemented using cmpxchg without a lock prefix. xchg is
+ * expensive due to the implied lock prefix. The processor cannot prefetch
+ * cachelines if xchg is used.
  */
 #define percpu_xchg_op(var, nval)					\
 ({									\

@@ -222,25 +223,33 @@ do {									\
 	typeof(var) __new = (nval);					\
 	switch (sizeof(var)) {						\
 	case 1:								\
-		asm("xchgb %2, "__percpu_arg(1)			\
+		asm("\n1:mov "__percpu_arg(1)",%%al"			\
+		    "\n\tcmpxchgb %2, "__percpu_arg(1)			\
+		    "\n\tjnz 1b"					\
 			    : "=a" (__ret), "+m" (var)			\
 			    : "q" (__new)				\
 			    : "memory");				\
 		break;							\
 	case 2:								\
-		asm("xchgw %2, "__percpu_arg(1)			\
+		asm("\n1:mov "__percpu_arg(1)",%%ax"			\
+		    "\n\tcmpxchgw %2, "__percpu_arg(1)			\
+		    "\n\tjnz 1b"					\
 			    : "=a" (__ret), "+m" (var)			\
 			    : "r" (__new)				\
 			    : "memory");				\
 		break;							\
 	case 4:								\
-		asm("xchgl %2, "__percpu_arg(1)			\
+		asm("\n1:mov "__percpu_arg(1)",%%eax"			\
+		    "\n\tcmpxchgl %2, "__percpu_arg(1)			\
+		    "\n\tjnz 1b"					\
 			    : "=a" (__ret), "+m" (var)			\
 			    : "r" (__new)				\
 			    : "memory");				\
 		break;							\
 	case 8:								\
-		asm("xchgq %2, "__percpu_arg(1)			\
+		asm("\n1:mov "__percpu_arg(1)",%%rax"			\
+		    "\n\tcmpxchgq %2, "__percpu_arg(1)			\
+		    "\n\tjnz 1b"					\
 			    : "=a" (__ret), "+m" (var)			\
 			    : "r" (__new)				\
 			    : "memory");				\

pièce jointe document texte brut (cpuops_xchg_with_cmpxchg) Use cmpxchg instead of xchg to realize this_cpu_xchg.

xchg will cause LOCK overhead since LOCK is always implied but cmpxchg will not.

Baselines:

xchg() = 18 cycles (no segment prefix, LOCK semantics)
__this_cpu_xchg = 1 cycle

(simulated using this_cpu_read/write, two prefixes. Looks like the cpu can use loop optimization to get rid of most of the overhead)

Cycles before:

this_cpu_xchg = 37 cycles (segment prefix and LOCK (implied by xchg))

After:

this_cpu_xchg = 11 cycle (using cmpxchg without lock semantics)

Signed-off-by Christoph Lameter <[email protected]>

---
 arch/x86/include/asm/percpu.h |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Index: linux-2.6/arch/x86/include/asm/percpu.h
===================================================================

arch/x86/include/asm/percpu.h +3 -2

@@ -213,8 +213,9 @@ do {									\
 })
 
 /*
- * Beware: xchg on x86 has an implied lock prefix. There will be the cost of
- * full lock semantics even though they are not needed.
+ * xchg is implemented using cmpxchg without a lock prefix. xchg is
+ * expensive due to the implied lock prefix. The processor cannot prefetch
+ * cachelines if xchg is used.
  */
 #define percpu_xchg_op(var, nval)					\
 ({									\

arch/x86/include/asm/percpu.h +9 -3

 			    : "=a" (__ret), "+m" (var)			\
 			    : "q" (__new)				\
 			    : "memory");				\
 		break;							\
 	case 2:								\
-		asm("xchgw %2, "__percpu_arg(1)			\
+		asm("\n1:mov "__percpu_arg(1)",%%ax"			\
+		    "\n\tcmpxchgw %2, "__percpu_arg(1)			\
+		    "\n\tjnz 1b"					\
 			    : "=a" (__ret), "+m" (var)			\
 			    : "r" (__new)				\
 			    : "memory");				\
 		break;							\
 	case 4:								\
-		asm("xchgl %2, "__percpu_arg(1)			\
+		asm("\n1:mov "__percpu_arg(1)",%%eax"			\
+		    "\n\tcmpxchgl %2, "__percpu_arg(1)			\
+		    "\n\tjnz 1b"					\
 			    : "=a" (__ret), "+m" (var)			\
 			    : "r" (__new)				\
 			    : "memory");				\
 		break;							\
 	case 8:								\
-		asm("xchgq %2, "__percpu_arg(1)			\
+		asm("\n1:mov "__percpu_arg(1)",%%rax"			\
+		    "\n\tcmpxchgq %2, "__percpu_arg(1)			\
+		    "\n\tjnz 1b"					\
 			    : "=a" (__ret), "+m" (var)			\
 			    : "r" (__new)				\
 			    : "memory");				\

You should use the fact that the failed cmpxchg loads in al/ax/eax/rax the current value, so :

	"\n\tmov "__percpu_arg(1)",%%al"
	"\n1:\tcmpxchgb %2, "__percpu_arg(1)
	"\n\tjnz 1b"

(No need to reload the value again)

We do not need to reload the value as pointed out by Eric. It is already in the correct register so just rerun the cmpxchg without the load.

Signed-off-by Christoph Lameter <[email protected]>

Quoted 6 lines · Christoph Lameter

We do not need to reload the value as pointed out by Eric. It is already in the correct register so just rerun the cmpxchg without the load.

Signed-off-by Christoph Lameter <[email protected]>

Is it genuinely faster to do the pre-load mov, or can we drop that too?
My guess would be that yes it is, but if it happens not to be it would
be nice to reduce the code size.

Quoted 4 lines

Is it genuinely faster to do the pre-load mov, or can we drop that too?
My guess would be that yes it is, but if it happens not to be it would
be nice to reduce the code size.

Dropping the load increases the cycle count from 11 to 16.

Quoted 6 lines · Christoph Lameter

Quoted 4 lines

Is it genuinely faster to do the pre-load mov, or can we drop that too?
My guess would be that yes it is, but if it happens not to be it would
be nice to reduce the code size.

Dropping the load increases the cycle count from 11 to 16.

Great, that answers that! I'll pick up the patch hopefully today (I'm finally ramping back up on arch/x86 again after having been diverted to an internal project for a while...)

Quoted 3 lines

Great, that answers that! I'll pick up the patch hopefully today (I'm finally ramping back up on arch/x86 again after having been diverted to an internal project for a while...)

How do you want to route these? All patches before this series is already in the percpu tree. I can pull the generic bits and leave out the x86 bits so that x86 tree can pull in percpu bits and then put x86 stuff on top of it. If you wanna go that way, I would drop all x86 related patches from the previous patchsets too.

Hello,

Quoted 11 lines · H. Peter Anvin

Quoted 6 lines · Christoph Lameter

Quoted 4 lines

Is it genuinely faster to do the pre-load mov, or can we drop that too?
My guess would be that yes it is, but if it happens not to be it would
be nice to reduce the code size.

Dropping the load increases the cycle count from 11 to 16.

Great, that answers that! I'll pick up the patch hopefully today (I'm finally ramping back up on arch/x86 again after having been diverted to an internal project for a while...)

How do you want to route these? All patches before this series is already in the percpu tree. I can pull the generic bits and leave out the x86 bits so that x86 tree can pull in percpu bits and then put x86 stuff on top of it. If you wanna go that way, I would drop all x86 related patches from the previous patchsets too.

It's probably easiest if you just add them with my Acked-by: then. Alternatively, I could pick them up so they go into -tip and the tip test machinery.

The irq work queue is a per cpu object and it is sufficient for synchronization if per cpu atomics are used. Doing so simplifies the code and reduces the overhead of the code.

Before:

[email protected]$ size kernel/irq_work.o
   text	   data	    bss	    dec	    hex	filename
    451	      8	      1	    460	    1cc	kernel/irq_work.o

After:

[email protected]$ size kernel/irq_work.o 
   text	   data	    bss	    dec	    hex	filename
    438	      8	      1	    447	    1bf	kernel/irq_work.o

Cc Peter Zijlstra <[email protected]>

Quoted 16 lines · Christoph Lameter

The irq work queue is a per cpu object and it is sufficient for synchronization if per cpu atomics are used. Doing so simplifies the code and reduces the overhead of the code.

Before:

[email protected]$ size kernel/irq_work.o
   text	   data	    bss	    dec	    hex	filename
    451	      8	      1	    460	    1cc	kernel/irq_work.o

After:

[email protected]$ size kernel/irq_work.o 
   text	   data	    bss	    dec	    hex	filename
    438	      8	      1	    447	    1bf	kernel/irq_work.o

Quoted 1 line

Cc Peter Zijlstra <[email protected]>

Peter, can you please ack this one?

Thanks.

Hello,

Quoted 3 lines · H. Peter Anvin

It's probably easiest if you just add them with my Acked-by: then. Alternatively, I could pick them up so they go into -tip and the tip test machinery.

All patches accepted into percpu till now are in the following branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/percpu.git for-next

The tree is based on v2.6.37-rc5 and v2.6.37-rc5..for-next would give 21 patches. I think most of these don't fit x86 tree but there are several which would fit there much better. d80aadf9 (x86: Replace uses of current_cpu_data with this_cpu ops) and e5195e91 (x86: Use this_cpu_inc_return for nmi counter).

As it would be beneficial to push these through -tip testing anyway, how about the following?

* If you review and ack the x86 related bits in the series, I'll regenerated and add the ACKs.

* It would be better if the two commits mentioned above get routed through x86 tree rather than percpu tree, so I'll drop the above two from percpu tree and you can pull percpu into x86 and then apply those in x86 tree.

I think we can do that... let me look through the tree first officially committing. Alternatively, I can pull the patches from your tree and individually review them and add them to a tip branch, if you prefer.

Currently the operations to increment vm counters must disable interrupts in order to not mess up their housekeeping of counters.

So use this_cpu_cmpxchg() to avoid the overhead. Since we can no longer count on preremption being disabled we still have some minor issues. The fetching of the counter thresholds is racy.
A threshold from another cpu may be applied if we happen to be rescheduled on another cpu. However, the following vmstat operation will then bring the counter again under the threshold limit.

The operations for __xxx_zone_state are not changed since the caller has taken care of the synchronization needs (and therefore the cycle count is even less than the optimized version for the irq disable case provided here).

The optimization using this_cpu_cmpxchg will only be used if the arch supports efficient this_cpu_ops (must have CONFIG_CMPXCHG_LOCAL set!)

The use of this_cpu_cmpxchg reduces the cycle count for the counter operations by %80 (inc_zone_page_state goes from 170 cycles to 32).

Signed-off-by Christoph Lameter <[email protected]>

Use cmpxchg instead of xchg to realize this_cpu_xchg.

xchg will cause LOCK overhead since LOCK is always implied but cmpxchg will not.

Baselines:

xchg() = 18 cycles (no segment prefix, LOCK semantics)
__this_cpu_xchg = 1 cycle

(simulated using this_cpu_read/write, two prefixes. Looks like the cpu can use loop optimization to get rid of most of the overhead)

Cycles before:

this_cpu_xchg = 37 cycles (segment prefix and LOCK (implied by xchg))

After:

this_cpu_xchg = 11 cycle (using cmpxchg without lock semantics)

Signed-off-by Christoph Lameter <[email protected]>

Quoted 12 lines · Christoph Lameter

The irq work queue is a per cpu object and it is sufficient for synchronization if per cpu atomics are used. Doing so simplifies the code and reduces the overhead of the code.

Before:

[email protected]$ size kernel/irq_work.o
   text	   data	    bss	    dec	    hex	filename
    451	      8	      1	    460	    1cc	kernel/irq_work.o

After:

Quoted 5 lines

[email protected]$ size kernel/irq_work.o 
   text	   data	    bss	    dec	    hex	filename
    438	      8	      1	    447	    1bf	kernel/irq_work.o

Cc Peter Zijlstra <[email protected]>

Peter, can you please ack this one?

(quoted diff) +7 -0

+		/*
+		 * The fetching of the stat_threshold is racy. We may apply
+		 * a counter threshold to the wrong the cpu if we get
+		 * rescheduled while executing here. However, the following
+		 * will apply the threshold again and therefore bring the
+		 * counter under the threshold.
+		 */

What does "the following" mean here? Later executions of the function? It seems like the counter can go out of the threshold at least temporarily, which probably is okay but I think the comment can be improved a bit.

Quoted 19 lines · Tejun Heo

Quoted 17 lines · Christoph Lameter

The irq work queue is a per cpu object and it is sufficient for synchronization if per cpu atomics are used. Doing so simplifies the code and reduces the overhead of the code.

Before:

[email protected]$ size kernel/irq_work.o
   text	   data	    bss	    dec	    hex	filename
    451	      8	      1	    460	    1cc	kernel/irq_work.o

After:

[email protected]$ size kernel/irq_work.o
   text	   data	    bss	    dec	    hex	filename
    438	      8	      1	    447	    1bf	kernel/irq_work.o

Cc Peter Zijlstra <[email protected]>

Quoted 1 line

Peter, can you please ack this one?

I guess so, I don't much like the bare preempt_disable/enable there, and I'm wondering, aren't %fs prefixed insn slower than regular insn? Does it really pay to avoid this one address computation if there's multiple users in a function. %fs prefixes do take another byte, so it will also result in larger code at some point.

Prefixes are faster than explicit address calculations. A prefix allows you to integrate the per cpu address calculation into an arithmetic operation.

A prefix is one byte which is less that multiple arithmetic operations to calculate an address.

I am not sure that the preempt_disable/enable is needed. They are just there because you had a get/put_cpu there.

If the code is run from hardirq context then preempt is already disabled. We can just drop those then.

Quoted 3 lines · Christoph Lameter

Prefixes are faster than explicit address calculations. A prefix allows you to integrate the per cpu address calculation into an arithmetic operation.

Well, depends on how often you need that address I'd think. If you'd have a per-cpu struct and need to frob lots of variables in that struct it might be cheaper to simply compute the struct address once and then use relative addresses than to prefix everything with %fs.

Quoted 2 lines

A prefix is one byte which is less that multiple arithmetic operations to calculate an address.

I thought you'd only need a single arithmetic op to calculate the address, anyway at some point those 1 byte prefixes will add up to more than the ops saved.

Still, non of this is really fast-path code, so I really wonder why we're optimizing this over keeping the code obvious.

I'd prefer percpu things going through percpu tree, if for nothing else for git history's sake, but I don't think it really matters. The series is spread all over the place anyway. As long as each maintainer is properly alerted about the changes, it should be okay. Please let me know whether you agree with the changes currently queued in percpu#for-next. I'll update the tree with your Acked-by's and freeze it.

Are you okay with the patches currently in percpu#for-next? If so, I'll regenerate patches with your acked-by and pop the two previously mentioned commits and proceed with the rest of the series.

Thank you.

Quoted 4 lines · Tejun Heo

Are you okay with the patches currently in percpu#for-next? If so, I'll regenerate patches with your acked-by and pop the two previously mentioned commits and proceed with the rest of the series.

Still looking through them (sorry.) I note that we probably do need to get Christoph's followup patches into -tip, so we need to get it all into tip; as such, even if it goes through your tree I'll need to pull it into a tip branch.

Hey, again.

Quoted 7 lines · Tejun Heo

I'd prefer percpu things going through percpu tree, if for nothing else for git history's sake, but I don't think it really matters. The series is spread all over the place anyway. As long as each maintainer is properly alerted about the changes, it should be okay. Please let me know whether you agree with the changes currently queued in percpu#for-next. I'll update the tree with your Acked-by's and freeze it.

Are you okay with the patches currently in percpu#for-next? If so, I'll regenerate patches with your acked-by and pop the two previously mentioned commits and proceed with the rest of the series.

Index: linux-2.6/include/linux/percpu.h
===================================================================

include/linux/percpu.h +4 -4

@@ -242,21 +242,21 @@ extern void __bad_size_call_parameter(vo
 
 #define __pcpu_size_call_return2(stem, pcp, ...)			\
 ({									\
-	typeof(pcp) ret__;						\
+	typeof(pcp) pscr2_ret__;					\
 	__verify_pcpu_ptr(&(pcp));					\
 	switch(sizeof(pcp)) {						\
-	case 1: ret__ = stem##1(pcp, __VA_ARGS__);			\
+	case 1: pscr2_ret__ = stem##1(pcp, __VA_ARGS__);		\
 		break;							\
-	case 2: ret__ = stem##2(pcp, __VA_ARGS__);			\
+	case 2: pscr2_ret__ = stem##2(pcp, __VA_ARGS__);		\
 		break;							\
-	case 4: ret__ = stem##4(pcp, __VA_ARGS__);			\
+	case 4: pscr2_ret__ = stem##4(pcp, __VA_ARGS__);		\

include/linux/percpu.h +0 -0

  * IRQ safe versions of the per cpu RMW operations. Note that these operations
  * are *not* safe against modification of the same variable from another

* processors (which one gets when using regular atomic operations)
- . They are guaranteed to be atomic vs. local interrupts and
+ * They are guaranteed to be atomic vs. local interrupts and

Provide support as far as the hardware capabilities of the x86 cpus allow.

Define CONFIG_CMPXCHG_LOCAL in Kconfig.cpu to allow core code to test for fast cpuops implementations.

V1->V2:
- Take out the definition for this_cpu_cmpxchg_8 and move it into a separate patch.

Signed-off-by Christoph Lameter <[email protected]>

The irq work queue is a per cpu object and it is sufficient for synchronization if per cpu atomics are used. Doing so simplifies the code and reduces the overhead of the code.

Before:

[email protected]$ size kernel/irq_work.o
   text	   data	    bss	    dec	    hex	filename
    451	      8	      1	    460	    1cc	kernel/irq_work.o

After:

[email protected]$ size kernel/irq_work.o 
   text	   data	    bss	    dec	    hex	filename
    438	      8	      1	    447	    1bf	kernel/irq_work.o

Cc Peter Zijlstra <[email protected]>

Signed-off-by Christoph Lameter <[email protected]>

Signed-off-by Christoph Lameter <[email protected]>

It is more effective to use a segment prefix instead of calculating the address of the current cpu area amd then testing flags.

Signed-off-by Christoph Lameter <[email protected]>

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 <[email protected]>

---
 drivers/acpi/processor_throttling.c |   32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

Index: linux-2.6/drivers/acpi/processor_throttling.c
===================================================================

drivers/acpi/processor_throttling.c +9 -21

@@ -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",

Quoted 3 lines · Christoph Lameter

It is more effective to use a segment prefix instead of calculating the address of the current cpu area amd then testing flags.

Quoted 1 line

Signed-off-by Christoph Lameter <[email protected]>

For this and the other x86 patch.

Acked-by Tejun Heo <[email protected]>

I suppose these two x86 patches and the gameport one would go through the x86 tree, right?

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

Quoted 94 lines · Christoph Lameter

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 <[email protected]>

---
 drivers/acpi/processor_throttling.c |   32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

Index: linux-2.6/drivers/acpi/processor_throttling.c
===================================================================

drivers/acpi/processor_throttling.c +10 -22

@@ -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);

drivers/acpi/processor_throttling.c static int acpi_read_throttling_status(s · line 747

Quoted 4 lines

drivers/acpi/processor_throttling.c +0 -0

 		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 <[email protected]>

I think this one too fits the x86 tree better.

Thanks.

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 <[email protected]>

---
 drivers/acpi/processor_throttling.c |   32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

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?

Quoted 8 lines · Tejun Heo

Quoted 4 lines · Christoph Lameter

It is more effective to use a segment prefix instead of calculating the address of the current cpu area amd then testing flags.

Signed-off-by Christoph Lameter <[email protected]>

For this and the other x86 patch.

Quoted 5 lines

Acked-by Tejun Heo <[email protected]>

I suppose these two x86 patches and the gameport one would go through the x86 tree, right?

Yes, as long as I can get a stable base to pull into -tip.

Quoted 11 lines · H. Peter Anvin

Quoted 10 lines · Tejun Heo

Quoted 4 lines · Christoph Lameter

It is more effective to use a segment prefix instead of calculating the address of the current cpu area amd then testing flags.

Signed-off-by Christoph Lameter <[email protected]>

For this and the other x86 patch.

Acked-by Tejun Heo <[email protected]>

Quoted 5 lines

Quoted 3 lines

I suppose these two x86 patches and the gameport one would go through the x86 tree, right?

Yes, as long as I can get a stable base to pull into -tip.

I suppose I should take the two x86 and the gameport patches here into percpu tree with your ACKs too. Would that be okay with you?

Acked-by H. Peter Anvin <[email protected]>