arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c: avoid cross-CPU interrupts

PATCH In review [email protected] 5 participants 10 messages Apr 11, 2009 – Apr 20, 2009

Subsystems (4)

x86 ACPI Library code Bitmap API

lore

Share & tools

lore

[email protected]

author maintainer

2009-04-11 06:26 UTC #0 lore raw

PATCH 2/2 arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c: avoid cross-CPU interrupts

From: Andrew Morton <[email protected]>

In drv_read(), check to see whether we can run the rdmsr() on the current CPU. If so, do that. So smp_call_function_single() can avoid the IPI.

Arguably, cpumask_any() should do this.

Cc Rusty Russell <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Venkatesh Pallipadi <[email protected]>

Cc Len Brown <[email protected]>

Cc Zhao Yakui <[email protected]>

Cc Dave Jones <[email protected]>

Cc Thomas Gleixner <[email protected]>

Tested-by Mike Galbraith <[email protected]>

Cc "Zhang, Yanmin" <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c renamed from arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts +15 -2

diff -puN arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
@@ -197,9 +197,22 @@ static void do_drv_write(void *_cmd)
 
 static void drv_read(struct drv_cmd *cmd)
 {
-	cmd->val = 0;
+	int target_cpu;		/* The CPU on which to perform thr rdmsr() */
+	int this_cpu;
+
+	/*
+	 * If the current CPU is in cmd->mask then run the rdmsr() on this
+	 * CPU to avoid the cross-cpu interrupt.
+	 */
+	this_cpu = get_cpu();
+	if (cpu_isset(this_cpu, *(cmd->mask)))
+		target_cpu = this_cpu;
+	else
+		target_cpu = cpumask_any(cmd->mask);
 
-	smp_call_function_single(cpumask_any(cmd->mask), do_drv_read, cmd, 1);
+	cmd->val = 0;
+	smp_call_function_single(target_cpu, do_drv_read, cmd, 1);
+	put_cpu();
 }
 
 static void drv_write(struct drv_cmd *cmd)

Jaswinder Singh Rajput

2009-04-11 06:37 UTC #1 lore raw

Quoted 26 lines · On Fri, 2009-04-10 at 23:17 -0700, [email protected] wrote:

From: Andrew Morton <[email protected]>
In drv_read(), check to see whether we can run the rdmsr() on the current CPU. If so, do that. So smp_call_function_single() can avoid the IPI.
Arguably, cpumask_any() should do this.
Cc Rusty Russell <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Venkatesh Pallipadi <[email protected]>
Cc Len Brown <[email protected]>
Cc Zhao Yakui <[email protected]>
Cc Dave Jones <[email protected]>
Cc Thomas Gleixner <[email protected]>
Tested-by Mike Galbraith <[email protected]>
Cc "Zhang, Yanmin" <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
---
 arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
diff -puN arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c

arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c static void do_drv_write(void *_cmd) · line 200

Quoted 5 lines

arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c renamed from arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts +1 -1
@@ -197,9 +197,22 @@ static void do_drv_write(void *_cmd)
 
 static void drv_read(struct drv_cmd *cmd)
 {
-	cmd->val = 0;
+	int target_cpu;		/* The CPU on which to perform thr rdmsr() */

+ int target_cpu; /* The CPU on which to perform the rdmsr() */

--
JSR

Dave Jones

2009-04-12 00:08 UTC #2 lore raw

In reply to [email protected]

Quoted 4 lines · Andrew Morton

From: Andrew Morton <[email protected]>
In drv_read(), check to see whether we can run the rdmsr() on the current CPU. If so, do that. So smp_call_function_single() can avoid the IPI.

Wouldn't it be a better to make smp_call_function_single do this check itself, so all callers benefit from this optimisation?

*looks*

Wait, won't this already be caught by this code in smp_call_function_single() ?

286 this_cpu = get_cpu();
...
291 if (cpu == this_cpu) {

292                 local_irq_save(flags);
293                 func(info);
294                 local_irq_restore(flags);

295 } else {

Dave

Andrew Morton

author maintainer

2009-04-12 00:55 UTC #3 lore raw

Quoted 13 lines · On Sat, 11 Apr 2009 20:06:05 -0400 Dave Jones <[email protected]> wrote:

Quoted 4 lines · Andrew Morton
From: Andrew Morton <[email protected]>
In drv_read(), check to see whether we can run the rdmsr() on the current CPU. If so, do that. So smp_call_function_single() can avoid the IPI.
Wouldn't it be a better to make smp_call_function_single do this check itself, so all callers benefit from this optimisation?
*looks*
Wait, won't this already be caught by this code in smp_call_function_single() ?

Quoted 10 lines

286 this_cpu = get_cpu();
...
291 if (cpu == this_cpu) {
292                 local_irq_save(flags);
293                 func(info);
294                 local_irq_restore(flags);
295 } else {

The problem is that the caller (acpi-cpufreq) is doing

	cpu = cpumask_any(mask);
	smp_call_function_single(cpu);

and cpumask_any(mask) does cpumask_first(mask). Which might be a different CPU, even though this thread of control is running on a CPU which is present in `mask'.

- We could fix this by making cpumask_any(mask) return this-cpu if this-cpu is present `mask'.

- We could fix this by changing smp_call_function_single() to take a mask, rather than a particular CPU. Then of course it preferentially chooses this-cpu if possible.

  Or write a new smp_call_function_any(mask, ...);

I suspect that changing cpumask_any() to preferentially return this-cpu will always give us the behaviour that we prefer, but I haven't looked into it.

Rusty Russell

2009-04-14 08:51 UTC #4 lore raw

Quoted 3 lines · Andrew Morton

I suspect that changing cpumask_any() to preferentially return this-cpu will always give us the behaviour that we prefer, but I haven't looked into it.

How's this?

Subject: cpumask: cpumask_closest()

Impact: new function

Andrew points out that acpi-cpufreq uses cpumask_any, when it really would prefer to use the same CPU if possible (to avoid an IPI). In general, this seems a good idea to offer.

Signed-off-by Rusty Russell <[email protected]>

CC Andrew Morton <[email protected]>

include/linux/cpumask.h +2 -0

@@ -931,6 +931,8 @@ static inline void cpumask_copy(struct c
  */
 #define cpumask_of(cpu) (get_cpu_mask(cpu))
 
+unsigned int cpumask_closest(const struct cpumask *mask);
+
 /**
  * cpumask_scnprintf - print a cpumask into a string as comma-separated hex
  * @buf: the buffer to sprintf into

lib/cpumask.c +23 -0

@@ -170,3 +170,26 @@ void __init free_bootmem_cpumask_var(cpu
 	free_bootmem((unsigned long)mask, cpumask_size());
 }
 #endif
+
+/**
+ * cpumask_closest - return the closest cpu in mask.
+ * @mask: the cpus to choose from.
+ *
+ * Returns >= nr_cpu_ids if no bits are set in @mask.
+ */
+unsigned int cpumask_closest(const struct cpumask *mask)
+{
+	unsigned int cpu = raw_smp_processor_id();
+
+	/* Try for same CPU. */
+	if (cpumask_test_cpu(cpu, mask))
+		return cpu;
+
+	/* Try for same node. */
+	cpu = cpumask_any_and(cpumask_of_node(cpu), mask);
+	if (cpu <= nr_cpu_ids)
+		return cpu;
+
+	/* Anything will do. */
+	return cpumask_any(mask);
+}

Andrew Morton

author maintainer

2009-04-14 17:27 UTC #5 lore raw

Quoted 50 lines · On Tue, 14 Apr 2009 18:21:36 +0930 Rusty Russell <[email protected]> wrote:

Quoted 3 lines · Andrew Morton
I suspect that changing cpumask_any() to preferentially return this-cpu will always give us the behaviour that we prefer, but I haven't looked into it.
How's this?
Subject: cpumask: cpumask_closest()
Impact: new function
Andrew points out that acpi-cpufreq uses cpumask_any, when it really would prefer to use the same CPU if possible (to avoid an IPI). In general, this seems a good idea to offer.
Signed-off-by Rusty Russell <[email protected]>
CC Andrew Morton <[email protected]>
include/linux/cpumask.h +2 -0
@@ -931,6 +931,8 @@ static inline void cpumask_copy(struct c
  */
 #define cpumask_of(cpu) (get_cpu_mask(cpu))
 
+unsigned int cpumask_closest(const struct cpumask *mask);
+
 /**
  * cpumask_scnprintf - print a cpumask into a string as comma-separated hex
  * @buf: the buffer to sprintf into
lib/cpumask.c +13 -0
@@ -170,3 +170,26 @@ void __init free_bootmem_cpumask_var(cpu
 	free_bootmem((unsigned long)mask, cpumask_size());
 }
 #endif
+
+/**
+ * cpumask_closest - return the closest cpu in mask.
+ * @mask: the cpus to choose from.
+ *
+ * Returns >= nr_cpu_ids if no bits are set in @mask.
+ */
+unsigned int cpumask_closest(const struct cpumask *mask)
+{
+	unsigned int cpu = raw_smp_processor_id();
+
+	/* Try for same CPU. */
+	if (cpumask_test_cpu(cpu, mask))

lib/cpumask.c void __init free_bootmem_cpumask_var(cpu · line 195

Quoted 10 lines

lib/cpumask.c +10 -0

+		return cpu;
+
+	/* Try for same node. */
+	cpu = cpumask_any_and(cpumask_of_node(cpu), mask);
+	if (cpu <= nr_cpu_ids)
+		return cpu;
+
+	/* Anything will do. */
+	return cpumask_any(mask);
+}

Should it be exported?

It looks all racy against hotplug. What are the caller's responsibilities here?

any_online_cpu() could use cpumask_closest(), against (*mask & cpu_online_map).

I think all cpumask_any() call sites can be migrated to cpumask_closest() with, at worst, no benefit.

Rusty Russell

2009-04-15 06:35 UTC #6 lore raw

Quoted 2 lines · Andrew Morton

Quoted 1 line · On Tue, 14 Apr 2009 18:21:36 +0930 Rusty Russell <[email protected]> wrote:
Subject: cpumask: cpumask_closest()

Quoted 1 line

Should it be exported?

Ah yes.

Quoted 2 lines

It looks all racy against hotplug. What are the caller's responsibilities here?

Kind of independent. There's no implied internal reference to online_mask.

Quoted 1 line

any_online_cpu() could use cpumask_closest(), against (*mask & cpu_online_map).

Note that I've been killing any_online_cpu(). It passes a cpumask on stack, and cpumask_any(cpu_online_mask) / cpumask_any_and(mask, cpu_online_mask) work just as well.

Quoted 2 lines

I think all cpumask_any() call sites can be migrated to cpumask_closest() with, at worst, no benefit.

OK, here's the updated patch.

Rusty.

Subject: cpumask: cpumask_closest() and cpumask_closest_and()

Impact: new functions

Andrew points out that acpi-cpufreq uses cpumask_any, when it really would prefer to use the same CPU if possible (to avoid an IPI). In general, this seems a good idea to offer.

Signed-off-by Rusty Russell <[email protected]>

CC Andrew Morton <[email protected]>

---
 include/linux/cpumask.h |    4 +++
 lib/cpumask.c           |   54 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

include/linux/cpumask.h +4 -0

@@ -931,6 +931,10 @@ static inline void cpumask_copy(struct c
  */
 #define cpumask_of(cpu) (get_cpu_mask(cpu))
 
+unsigned int cpumask_closest(const struct cpumask *mask);
+unsigned int cpumask_closest_and(const struct cpumask *mask1,
+				 const struct cpumask *mask2);
+
 /**
  * cpumask_scnprintf - print a cpumask into a string as comma-separated hex
  * @buf: the buffer to sprintf into

lib/cpumask.c +54 -0

57 lines not on this page · raw message

Len Brown

reviewer

2009-04-20 03:22 UTC #7 lore raw

Quoted 4 lines

Andrew points out that acpi-cpufreq uses cpumask_any, when it really would prefer to use the same CPU if possible (to avoid an IPI). In general, this seems a good idea to offer.

I like this idea.

I'll drop andrew's patch "acpi-cpufreq.c: avoid cross-CPU interrupts" b/c that is not an emergency and this is more clever.

thanks,
-Len Brown, Intel Open Source Technology Center.

Len Brown

reviewer

2009-04-20 02:57 UTC #8 lore raw

In reply to [email protected]

Quoted 53 lines

From: Andrew Morton <[email protected]>
In drv_read(), check to see whether we can run the rdmsr() on the current CPU. If so, do that. So smp_call_function_single() can avoid the IPI.
Arguably, cpumask_any() should do this.
Cc Rusty Russell <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Venkatesh Pallipadi <[email protected]>
Cc Len Brown <[email protected]>
Cc Zhao Yakui <[email protected]>
Cc Dave Jones <[email protected]>
Cc Thomas Gleixner <[email protected]>
Tested-by Mike Galbraith <[email protected]>
Cc "Zhang, Yanmin" <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
---
 arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
diff -puN arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c renamed from arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts +15 -2
@@ -197,9 +197,22 @@ static void do_drv_write(void *_cmd)
 
 static void drv_read(struct drv_cmd *cmd)
 {
-	cmd->val = 0;
+	int target_cpu;		/* The CPU on which to perform thr rdmsr() */
+	int this_cpu;
+
+	/*
+	 * If the current CPU is in cmd->mask then run the rdmsr() on this
+	 * CPU to avoid the cross-cpu interrupt.
+	 */
+	this_cpu = get_cpu();
+	if (cpu_isset(this_cpu, *(cmd->mask)))
+		target_cpu = this_cpu;
+	else
+		target_cpu = cpumask_any(cmd->mask);
 
-	smp_call_function_single(cpumask_any(cmd->mask), do_drv_read, cmd, 1);
+	cmd->val = 0;
+	smp_call_function_single(target_cpu, do_drv_read, cmd, 1);
+	put_cpu();
 }
 
 static void drv_write(struct drv_cmd *cmd)

Quoted 1 line

_

Rather than this patch I would expect we would want to either:

A. as we went to the trouble to detect the local case
in drv_read, why call smp_call_function at all for that case?

B. optimize smp_call_function_single to beneift all users
instead of just this customer.

thanks,
-Len Brown, Intel Open Source Technology Center

Andrew Morton

author maintainer

2009-04-20 03:29 UTC #9 lore raw

Quoted 27 lines · On Sun, 19 Apr 2009 22:57:24 -0400 (EDT) Len Brown <[email protected]> wrote:

Quoted 27 lines

+++ a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c

arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c renamed from arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts +15 -2

@@ -197,9 +197,22 @@ static void do_drv_write(void *_cmd)
 
 static void drv_read(struct drv_cmd *cmd)
 {
-	cmd->val = 0;
+	int target_cpu;		/* The CPU on which to perform thr rdmsr() */
+	int this_cpu;
+
+	/*
+	 * If the current CPU is in cmd->mask then run the rdmsr() on this
+	 * CPU to avoid the cross-cpu interrupt.
+	 */
+	this_cpu = get_cpu();
+	if (cpu_isset(this_cpu, *(cmd->mask)))
+		target_cpu = this_cpu;
+	else
+		target_cpu = cpumask_any(cmd->mask);
 
-	smp_call_function_single(cpumask_any(cmd->mask), do_drv_read, cmd, 1);
+	cmd->val = 0;
+	smp_call_function_single(target_cpu, do_drv_read, cmd, 1);
+	put_cpu();
 }

Quoted 7 lines

Quoted 2 lines
arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c renamed from arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts +0 -0
 static void drv_write(struct drv_cmd *cmd)
_
Rather than this patch I would expect we would want to either:
A. as we went to the trouble to detect the local case
in drv_read, why call smp_call_function at all for that case?

Sure, that would work.

I felt it was a little cleaner to always delegate the call to smp_call_function() rather than open-coding smp_call_function()'s internal implementation details at this site. We'd need to do:

	local_irq_disable();		/* Because this is what smp_call_function_single() does */
	do_drv_read(...);
	local_irq_enable();

Quoted 4 lines

or
B. optimize smp_call_function_single to beneift all users
instead of just this customer.

Yep. That would be a new smp_call_function_any() which takes a cpumask rather than a single CPU number. I think Rusty was cooking something up..