Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1694460 > unrolled thread
| Started by | Huaisheng HS1 Ye <yehs1@lenovo.com> |
|---|---|
| First post | 2017-07-24 07:50 +0200 |
| Last post | 2017-07-26 00:00 +0200 |
| Articles | 9 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes Huaisheng HS1 Ye <yehs1@lenovo.com> - 2017-07-24 07:50 +0200
Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-07-24 13:50 +0200
Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-07-25 02:10 +0200
Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> - 2017-07-25 05:00 +0200
Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> - 2017-07-25 16:40 +0200
Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-07-25 17:50 +0200
[PATCH] cpufreq: intel_pstate: Drop ->get from intel_pstate structure "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-07-26 01:00 +0200
Re: [PATCH] cpufreq: intel_pstate: Drop ->get from intel_pstate structure Viresh Kumar <viresh.kumar@linaro.org> - 2017-07-27 05:50 +0200
Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-07-26 00:00 +0200
| From | Huaisheng HS1 Ye <yehs1@lenovo.com> |
|---|---|
| Date | 2017-07-24 07:50 +0200 |
| Subject | [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes |
| Message-ID | <u6EpI-Fb-3@gated-at.bofh.it> |
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, and it still in last value from
powersave mode, so clear its value to get same behavior as Hardware
P-state to avoid confusion.
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);
}
--
1.8.3.1
[toc] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2017-07-24 13:50 +0200 |
| Subject | Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes |
| Message-ID | <u6K26-4ff-1@gated-at.bofh.it> |
| In reply to | #1694460 |
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.
I guess you carried out some tests and the results were not as expected,
so what was the test?
> 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.
> 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);
> }
>
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2017-07-25 02:10 +0200 |
| Subject | Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes |
| Message-ID | <u6VAe-3W4-9@gated-at.bofh.it> |
| In reply to | #1694653 |
On Monday, July 24, 2017 03:32:47 PM Huaisheng HS1 Ye wrote:
> 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.
OK, so what about the change below:
---
drivers/cpufreq/intel_pstate.c | 8 --------
1 file changed, 8 deletions(-)
Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1674,13 +1674,6 @@ static int intel_pstate_init_cpu(unsigne
return 0;
}
-static unsigned int intel_pstate_get(unsigned int cpu_num)
-{
- struct cpudata *cpu = all_cpu_data[cpu_num];
-
- return cpu ? get_avg_frequency(cpu) : 0;
-}
-
static void intel_pstate_set_update_util_hook(unsigned int cpu_num)
{
struct cpudata *cpu = all_cpu_data[cpu_num];
@@ -1921,7 +1914,6 @@ static struct cpufreq_driver intel_pstat
.setpolicy = intel_pstate_set_policy,
.suspend = intel_pstate_hwp_save_state,
.resume = intel_pstate_resume,
- .get = intel_pstate_get,
.init = intel_pstate_cpu_init,
.exit = intel_pstate_cpu_exit,
.stop_cpu = intel_pstate_stop_cpu,
[toc] | [prev] | [next] | [standalone]
| From | Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> |
|---|---|
| Date | 2017-07-25 05:00 +0200 |
| Subject | Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes |
| Message-ID | <u6YeJ-5ys-1@gated-at.bofh.it> |
| In reply to | #1695293 |
On Tue, 2017-07-25 at 01:46 +0000, Huaisheng HS1 Ye wrote:
> Hi Rafael,
>
> If you delete "get" function implement within intel_pstate, the
> sysfs interface cpuinfo_cur_freq will display <unknown> all the
> time.
cpuinfo_cur_freq by definition should show actual frequency HW
frequency. Unless I missed something. So Len Brown's patch should also
take care of this to get from arch specific function is available.
So in addition to Rafael's change, what about this?
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9bf97a3..29ec687 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -689,8 +689,13 @@ store_one(scaling_max_freq, max);
static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy *policy,
char *buf)
{
- unsigned int cur_freq = __cpufreq_get(policy);
+ unsigned int cur_freq;
+ cur_freq = arch_freq_get_on_cpu(policy->cpu);
+ if (cur_freq)
+ return sprintf(buf, "%u\n", cur_freq);
+
+ cur_freq = __cpufreq_get(policy);
if (cur_freq)
return sprintf(buf, "%u\n", cur_freq);
Thanks,
Srinivas
> To be honest, at the beginning I have consider this way like you
> patched, but based two reasons below, it is conservative for us to do
> that.
>
> 1. I am worried about whether it would lead to confusion for
> customers or
> Linux OS venders who are accustomed to cpuinfo_cur_freq.
> 2. This is the first time for me to offer patch to intel_pstate, not
> sure
> whether it could be accepted by you.
>
> >
> > On Monday, July 24, 2017 03:32:47 PM Huaisheng HS1 Ye wrote:
> > >
> > > 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.
> > OK, so what about the change below:
> >
> > ---
> > drivers/cpufreq/intel_pstate.c | 8 --------
> > 1 file changed, 8 deletions(-)
> >
> > Index: linux-pm/drivers/cpufreq/intel_pstate.c
> > ==============================================================
> > =====
> > --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> > +++ linux-pm/drivers/cpufreq/intel_pstate.c
> > @@ -1674,13 +1674,6 @@ static int intel_pstate_init_cpu(unsigne
> > return 0;
> > }
> >
> > -static unsigned int intel_pstate_get(unsigned int cpu_num) -{
> > - struct cpudata *cpu = all_cpu_data[cpu_num];
> > -
> > - return cpu ? get_avg_frequency(cpu) : 0;
> > -}
> > -
> > static void intel_pstate_set_update_util_hook(unsigned int
> > cpu_num) {
> > struct cpudata *cpu = all_cpu_data[cpu_num]; @@ -1921,7
> > +1914,6 @@
> > static struct cpufreq_driver intel_pstat
> > .setpolicy = intel_pstate_set_policy,
> > .suspend = intel_pstate_hwp_save_state,
> > .resume = intel_pstate_resume,
> > - .get = intel_pstate_get,
> > .init = intel_pstate_cpu_init,
> > .exit = intel_pstate_cpu_exit,
> > .stop_cpu = intel_pstate_stop_cpu,
[toc] | [prev] | [next] | [standalone]
| From | Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> |
|---|---|
| Date | 2017-07-25 16:40 +0200 |
| Subject | Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes |
| Message-ID | <u79aa-3YE-21@gated-at.bofh.it> |
| In reply to | #1695359 |
Hi Huaisheng,
On Tue, 2017-07-25 at 07:03 +0000, Huaisheng HS1 Ye wrote:
> Hi Srinivas,
> Your idea is great, but your patch at cpufreq.c will force all
> platforms to use scaling_cur_freq as first choice when userspace
> wants to access cpuinfo_cur_freq. It is ok for intel x86 platfrom but
> hard to say with other platforms.
arch_freq_get_on_cpu is only implemented on x86, for other platforms it
will not change behavior. I didn't understand your comment about first
choice.
Thanks,
Srinivas
> I modified it like that, it looks more reasonable. How about that?
>
> Hi Rafael,
> Deleting "get" function pointer within intel_pstate would lead to
> sysfs interface cpuinfo_cur_freq disappearing, because of
> cpufreq_add_dev_interface will check "cpufreq_driver->get" for it.
> Perhaps just return 0 with in intel_pstate_get would be a workaround
> for this issue, how about it?
>
> I have tested this patch based on Purley platform, both Hardware and
> Software P-states works correct, we could get accurate and same
> frequency from cpuinfo_cur_freq and scaling_cur_freq.
>
> drivers/cpufreq/cpufreq.c | 4 ++++
> drivers/cpufreq/intel_pstate.c | 8 +++++---
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 9bf97a3..922f9d9 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -694,6 +694,10 @@ static ssize_t show_cpuinfo_cur_freq(struct
> cpufreq_policy *policy,
> if (cur_freq)
> return sprintf(buf, "%u\n", cur_freq);
>
> + cur_freq = arch_freq_get_on_cpu(policy->cpu);
> + if (cur_freq)
> + return sprintf(buf, "%u\n", cur_freq);
> +
> return sprintf(buf, "<unknown>\n");
> }
>
> diff --git a/drivers/cpufreq/intel_pstate.c
> b/drivers/cpufreq/intel_pstate.c
> index 6cd5035..33e6c10 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1924,9 +1924,11 @@ static int intel_pstate_init_cpu(unsigned int
> cpunum)
>
> static unsigned int intel_pstate_get(unsigned int cpu_num)
> {
> - struct cpudata *cpu = all_cpu_data[cpu_num];
> -
> - return cpu ? get_avg_frequency(cpu) : 0;
> + /*
> + * Use frequency from scaling_cur_freq, reserve this
> function
> + * for existing of sysfs cpuinfo_cur_freq.
> + */
> + return 0;
> }
>
> static void intel_pstate_set_update_util_hook(unsigned int cpu_num)
>
>
> >
> > On Tue, 2017-07-25 at 01:46 +0000, Huaisheng HS1 Ye wrote:
> > >
> > > Hi Rafael,
> > >
> > > If you delete "get" function implement within intel_pstate, the
> > > sysfs
> > > interface cpuinfo_cur_freq will display <unknown> all the time.
> > cpuinfo_cur_freq by definition should show actual frequency HW
> > frequency.
> > Unless I missed something. So Len Brown's patch should also take
> > care of this
> > to get from arch specific function is available.
> > So in addition to Rafael's change, what about this?
> >
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index
> > 9bf97a3..29ec687 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -689,8 +689,13 @@ store_one(scaling_max_freq, max);
> > static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy
> > *policy,
> > char *buf)
> > {
> > - unsigned int cur_freq = __cpufreq_get(policy);
> > + unsigned int cur_freq;
> >
> > + cur_freq = arch_freq_get_on_cpu(policy->cpu);
> > + if (cur_freq)
> > + return sprintf(buf, "%u\n", cur_freq);
> > +
> > + cur_freq = __cpufreq_get(policy);
> > if (cur_freq)
> > return sprintf(buf, "%u\n", cur_freq);
> >
> >
> >
> > Thanks,
> > Srinivas
> >
> > >
> > > To be honest, at the beginning I have consider this way like you
> > > patched, but based two reasons below, it is conservative for us
> > > to do
> > > that.
> > >
> > > 1. I am worried about whether it would lead to confusion for
> > > customers
> > > or Linux OS venders who are accustomed to cpuinfo_cur_freq.
> > > 2. This is the first time for me to offer patch to intel_pstate,
> > > not
> > > sure whether it could be accepted by you.
> > >
> > > >
> > > >
> > > > On Monday, July 24, 2017 03:32:47 PM Huaisheng HS1 Ye wrote:
> > > > >
> > > > >
> > > > > 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.
> > > > OK, so what about the change below:
> > > >
> > > > ---
> > > > drivers/cpufreq/intel_pstate.c | 8 --------
> > > > 1 file changed, 8 deletions(-)
> > > >
> > > > Index: linux-pm/drivers/cpufreq/intel_pstate.c
> > > >
> > ==============================================================
> > >
> > > >
> > > > =====
> > > > --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> > > > +++ linux-pm/drivers/cpufreq/intel_pstate.c
> > > > @@ -1674,13 +1674,6 @@ static int intel_pstate_init_cpu(unsigne
> > > > return 0;
> > > > }
> > > >
> > > > -static unsigned int intel_pstate_get(unsigned int cpu_num) -{
> > > > - struct cpudata *cpu = all_cpu_data[cpu_num];
> > > > -
> > > > - return cpu ? get_avg_frequency(cpu) : 0;
> > > > -}
> > > > -
> > > > static void intel_pstate_set_update_util_hook(unsigned int
> > > > cpu_num) {
> > > > struct cpudata *cpu = all_cpu_data[cpu_num]; @@
> > > > -1921,7
> > > > +1914,6 @@
> > > > static struct cpufreq_driver intel_pstat
> > > > .setpolicy = intel_pstate_set_policy,
> > > > .suspend = intel_pstate_hwp_save_state,
> > > > .resume = intel_pstate_resume,
> > > > - .get = intel_pstate_get,
> > > > .init = intel_pstate_cpu_init,
> > > > .exit = intel_pstate_cpu_exit,
> > > > .stop_cpu = intel_pstate_stop_cpu,
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2017-07-25 17:50 +0200 |
| Subject | Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes |
| Message-ID | <u7afV-4DI-53@gated-at.bofh.it> |
| In reply to | #1695359 |
On Monday, July 24, 2017 07:57:45 PM Srinivas Pandruvada wrote:
> On Tue, 2017-07-25 at 01:46 +0000, Huaisheng HS1 Ye wrote:
> > Hi Rafael,
> >
> > If you delete "get" function implement within intel_pstate, the
> > sysfs interface cpuinfo_cur_freq will display <unknown> all the
> > time.
> cpuinfo_cur_freq by definition should show actual frequency HW
> frequency.
Right.
> Unless I missed something. So Len Brown's patch should also
> take care of this to get from arch specific function is available.
> So in addition to Rafael's change, what about this?
>
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 9bf97a3..29ec687 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -689,8 +689,13 @@ store_one(scaling_max_freq, max);
> static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy *policy,
> char *buf)
> {
> - unsigned int cur_freq = __cpufreq_get(policy);
> + unsigned int cur_freq;
>
> + cur_freq = arch_freq_get_on_cpu(policy->cpu);
> + if (cur_freq)
> + return sprintf(buf, "%u\n", cur_freq);
> +
> + cur_freq = __cpufreq_get(policy);
> if (cur_freq)
> return sprintf(buf, "%u\n", cur_freq);
>
So also by definition cpuinfo_cur_freq should not return the same thing as
scaling_cur_freq.
I actually would like cpuinfo_cur_freq to not be present at all, I need to
check why it still shows up if ->get is not present.
Thanks,
Rafael
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2017-07-26 01:00 +0200 |
| Subject | [PATCH] cpufreq: intel_pstate: Drop ->get from intel_pstate structure |
| Message-ID | <u7gY2-xa-27@gated-at.bofh.it> |
| In reply to | #1695886 |
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
The ->get callback in the intel_pstate structure was mostly there
for the scaling_cur_freq sysfs attribute to work, but after commit
f8475cef9008 (x86: use common aperfmperf_khz_on_cpu() to calculate
KHz using APERF/MPERF) that attribute uses arch_freq_get_on_cpu()
provided by the x86 arch code on all processors supported by
intel_pstate, so it doesn't need the ->get callback from the
driver any more.
Moreover, the very presence of the ->get callback in the intel_pstate
structure causes the cpuinfo_cur_freq attribute to be present when
intel_pstate operates in the active mode, which is bogus, because
the role of that attribute is to return the current CPU frequency
as seen by the hardware. For intel_pstate, though, this is just an
average frequency and not really current, but computed for the
previous sampling interval (the actual current frequency may be
way different at the point this value is obtained by reading from
cpuinfo_cur_freq), and after commit 82b4e03e01bc (intel_pstate: skip
scheduler hook when in "performance" mode) the value in
cpuinfo_cur_freq may be stale or just 0, depending on the driver's
operation mode. In fact, however, on the hardware supported by
intel_pstate there is no way to read the current CPU frequency
from it, so the cpuinfo_cur_freq attribute should not be present
at all when this driver is in use.
For this reason, drop intel_pstate_get() and clear the ->get
callback pointer pointing to it, so that the cpuinfo_cur_freq is
not present for intel_pstate in the active mode any more.
Fixes: 82b4e03e01bc (intel_pstate: skip scheduler hook when in "performance" mode)
Reported-by: Huaisheng Ye <yehs1@lenovo.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpufreq/intel_pstate.c | 8 --------
1 file changed, 8 deletions(-)
Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1674,13 +1674,6 @@ static int intel_pstate_init_cpu(unsigne
return 0;
}
-static unsigned int intel_pstate_get(unsigned int cpu_num)
-{
- struct cpudata *cpu = all_cpu_data[cpu_num];
-
- return cpu ? get_avg_frequency(cpu) : 0;
-}
-
static void intel_pstate_set_update_util_hook(unsigned int cpu_num)
{
struct cpudata *cpu = all_cpu_data[cpu_num];
@@ -1921,7 +1914,6 @@ static struct cpufreq_driver intel_pstat
.setpolicy = intel_pstate_set_policy,
.suspend = intel_pstate_hwp_save_state,
.resume = intel_pstate_resume,
- .get = intel_pstate_get,
.init = intel_pstate_cpu_init,
.exit = intel_pstate_cpu_exit,
.stop_cpu = intel_pstate_stop_cpu,
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2017-07-27 05:50 +0200 |
| Subject | Re: [PATCH] cpufreq: intel_pstate: Drop ->get from intel_pstate structure |
| Message-ID | <u7HYe-U7-3@gated-at.bofh.it> |
| In reply to | #1696654 |
On 26-07-17, 00:42, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The ->get callback in the intel_pstate structure was mostly there
> for the scaling_cur_freq sysfs attribute to work, but after commit
> f8475cef9008 (x86: use common aperfmperf_khz_on_cpu() to calculate
> KHz using APERF/MPERF) that attribute uses arch_freq_get_on_cpu()
> provided by the x86 arch code on all processors supported by
> intel_pstate, so it doesn't need the ->get callback from the
> driver any more.
>
> Moreover, the very presence of the ->get callback in the intel_pstate
> structure causes the cpuinfo_cur_freq attribute to be present when
> intel_pstate operates in the active mode, which is bogus, because
> the role of that attribute is to return the current CPU frequency
> as seen by the hardware. For intel_pstate, though, this is just an
> average frequency and not really current, but computed for the
> previous sampling interval (the actual current frequency may be
> way different at the point this value is obtained by reading from
> cpuinfo_cur_freq), and after commit 82b4e03e01bc (intel_pstate: skip
> scheduler hook when in "performance" mode) the value in
> cpuinfo_cur_freq may be stale or just 0, depending on the driver's
> operation mode. In fact, however, on the hardware supported by
> intel_pstate there is no way to read the current CPU frequency
> from it, so the cpuinfo_cur_freq attribute should not be present
> at all when this driver is in use.
>
> For this reason, drop intel_pstate_get() and clear the ->get
> callback pointer pointing to it, so that the cpuinfo_cur_freq is
> not present for intel_pstate in the active mode any more.
>
> Fixes: 82b4e03e01bc (intel_pstate: skip scheduler hook when in "performance" mode)
> Reported-by: Huaisheng Ye <yehs1@lenovo.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/cpufreq/intel_pstate.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -1674,13 +1674,6 @@ static int intel_pstate_init_cpu(unsigne
> return 0;
> }
>
> -static unsigned int intel_pstate_get(unsigned int cpu_num)
> -{
> - struct cpudata *cpu = all_cpu_data[cpu_num];
> -
> - return cpu ? get_avg_frequency(cpu) : 0;
> -}
> -
> static void intel_pstate_set_update_util_hook(unsigned int cpu_num)
> {
> struct cpudata *cpu = all_cpu_data[cpu_num];
> @@ -1921,7 +1914,6 @@ static struct cpufreq_driver intel_pstat
> .setpolicy = intel_pstate_set_policy,
> .suspend = intel_pstate_hwp_save_state,
> .resume = intel_pstate_resume,
> - .get = intel_pstate_get,
> .init = intel_pstate_cpu_init,
> .exit = intel_pstate_cpu_exit,
> .stop_cpu = intel_pstate_stop_cpu,
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2017-07-26 00:00 +0200 |
| Subject | Re: [PATCH] cpufreq: intel_pstate: Fix cpuinfo_cur_freq after performance governor changes |
| Message-ID | <u7g1Y-8nQ-15@gated-at.bofh.it> |
| In reply to | #1695359 |
On Tuesday, July 25, 2017 07:03:36 AM Huaisheng HS1 Ye wrote: > Hi Srinivas, > Your idea is great, but your patch at cpufreq.c will force all platforms to use scaling_cur_freq as first choice when userspace wants to access cpuinfo_cur_freq. It is ok for intel x86 platfrom but hard to say with other platforms. > I modified it like that, it looks more reasonable. How about that? > > Hi Rafael, > Deleting "get" function pointer within intel_pstate would lead to sysfs > interface cpuinfo_cur_freq disappearing, because of > cpufreq_add_dev_interface will check "cpufreq_driver->get" for it. Which is exactly what I want. cpuinfo_cur_freq is bogus for intel_pstate and it should have never been exported for this driver. > Perhaps just return 0 with in intel_pstate_get would be a workaround for this > issue, how about it? > > I have tested this patch based on Purley platform, both Hardware and Software > P-states works correct, we could get accurate and same frequency from > cpuinfo_cur_freq and scaling_cur_freq. But this is not correct. These two attributes should not be expected to always return the same value. Thanks, Rafael
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web