From: Huaisheng HS1 Ye <yehs1@lenovo.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
CC: "srinivas.pandruvada@linux.intel.com" 
        <srinivas.pandruvada@linux.intel.com>,
        "lenb@kernel.org" <lenb@kernel.org>,
        "viresh.kumar@linaro.org" <viresh.kumar@linaro.org>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after
 performance governor changes
Thread-Topic: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after
 performance governor changes
Thread-Index: AQHTBD++fCU7C/LFX0qBYd13GtU46aJi2Z6AgAAwqJA=
Date: Mon, 24 Jul 2017 15:32:47 +0000
Message-ID: <KL1PR0302MB2502C43359F4B6A738D509B992BB0@KL1PR0302MB2502.apcprd03.prod.outlook.com>
References: <1500875013-123321-1-git-send-email-yehs1@lenovo.com>
 <7185077.O26hx51RqR@aspire.rjw.lan>
In-Reply-To: <7185077.O26hx51RqR@aspire.rjw.lan>
Accept-Language: zh-CN, en-US
Content-Language: zh-CN
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jul 2017 15:32:47.0921
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 5c7d0b28-bdf8-410c-aa93-4df372b16203
X-MS-Exchange-Transport-CrossTenantHeadersStamped: KL1PR0302MB2616
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v6OFXEDY014930
Content-Length: 4682
Lines: 112

Hi Rafael,
Thanks for your reply.

> On Monday, July 24, 2017 05:43:14 AM Huaisheng HS1 Ye wrote:
> > After commit 82b4e03e01bc (intel_pstate: skip scheduler hook when in
> > "performance" mode) Software P-state control modes couldn't get
> > dynamic value during performance mode,
> 
> Please explain what you mean here.
> 
commit 82b4e03e01bc (intel_pstate: skip scheduler hook when in 
"performance" mode) disables intel_pstate_set_update_util_hook when current
policy is performance within function intel_pstate_set_policy. It leads to 
Software P-states couldn't update sysfs interface cpuinfo_cur_freq's value 
during performance mode, because of pstate_funcs.update_util couldn't set 
for the given CPU.

> I guess you carried out some tests and the results were not as expected, so
> what was the test?
Exactly, we check the sysfs interface cpuinfo_cur_freq and the output of 
cpupower frequency-info both with performance mode.

For example, we list logic CPU 63's sysfs interface and output of cpupower with performance 
mode below,
analyzing CPU 63:
  driver: intel_pstate
  CPUs which run at the same hardware frequency: 63
  CPUs which need to have their frequency coordinated by software: 63
  maximum transition latency:  Cannot determine or is not supported.
  hardware limits: 1000 MHz - 3.70 GHz
  available cpufreq governors: performance powersave
  current policy: frequency should be within 1000 MHz and 3.70 GHz.
                  The governor "performance" may decide which speed to use
                  within this range.
  current CPU frequency: 1.07 GHz (asserted by call to hardware)  <--------cpuinfo_cur_freq 
  boost state support:
    Supported: yes
    Active: yes

[root@localhost cpufreq]# pwd
/sys/devices/system/cpu/cpu63/cpufreq
[root@localhost cpufreq]# cat cpuinfo_cur_freq scaling_cur_freq 
1070379
2796179

The value of cpuinfo_cur_freq is static always during performance mode, because of 
"cpu->sample.core_avg_perf" doesn't have chance to be updated when driver leaves 
from powersave mode. Its value equals to last sample result which comes from function 
intel_pstate_calc_avg_perf.
commit f8475cef (x86: use common aperfmperf_khz_on_cpu() to calculate KHz using 
APERF/MPERF) offers a great method to help scaling_cur_freq gets accurate frequency. 
And for some tools like cpupower which would try to get cpuinfo_cur_freq at first to show 
current frequency, if it fails then it would move to scaling_cur_freq which represents the 
real value.

> 
> > and it still in last value from powersave mode, so clear its value to
> > get same behavior as Hardware P-state to avoid confusion.
> 
> And please explain why it should be fixed the way you've done that.
> 

In order to make sure that cpuinfo_cur_freq couldn't get an effective value, 
we clean up "sample.core_avg_perf" when cpu's policy set to performance mode. 
so the reading of sysfs cpuinfo_cur_freq would get "<unknown>" always within 
performance mode, which is same with the status when HWP has been enabled.

With this patch, logic CPU 63's sysfs interface and output of cpupower with performance 
mode would be like this,

analyzing CPU 63:
  driver: intel_pstate
  CPUs which run at the same hardware frequency: 63
  CPUs which need to have their frequency coordinated by software: 63
  maximum transition latency:  Cannot determine or is not supported.
  hardware limits: 1000 MHz - 3.70 GHz
  available cpufreq governors: performance powersave
  current policy: frequency should be within 1000 MHz and 3.70 GHz.
                  The governor "performance" may decide which speed to use
                  within this range.
  current CPU frequency: Unable to call hardware				<--------cpuinfo_cur_freq
  current CPU frequency: 2.80 GHz (asserted by call to kernel)	<--------scaling_cur_freq
  boost state support:
    Supported: yes
    Active: yes

[root@localhost cpufreq]# pwd
/sys/devices/system/cpu/cpu63/cpufreq
[root@localhost cpufreq]# cat cpuinfo_cur_freq scaling_cur_freq 
<unknown>
2800000

> > Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
> > ---
> >  drivers/cpufreq/intel_pstate.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/cpufreq/intel_pstate.c
> > b/drivers/cpufreq/intel_pstate.c index 6cd5035..c675626 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -2050,6 +2050,7 @@ static int intel_pstate_set_policy(struct
> cpufreq_policy *policy)
> >  		 */
> >  		intel_pstate_clear_update_util_hook(policy->cpu);
> >  		intel_pstate_max_within_limits(cpu);
> > +		cpu->sample.core_avg_perf = 0;
> >  	} else {
> >  		intel_pstate_set_update_util_hook(policy->cpu);
> >  	}
> >