Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

On Wed, 2024-06-05 at 13:21 +0800, Xi Ruoyao wrote:
> On Tue, 2024-06-04 at 09:56 -0700, srinivas pandruvada wrote:
> > > > With such a delay, I am not sure how this even worked before.
> 
> It didn't work out of box but it worked after manually writing 0 to
> no_turbo after 20 seconds, see
> https://bugzilla.kernel.org/show_bug.cgi?id=218702.

That make sense. So it never worked out of box. The store_no_turbo()
has additional read for turbo flag before, which is removed now. I
think adding that back will will restore old behavior.

diff --git a/drivers/cpufreq/intel_pstate.c
b/drivers/cpufreq/intel_pstate.c
index 4b986c044741..0d5330e5b96b 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1301,6 +1301,8 @@ static ssize_t store_no_turbo(struct kobject *a,
struct kobj_attribute *b,
 
        no_turbo = !!clamp_t(int, input, 0, 1);
 
+       global.turbo_disabled = turbo_is_disabled();
+
        if (no_turbo == global.no_turbo)
                goto unlock_driver;


Need to adjust the mutex around it also.

Regarding the bugzilla, I disabled turbo and no_turbo shows 1. Not sure
how it shows "0" to you.

Thanks,
Srinivas


> 
> > > > Can you revert the patch in question and use kernel dynamic
> > > > debug
> > > > dyndbg="file intel_pstate.c +p" kernel command line and collect
> > > > log
> > > > for
> > > > 30 seconds?
> 
> Attached.  The related part seems:
> 
> [    0.553606] intel_pstate: Intel P-state driver initializing
> [    0.553630] intel_pstate: controlling: cpu 0
> [    0.553640] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553642] intel_pstate: cpu:0 min_policy_perf:4
> max_policy_perf:31
> [    0.553643] intel_pstate: cpu:0 global_min:0 global_max:44
> [    0.553644] intel_pstate: cpu:0 max_perf_ratio:31 min_perf_ratio:4
> [    0.553680] intel_pstate: controlling: cpu 1
> [    0.553702] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553703] intel_pstate: cpu:1 min_policy_perf:4
> max_policy_perf:31
> [    0.553704] intel_pstate: cpu:1 global_min:0 global_max:44
> [    0.553705] intel_pstate: cpu:1 max_perf_ratio:31 min_perf_ratio:4
> [    0.553742] intel_pstate: controlling: cpu 2
> [    0.553763] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553764] intel_pstate: cpu:2 min_policy_perf:4
> max_policy_perf:31
> [    0.553765] intel_pstate: cpu:2 global_min:0 global_max:44
> [    0.553766] intel_pstate: cpu:2 max_perf_ratio:31 min_perf_ratio:4
> [    0.553809] intel_pstate: controlling: cpu 3
> [    0.553830] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553831] intel_pstate: cpu:3 min_policy_perf:4
> max_policy_perf:31
> [    0.553831] intel_pstate: cpu:3 global_min:0 global_max:44
> [    0.553832] intel_pstate: cpu:3 max_perf_ratio:31 min_perf_ratio:4
> [    0.553868] intel_pstate: controlling: cpu 4
> [    0.553892] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553893] intel_pstate: cpu:4 min_policy_perf:4
> max_policy_perf:31
> [    0.553894] intel_pstate: cpu:4 global_min:0 global_max:44
> [    0.553895] intel_pstate: cpu:4 max_perf_ratio:31 min_perf_ratio:4
> [    0.553943] intel_pstate: controlling: cpu 5
> [    0.553967] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553968] intel_pstate: cpu:5 min_policy_perf:4
> max_policy_perf:31
> [    0.553968] intel_pstate: cpu:5 global_min:0 global_max:44
> [    0.553969] intel_pstate: cpu:5 max_perf_ratio:31 min_perf_ratio:4
> [    0.554009] intel_pstate: controlling: cpu 6
> [    0.554034] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.554035] intel_pstate: cpu:6 min_policy_perf:4
> max_policy_perf:31
> [    0.554036] intel_pstate: cpu:6 global_min:0 global_max:44
> [    0.554037] intel_pstate: cpu:6 max_perf_ratio:31 min_perf_ratio:4
> [    0.554077] intel_pstate: controlling: cpu 7
> [    0.554098] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.554099] intel_pstate: cpu:7 min_policy_perf:4
> max_policy_perf:31
> [    0.554099] intel_pstate: cpu:7 global_min:0 global_max:44
> [    0.554100] intel_pstate: cpu:7 max_perf_ratio:31 min_perf_ratio:4
> [    0.554104] intel_pstate: HWP enabled
> [    2.183669] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183675] intel_pstate: cpu:6 min_policy_perf:4
> max_policy_perf:31
> [    2.183677] intel_pstate: cpu:6 global_min:4 global_max:44
> [    2.183679] intel_pstate: cpu:6 max_perf_ratio:31 min_perf_ratio:4
> [    2.183710] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183711] intel_pstate: cpu:4 min_policy_perf:4
> max_policy_perf:31
> [    2.183713] intel_pstate: cpu:4 global_min:4 global_max:44
> [    2.183715] intel_pstate: cpu:4 max_perf_ratio:31 min_perf_ratio:4
> [    2.183742] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183744] intel_pstate: cpu:2 min_policy_perf:4
> max_policy_perf:31
> [    2.183745] intel_pstate: cpu:2 global_min:4 global_max:44
> [    2.183747] intel_pstate: cpu:2 max_perf_ratio:31 min_perf_ratio:4
> [    2.183773] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183775] intel_pstate: cpu:0 min_policy_perf:4
> max_policy_perf:31
> [    2.183776] intel_pstate: cpu:0 global_min:4 global_max:44
> [    2.183777] intel_pstate: cpu:0 max_perf_ratio:31 min_perf_ratio:4
> [    2.183803] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183805] intel_pstate: cpu:7 min_policy_perf:4
> max_policy_perf:31
> [    2.183806] intel_pstate: cpu:7 global_min:4 global_max:44
> [    2.183807] intel_pstate: cpu:7 max_perf_ratio:31 min_perf_ratio:4
> [    2.183831] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183833] intel_pstate: cpu:5 min_policy_perf:4
> max_policy_perf:31
> [    2.183834] intel_pstate: cpu:5 global_min:4 global_max:44
> [    2.183836] intel_pstate: cpu:5 max_perf_ratio:31 min_perf_ratio:4
> [    2.183862] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183864] intel_pstate: cpu:3 min_policy_perf:4
> max_policy_perf:31
> [    2.183865] intel_pstate: cpu:3 global_min:4 global_max:44
> [    2.183867] intel_pstate: cpu:3 max_perf_ratio:31 min_perf_ratio:4
> [    2.183893] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183895] intel_pstate: cpu:1 min_policy_perf:4
> max_policy_perf:31
> [    2.183896] intel_pstate: cpu:1 global_min:4 global_max:44
> [    2.183898] intel_pstate: cpu:1 max_perf_ratio:31 min_perf_ratio:4
> 
> > > I think that it worked because the MSR was read every time
> > > intel_pstate ran, so it got updated at one point and stayed that
> > > way.
> > 
> > But here HWP in active mode is getting used. So it should have
> > fewer
> > request calls to set accept via cpufreq set_policy()
> > 
> >  callback or with some HWP interrupt.
>