diff mbox

[2/8] arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c: avoid cross-CPU interrupts by using smp_call_function_any()

Message ID 200909181941.n8IJf5oR002397@imap1.linux-foundation.org (mailing list archive)
State RFC, archived
Headers show

Commit Message

Andrew Morton Sept. 18, 2009, 7:41 p.m. UTC 
From: Andrew Morton <akpm@linux-foundation.org>

Presently acpi-cpufreq will perform the MSR read on the first CPU in the
mask.  That's inefficient if that CPU differs from the current CPU. 
Because we have to perform a cross-CPU call, but we could have run the
rdmsr on the current CPU.

So switch to using the new smp_call_function_any(), whcih will perform the
call on the current CPU if that CPU is present in the mask (it is).

Cc: "Zhang, Yanmin" <yanmin_zhang@linux.intel.com>
Cc: Dave Jones <davej@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Jaswinder Singh Rajput <jaswinder@kernel.org>
Cc: Len Brown <len.brown@intel.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: Zhao Yakui <yakui.zhao@intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Dave Jones Sept. 18, 2009, 7:59 p.m. UTC | #1 
On Fri, Sep 18, 2009 at 12:41:05PM -0700, Andrew Morton wrote:

 > diff -puN arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts-by-using-smp_call_function_any arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
 > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts-by-using-smp_call_function_any
 > +++ a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
 > @@ -192,9 +192,11 @@ static void do_drv_write(void *_cmd)
 >  
 >  static void drv_read(struct drv_cmd *cmd)
 >  {
 > +	int err;
 >  	cmd->val = 0;
 >  
 > -	smp_call_function_single(cpumask_any(cmd->mask), do_drv_read, cmd, 1);
 > +	err = smp_call_function_any(cmd->mask, do_drv_read, cmd, 1);
 > +	WARN_ON_ONCE(err);	/* smp_call_function_any() was buggy? */
 >  }
 >  
 >  static void drv_write(struct drv_cmd *cmd)

I'm ok with this going in, but I still wonder if we could have done it
all a lot easier, by making cpumask_any pick the current cpu if it was
in the mask instead of introducing yet another variant to an already
enormous api.

	Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Morton Sept. 18, 2009, 8:11 p.m. UTC | #2 
On Fri, 18 Sep 2009 15:59:53 -0400
Dave Jones <davej@redhat.com> wrote:

> On Fri, Sep 18, 2009 at 12:41:05PM -0700, Andrew Morton wrote:
> 
>  > diff -puN arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts-by-using-smp_call_function_any arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
>  > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts-by-using-smp_call_function_any
>  > +++ a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
>  > @@ -192,9 +192,11 @@ static void do_drv_write(void *_cmd)
>  >  
>  >  static void drv_read(struct drv_cmd *cmd)
>  >  {
>  > +	int err;
>  >  	cmd->val = 0;
>  >  
>  > -	smp_call_function_single(cpumask_any(cmd->mask), do_drv_read, cmd, 1);
>  > +	err = smp_call_function_any(cmd->mask, do_drv_read, cmd, 1);
>  > +	WARN_ON_ONCE(err);	/* smp_call_function_any() was buggy? */
>  >  }
>  >  
>  >  static void drv_write(struct drv_cmd *cmd)
> 
> I'm ok with this going in, but I still wonder if we could have done it
> all a lot easier, by making cpumask_any pick the current cpu if it was
> in the mask instead of introducing yet another variant to an already
> enormous api.
> 

Yeah, I wondered about that.  But iirc Rusty had a good-sounding reason
for adding a new function but that reason didn't get into the changelog so
it's lost.

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rusty Russell Sept. 22, 2009, 7:17 a.m. UTC | #3 
On Sat, 19 Sep 2009 05:29:53 am Dave Jones wrote:
> I'm ok with this going in, but I still wonder if we could have done it
> all a lot easier, by making cpumask_any pick the current cpu if it was
> in the mask instead of introducing yet another variant to an already
> enormous api.

That was the initial proposal, but I'm uncomfortable with that, too.  "any"
really implies "doesn't matter"; if we overload it to have a preference, it
becomes really hard to tell which ones really care and which ones don't.

Then refactoring becomes harder (performance regression when you alter code).

So yeah, not ideal, but this seemed like the minimal API extension.

Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff -puN arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts-by-using-smp_call_function_any arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
--- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c~arch-x86-kernel-cpu-cpufreq-acpi-cpufreqc-avoid-cross-cpu-interrupts-by-using-smp_call_function_any
+++ a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
@@ -192,9 +192,11 @@  static void do_drv_write(void *_cmd)
 
 static void drv_read(struct drv_cmd *cmd)
 {
+	int err;
 	cmd->val = 0;
 
-	smp_call_function_single(cpumask_any(cmd->mask), do_drv_read, cmd, 1);
+	err = smp_call_function_any(cmd->mask, do_drv_read, cmd, 1);
+	WARN_ON_ONCE(err);	/* smp_call_function_any() was buggy? */
 }
 
 static void drv_write(struct drv_cmd *cmd)