Re: [RFC PATCH 6/8] cpufreq: intel_pstate: Remove iowait boost

["Rafael J. Wysocki" <rafael@kernel.org>]

+Srinivas who can say more about the reasons why iowait boosting makes
a difference for intel_pstate than I do.

On Thu, Sep 5, 2024 at 11:27 AM Christian Loehle
<christian.loehle@arm.com> wrote:
>
> Analogous to schedutil, remove iowait boost for the same reasons.

Well, first of all, iowait boosting was added to intel_pstate to help
some workloads that otherwise were underperforming.  I'm not sure if
you can simply remove it without introducing performance regressions
in those workloads.

While you can argue that it is not useful in schedutil any more due to
the improved scheduler input for it, you can hardly extend that
argument to intel_pstate because it doesn't use all of the scheduler
input used by schedutil.

Also, the EAS and UCLAMP_MAX arguments are not applicable to
intel_pstate because it doesn't support any of them.

This applies to the ondemand cpufreq governor either.


> Signed-off-by: Christian Loehle <christian.loehle@arm.com>
> ---
>  drivers/cpufreq/intel_pstate.c | 50 ++--------------------------------
>  1 file changed, 3 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index c0278d023cfc..7f30b2569bb3 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -191,7 +191,6 @@ struct global_params {
>   * @policy:            CPUFreq policy value
>   * @update_util:       CPUFreq utility callback information
>   * @update_util_set:   CPUFreq utility callback is set
> - * @iowait_boost:      iowait-related boost fraction
>   * @last_update:       Time of the last update.
>   * @pstate:            Stores P state limits for this CPU
>   * @vid:               Stores VID limits for this CPU
> @@ -245,7 +244,6 @@ struct cpudata {
>         struct acpi_processor_performance acpi_perf_data;
>         bool valid_pss_table;
>  #endif
> -       unsigned int iowait_boost;
>         s16 epp_powersave;
>         s16 epp_policy;
>         s16 epp_default;
> @@ -2136,28 +2134,7 @@ static inline void intel_pstate_update_util_hwp_local(struct cpudata *cpu,
>  {
>         cpu->sample.time = time;
>
> -       if (cpu->sched_flags & SCHED_CPUFREQ_IOWAIT) {
> -               bool do_io = false;
> -
> -               cpu->sched_flags = 0;
> -               /*
> -                * Set iowait_boost flag and update time. Since IO WAIT flag
> -                * is set all the time, we can't just conclude that there is
> -                * some IO bound activity is scheduled on this CPU with just
> -                * one occurrence. If we receive at least two in two
> -                * consecutive ticks, then we treat as boost candidate.
> -                */
> -               if (time_before64(time, cpu->last_io_update + 2 * TICK_NSEC))
> -                       do_io = true;
> -
> -               cpu->last_io_update = time;
> -
> -               if (do_io)
> -                       intel_pstate_hwp_boost_up(cpu);
> -
> -       } else {
> -               intel_pstate_hwp_boost_down(cpu);
> -       }
> +       intel_pstate_hwp_boost_down(cpu);
>  }
>
>  static inline void intel_pstate_update_util_hwp(struct update_util_data *data,
> @@ -2240,9 +2217,6 @@ static inline int32_t get_target_pstate(struct cpudata *cpu)
>         busy_frac = div_fp(sample->mperf << cpu->aperf_mperf_shift,
>                            sample->tsc);
>
> -       if (busy_frac < cpu->iowait_boost)
> -               busy_frac = cpu->iowait_boost;
> -
>         sample->busy_scaled = busy_frac * 100;
>
>         target = READ_ONCE(global.no_turbo) ?
> @@ -2303,7 +2277,7 @@ static void intel_pstate_adjust_pstate(struct cpudata *cpu)
>                 sample->aperf,
>                 sample->tsc,
>                 get_avg_frequency(cpu),
> -               fp_toint(cpu->iowait_boost * 100));
> +               0);
>  }
>
>  static void intel_pstate_update_util(struct update_util_data *data, u64 time,
> @@ -2317,24 +2291,6 @@ static void intel_pstate_update_util(struct update_util_data *data, u64 time,
>                 return;
>
>         delta_ns = time - cpu->last_update;
> -       if (flags & SCHED_CPUFREQ_IOWAIT) {
> -               /* Start over if the CPU may have been idle. */
> -               if (delta_ns > TICK_NSEC) {
> -                       cpu->iowait_boost = ONE_EIGHTH_FP;
> -               } else if (cpu->iowait_boost >= ONE_EIGHTH_FP) {
> -                       cpu->iowait_boost <<= 1;
> -                       if (cpu->iowait_boost > int_tofp(1))
> -                               cpu->iowait_boost = int_tofp(1);
> -               } else {
> -                       cpu->iowait_boost = ONE_EIGHTH_FP;
> -               }
> -       } else if (cpu->iowait_boost) {
> -               /* Clear iowait_boost if the CPU may have been idle. */
> -               if (delta_ns > TICK_NSEC)
> -                       cpu->iowait_boost = 0;
> -               else
> -                       cpu->iowait_boost >>= 1;
> -       }
>         cpu->last_update = time;
>         delta_ns = time - cpu->sample.time;
>         if ((s64)delta_ns < INTEL_PSTATE_SAMPLING_INTERVAL)
> @@ -2832,7 +2788,7 @@ static void intel_cpufreq_trace(struct cpudata *cpu, unsigned int trace_type, in
>                 sample->aperf,
>                 sample->tsc,
>                 get_avg_frequency(cpu),
> -               fp_toint(cpu->iowait_boost * 100));
> +               0);
>  }
>
>  static void intel_cpufreq_hwp_update(struct cpudata *cpu, u32 min, u32 max,
> --
> 2.34.1
>