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

[srinivas pandruvada]

On Tue, 2024-06-04 at 18:46 +0200, Rafael J. Wysocki wrote:
> On Tue, Jun 4, 2024 at 6:41 PM srinivas pandruvada
> <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> > 
> > On Tue, 2024-06-04 at 18:32 +0800, Xi Ruoyao wrote:
> > > On Tue, 2024-06-04 at 03:29 -0700, srinivas pandruvada wrote:
> > > > On Tue, 2024-06-04 at 17:30 +0800, Xi Ruoyao wrote:
> > > > > On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote:
> > > > > 
> > > > > > > > Second, a delayed work can be added to check the MSR
> > > > > > > > long
> > > > > > > > enough
> > > > > > > > after
> > > > > > > > initialization and update global.turbo_disabled if it
> > > > > > > > is 1.
> > > > > > > > However,
> > > > > > > > that would require some code surgery.
> > > > > > > 
> > > > > > Something like the attached which does same way as user
> > > > > > space
> > > > > > no_turbo
> > > > > > update.
> > > > > 
> > > > > >  static int intel_pstate_register_driver(struct
> > > > > > cpufreq_driver
> > > > > > *driver)
> > > > > >  {
> > > > > >         int ret;
> > > > > > @@ -3114,6 +3137,9 @@ static int
> > > > > > intel_pstate_register_driver(struct cpufreq_driver *driver)
> > > > > >         global.turbo_disabled = turbo_is_disabled();
> > > > > >         global.no_turbo = global.turbo_disabled;
> > > > > > 
> > > > > > +       if (global.turbo_disabled)
> > > > > > +               schedule_delayed_work(&turbo_work, HZ);
> > > > > > +
> > > > > 
> > > > > I have to change it to 20 * HZ to make it work for me.  15 *
> > > > > HZ
> > > > > does
> > > > > not
> > > > > work.
> > > > 
> > > > Is there any consistency or it is changing every time?
> > > 
> > > It seems consistent.
> > With such a delay, I am not sure how this even worked before.
> > 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?
> 
> 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.