Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230509180503.739208-1-wyes.karny@amd.com> <20230509180503.739208-2-wyes.karny@amd.com>
 <CAJZ5v0hN7AxkSf7=8-xP1Pb_7bA2Ba6nGUiK45q01uo_MFa1qQ@mail.gmail.com>
In-Reply-To: <CAJZ5v0hN7AxkSf7=8-xP1Pb_7bA2Ba6nGUiK45q01uo_MFa1qQ@mail.gmail.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 9 May 2023 20:39:22 +0200
Message-ID: <CAJZ5v0gb9TpH1qCqhqAb28c7uRRk8=iufTkzc5aCpC=OJm8QaQ@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] cpufreq/schedutil: Remove fast_switch_possible
 flag if driver doesn't set fast_switch
To:     Wyes Karny <wyes.karny@amd.com>
Cc:     ray.huang@amd.com, rafael@kernel.org, viresh.kumar@linaro.org,
        srinivas.pandruvada@linux.intel.com, lenb@kernel.org,
        mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        joel@joelfernandes.org, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Tue, May 9, 2023 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, May 9, 2023 at 8:06 PM Wyes Karny <wyes.karny@amd.com> wrote:
> >
> > The set value of `fast_switch_enabled` indicates that fast_switch
> > callback is set.

BTW, this is not true.  The ACPI cpufreq driver has fast_switch, but
it doesn't always use it.

> >  For some drivers such as amd_pstate and intel_pstate,
> > the adjust_perf callback is used but it still sets
> > `fast_switch_possible` flag. This is because this flag also decides
> > whether schedutil governor selects adjust_perf function for frequency
> > update. This condition in the schedutil governor forces the scaling
> > driver to set the `fast_switch_possible` flag.

Yes, it does, but setting this flag is not sufficient.

> > Remove `fast_switch_enabled` check when schedutil decides to select
> > adjust_perf function for frequency update. Thus removing this drivers
> > are now free to remove `fast_switch_possible` flag if they don't use
> > fast_switch callback.
> >
> > This issue becomes apparent when aperf/mperf overflow occurs.  When this
> > happens, kernel disables frequency invariance calculation which causes
> > schedutil to fallback to sugov_update_single_freq which currently relies
> > on the fast_switch callback.
> >
> > Normal flow:
> >   sugov_update_single_perf
> >     cpufreq_driver_adjust_perf
> >       cpufreq_driver->adjust_perf
> >
> > Error case flow:
> >   sugov_update_single_perf
> >     sugov_update_single_freq  <-- This is chosen because the freq invariant is disabled due to aperf/mperf overflow
> >       cpufreq_driver_fast_switch
> >          cpufreq_driver->fast_switch <-- Here NULL pointer dereference is happening, because fast_switch is not set
>
> So you need to set fast_switch.
>
> Please read the comment in sugov_update_single_perf().  It explains
> why adjust_perf is not used when scale invariance is not enabled: the
> mapping between the performance levels and frequency are not generally
> defined in that case and it is up to the driver to figure out what
> perf level to use to get the given frequency.  And this is exactly why
> fast_switch is not optional: because scale invariance may be disabled.
>
> Please feel free to update the documentation to clarify this, but the
> way to fix the issue is to implement fast_switch in the driver.
>
> > Fixes: a61dec744745 ("cpufreq: schedutil: Avoid missing updates for one-CPU policies")
> > Signed-off-by: Wyes Karny <wyes.karny@amd.com>
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/cpufreq/amd-pstate.c     | 10 +++++++---
> >  drivers/cpufreq/cpufreq.c        | 20 +++++++++++++++++++-
> >  drivers/cpufreq/intel_pstate.c   |  3 +--
> >  include/linux/cpufreq.h          |  1 +
> >  kernel/sched/cpufreq_schedutil.c |  2 +-
> >  5 files changed, 29 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > index 5a3d4aa0f45a..007bfe724a6a 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -671,8 +671,14 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
> >         /* It will be updated by governor */
> >         policy->cur = policy->cpuinfo.min_freq;
> >
> > +       /**
> > +        * For shared memory system frequency update takes time that's why
> > +        * do this in deferred kthread context.
> > +        */
> >         if (boot_cpu_has(X86_FEATURE_CPPC))
> > -               policy->fast_switch_possible = true;
> > +               current_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
> > +       else
> > +               current_pstate_driver->adjust_perf = NULL;
> >
> >         ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
> >                                    FREQ_QOS_MIN, policy->cpuinfo.min_freq);
> > @@ -697,8 +703,6 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
> >         policy->driver_data = cpudata;
> >
> >         amd_pstate_boost_init(cpudata);
> > -       if (!current_pstate_driver->adjust_perf)
> > -               current_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
> >
> >         return 0;
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 6b52ebe5a890..366747012104 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -501,6 +501,13 @@ void cpufreq_enable_fast_switch(struct cpufreq_policy *policy)
> >         if (!policy->fast_switch_possible)
> >                 return;
> >
> > +       /**
> > +        * It's not expected driver's fast_switch callback is not set
> > +        * even fast_switch_possible is true.
> > +        */
> > +       if (WARN_ON(!cpufreq_driver_has_fast_switch()))
> > +               return;
> > +
> >         mutex_lock(&cpufreq_fast_switch_lock);
> >         if (cpufreq_fast_switch_count >= 0) {
> >                 cpufreq_fast_switch_count++;
> > @@ -2143,6 +2150,17 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> >  }
> >  EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
> >
> > +/**
> > + * cpufreq_driver_has_fast_switch - Check "fast switch" callback.
> > + *
> > + * Return 'true' if the ->fast_switch callback is present for the
> > + * current driver or 'false' otherwise.
> > + */
> > +bool cpufreq_driver_has_fast_switch(void)
> > +{
> > +       return !!cpufreq_driver->fast_switch;
> > +}
> > +
> >  /**
> >   * cpufreq_driver_adjust_perf - Adjust CPU performance level in one go.
> >   * @cpu: Target CPU.
> > @@ -2157,7 +2175,7 @@ EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
> >   * and it is expected to select a suitable performance level equal to or above
> >   * @min_perf and preferably equal to or below @target_perf.
> >   *
> > - * This function must not be called if policy->fast_switch_enabled is unset.
> > + * By default this function takes the fast frequency update path.
> >   *
> >   * Governors calling this function must guarantee that it will never be invoked
> >   * twice in parallel for the same CPU and that it will never be called in
> > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > index 2548ec92faa2..007893514c87 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -2698,8 +2698,6 @@ static int __intel_pstate_cpu_init(struct cpufreq_policy *policy)
> >
> >         intel_pstate_init_acpi_perf_limits(policy);
> >
> > -       policy->fast_switch_possible = true;
> > -
> >         return 0;
> >  }
> >
> > @@ -2955,6 +2953,7 @@ static int intel_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >         if (ret)
> >                 return ret;
> >
> > +       policy->fast_switch_possible = true;

I'm not sure what this is about.  Is it a cleanup of intel_pstate?

> >         policy->cpuinfo.transition_latency = INTEL_CPUFREQ_TRANSITION_LATENCY;
> >         /* This reflects the intel_pstate_get_cpu_pstates() setting. */
> >         policy->cur = policy->cpuinfo.min_freq;
> > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> > index 26e2eb399484..7a32cfca26c9 100644
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -604,6 +604,7 @@ struct cpufreq_governor {
> >  /* Pass a target to the cpufreq driver */
> >  unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> >                                         unsigned int target_freq);
> > +bool cpufreq_driver_has_fast_switch(void);
> >  void cpufreq_driver_adjust_perf(unsigned int cpu,
> >                                 unsigned long min_perf,
> >                                 unsigned long target_perf,
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index e3211455b203..f993ecf731a9 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -776,7 +776,7 @@ static int sugov_start(struct cpufreq_policy *policy)
> >
> >         if (policy_is_shared(policy))
> >                 uu = sugov_update_shared;
> > -       else if (policy->fast_switch_enabled && cpufreq_driver_has_adjust_perf())
> > +       else if (cpufreq_driver_has_adjust_perf())
> >                 uu = sugov_update_single_perf;
> >         else
> >                 uu = sugov_update_single_freq;
> > --