[PATCH 6/8]cpufreq:acpi-cpufreq: Eliminate get of current freq on notification

[PATCH 6/8]cpufreq:acpi-cpufreq: Eliminate get of current freq on notification

[Venkatesh Pallipadi]

Only change the frequency if the state previously set is different
from what we are trying to set. We don't really have to get the current
frequency at this point.

Signed-off-by: Denis Sadykov <denis.m.sadykov@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Signed-off-by: Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>

Index: linux-2.6.18/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
===================================================================
--- linux-2.6.18.orig/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
+++ linux-2.6.18/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
@@ -324,11 +324,8 @@ static int acpi_cpufreq_target(struct cp
 	online_policy_cpus = policy->cpus;
 #endif
 
-	cmd.val = get_cur_val(online_policy_cpus);
-	freqs.old = extract_freq(cmd.val, data);
-	freqs.new = data->freq_table[next_state].frequency;
 	next_perf_state = data->freq_table[next_state].index;
-	if (freqs.new == freqs.old) {
+	if (perf->state == next_perf_state) {
 		if (unlikely(data->resume)) {
 			dprintk("Called after resume, resetting to P%d\n",
 				next_perf_state);
@@ -366,6 +363,8 @@ static int acpi_cpufreq_target(struct cp
 	else
 		cpu_set(policy->cpu, cmd.mask);
 
+	freqs.old = data->freq_table[perf->state].frequency;
+	freqs.new = data->freq_table[next_perf_state].frequency;
 	for_each_cpu_mask(i, cmd.mask) {
 		freqs.cpu = i;
 		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
@@ -613,6 +612,7 @@ static int acpi_cpufreq_cpu_init(struct 
 		policy->cur = acpi_cpufreq_guess_freq(data, policy->cpu);
 		break;
 	case ACPI_ADR_SPACE_FIXED_HARDWARE:
+		acpi_cpufreq_driver.get = get_cur_freq_on_cpu;
 		get_cur_freq_on_cpu(cpu);
 		break;
 	default:
@@ -687,7 +687,6 @@ static struct freq_attr *acpi_cpufreq_at
 static struct cpufreq_driver acpi_cpufreq_driver = {
 	.verify = acpi_cpufreq_verify,
 	.target = acpi_cpufreq_target,
-	.get = get_cur_freq_on_cpu,
 	.init = acpi_cpufreq_cpu_init,
 	.exit = acpi_cpufreq_cpu_exit,
 	.resume = acpi_cpufreq_resume,

RE: [PATCH 6/8]cpufreq: acpi-cpufreq: Eliminate get of current freq on notification

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Dominik Brodowski [mailto:linux@dominikbrodowski.net] 
>Sent: Sunday, October 01, 2006 5:52 PM
>To: Pallipadi, Venkatesh
>Cc: Dave Jones; cpufreq; Starikovskiy, Alexey Y
>Subject: Re: [PATCH 6/8]cpufreq: acpi-cpufreq: Eliminate get 
>of current freq on notification
>
>Hi,
>
>On Sun, Oct 01, 2006 at 07:15:45AM -0700, Venkatesh Pallipadi wrote:
>> Only change the frequency if the state previously set is different
>> from what we are trying to set. We don't really have to get 
>the current
>> frequency at this point.
>
>What if it changed behind our back? Shouldn't happen, I know, 
>but it does
>happen.
>
>	Dominik

Yes. It happens and it can happen more often with multi-core CPUs which
has hardware coordination set up. Changing frequency on one CPU can
affect other CPU in the same package. But, changing frequency behind the
back does not matter on recent CPUs, as TSC is constant across P-state.
Also, depending on frequency that changes behind the back is not good.
If something changed behind the back, it will also later revert back to
original state in the same way. So, if we get current frequency in
between and take some decision based on that, will be bad.
Example:
- User has set the frequency to P0.
- TM2 kicks in and reduces the frequency to P3 state.
- Now user wants to reduce the frequency to P3 (This is the point where
above current freq was being checked).
- At this time, we can do two things
Option 1)
- Get the current frequency and see that we are in P3.
- Do not change the frequency as per user request.
- Now once TM2 event goes away (temperature is under control), CPU goes
back to P0, which was original user set state.

Option 2)
- Look at what user had set previously (P0) and set the state to P3
(even though we are in that state due to P3).
- Now once TM2 event goes away (temperature is under control), CPU goes
back to last user set state in this case P3.

Option 2 is what is ideal and the patch does it that way. Same example
as above can happen with hardware coordination as well.

Thanks,
Venki

Re: [PATCH 6/8]cpufreq: acpi-cpufreq: Eliminate get of current freq on notification

[Dominik Brodowski]

Hi,

On Mon, Oct 02, 2006 at 04:16:37PM -0700, Pallipadi, Venkatesh wrote:
> 
> >-----Original Message-----
> >From: Dominik Brodowski [mailto:linux@dominikbrodowski.net] 
> >Sent: Sunday, October 01, 2006 5:52 PM
> >To: Pallipadi, Venkatesh
> >Cc: Dave Jones; cpufreq; Starikovskiy, Alexey Y
> >Subject: Re: [PATCH 6/8]cpufreq: acpi-cpufreq: Eliminate get 
> >of current freq on notification
> >
> >Hi,
> >
> >On Sun, Oct 01, 2006 at 07:15:45AM -0700, Venkatesh Pallipadi wrote:
> >> Only change the frequency if the state previously set is different
> >> from what we are trying to set. We don't really have to get 
> >the current
> >> frequency at this point.
> >
> >What if it changed behind our back? Shouldn't happen, I know, 
> >but it does
> >happen.
> >
> >	Dominik
> 
> Yes. It happens and it can happen more often with multi-core CPUs which
> has hardware coordination set up. Changing frequency on one CPU can
> affect other CPU in the same package. But, changing frequency behind the
> back does not matter on recent CPUs, as TSC is constant across P-state.
> Also, depending on frequency that changes behind the back is not good.
> If something changed behind the back, it will also later revert back to
> original state in the same way. So, if we get current frequency in
> between and take some decision based on that, will be bad.
> Example:
> - User has set the frequency to P0.
> - TM2 kicks in and reduces the frequency to P3 state.
> - Now user wants to reduce the frequency to P3 (This is the point where
> above current freq was being checked).
> - At this time, we can do two things
> Option 1)
> - Get the current frequency and see that we are in P3.
> - Do not change the frequency as per user request.
> - Now once TM2 event goes away (temperature is under control), CPU goes
> back to P0, which was original user set state.
> 
> Option 2)
> - Look at what user had set previously (P0) and set the state to P3
> (even though we are in that state due to P3).
> - Now once TM2 event goes away (temperature is under control), CPU goes
> back to last user set state in this case P3.
> 
> Option 2 is what is ideal and the patch does it that way. Same example
> as above can happen with hardware coordination as well.

Thanks for the clarification. ACK. ;)

	Dominik

[PATCH 6/8]cpufreq: acpi-cpufreq: Eliminate get of current freq on notification

[Venkatesh Pallipadi]

Only change the frequency if the state previously set is different
from what we are trying to set. We don't really have to get the current
frequency at this point.

Signed-off-by: Denis Sadykov <denis.m.sadykov@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Signed-off-by: Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>

Index: linux-2.6.18/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
===================================================================
--- linux-2.6.18.orig/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
+++ linux-2.6.18/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
@@ -325,11 +325,8 @@ static int acpi_cpufreq_target(struct cp
 	online_policy_cpus = policy->cpus;
 #endif
 
-	cmd.val = get_cur_val(online_policy_cpus);
-	freqs.old = extract_freq(cmd.val, data);
-	freqs.new = data->freq_table[next_state].frequency;
 	next_perf_state = data->freq_table[next_state].index;
-	if (freqs.new == freqs.old) {
+	if (perf->state == next_perf_state) {
 		if (unlikely(data->resume)) {
 			dprintk("Called after resume, resetting to P%d\n",
 				next_perf_state);
@@ -367,6 +364,8 @@ static int acpi_cpufreq_target(struct cp
 	else
 		cpu_set(policy->cpu, cmd.mask);
 
+	freqs.old = data->freq_table[perf->state].frequency;
+	freqs.new = data->freq_table[next_perf_state].frequency;
 	for_each_cpu_mask(i, cmd.mask) {
 		freqs.cpu = i;
 		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);

Re: [PATCH 6/8]cpufreq: acpi-cpufreq: Eliminate get of current freq on notification

[Dominik Brodowski]

Hi,

On Sun, Oct 01, 2006 at 07:15:45AM -0700, Venkatesh Pallipadi wrote:
> Only change the frequency if the state previously set is different
> from what we are trying to set. We don't really have to get the current
> frequency at this point.

What if it changed behind our back? Shouldn't happen, I know, but it does
happen.

	Dominik