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: <20230808111325.8600-1-TonyWWang-oc@zhaoxin.com>
In-Reply-To: <20230808111325.8600-1-TonyWWang-oc@zhaoxin.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 22 Aug 2023 22:01:28 +0200
Message-ID: <CAJZ5v0h8M-hNJfRTSxtVmfmpF09h9zmNmG-e=iMemzPwsK50Zg@mail.gmail.com>
Subject: Re: [PATCH v2] cpufreq: ACPI: add ITMT support when CPPC enabled
To:     Tony W Wang-oc <TonyWWang-oc@zhaoxin.com>
Cc:     rafael@kernel.org, viresh.kumar@linaro.org,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        CobeChen@zhaoxin.com, TimGuo@zhaoxin.com, LeoLiu-oc@zhaoxin.com,
        LindaChai@zhaoxin.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Tue, Aug 8, 2023 at 1:13 PM Tony W Wang-oc <TonyWWang-oc@zhaoxin.com> wrote:
>
> The _CPC method can get per-core highest frequency.

Well, not exactly.  A more precise way to say this would be "The
per-core highest frequency can be obtained via CPPC."

> The highest frequency may varies between cores which mean cores can

"may vary" and "which means"

> running at different max frequency, so can use it as a core priority

"can run", but it would be better to say "may run".

> and give a hint to scheduler in order to put critical task to the
> higher priority core.

Well, roughly speaking ...

You should really talk about ITMT and how it can be hooked up to this.

> Signed-off-by: Tony W Wang-oc <TonyWWang-oc@zhaoxin.com>
> ---
> v1->v2: Fix build errors reported by kernel test robot
>
>  arch/x86/kernel/itmt.c         |  2 ++
>  drivers/cpufreq/acpi-cpufreq.c | 59 ++++++++++++++++++++++++++++++----
>  2 files changed, 54 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/itmt.c b/arch/x86/kernel/itmt.c
> index ee4fe8cdb857..b49ac8ecbbd6 100644
> --- a/arch/x86/kernel/itmt.c
> +++ b/arch/x86/kernel/itmt.c
> @@ -122,6 +122,7 @@ int sched_set_itmt_support(void)
>
>         return 0;
>  }
> +EXPORT_SYMBOL_GPL(sched_set_itmt_support);

This requires an ACK from the x86 maintainers.

>
>  /**
>   * sched_clear_itmt_support() - Revoke platform's support of ITMT
> @@ -181,3 +182,4 @@ void sched_set_itmt_core_prio(int prio, int cpu)
>  {
>         per_cpu(sched_core_priority, cpu) = prio;
>  }
> +EXPORT_SYMBOL_GPL(sched_set_itmt_core_prio);

And same here.

> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index b2f05d27167e..5733323e04ac 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -628,28 +628,35 @@ static int acpi_cpufreq_blacklist(struct cpuinfo_x86 *c)
>  #endif
>
>  #ifdef CONFIG_ACPI_CPPC_LIB
> -static u64 get_max_boost_ratio(unsigned int cpu)
> +static void cpufreq_get_core_perf(int cpu, u64 *highest_perf, u64 *nominal_perf)

This is not a cpufreq core function, so please use a different prefix
in its name.

>  {
>         struct cppc_perf_caps perf_caps;
> -       u64 highest_perf, nominal_perf;
>         int ret;
>
>         if (acpi_pstate_strict)
> -               return 0;
> +               return;
>
>         ret = cppc_get_perf_caps(cpu, &perf_caps);
>         if (ret) {
>                 pr_debug("CPU%d: Unable to get performance capabilities (%d)\n",
>                          cpu, ret);
> -               return 0;
> +               return;
>         }
>
>         if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> -               highest_perf = amd_get_highest_perf();
> +               *highest_perf = amd_get_highest_perf();
>         else
> -               highest_perf = perf_caps.highest_perf;
> +               *highest_perf = perf_caps.highest_perf;
> +
> +       *nominal_perf = perf_caps.nominal_perf;
> +       return;
> +}
>
> -       nominal_perf = perf_caps.nominal_perf;
> +static u64 get_max_boost_ratio(unsigned int cpu)
> +{
> +       u64 highest_perf, nominal_perf;
> +
> +       cpufreq_get_core_perf(cpu, &highest_perf, &nominal_perf);
>
>         if (!highest_perf || !nominal_perf) {
>                 pr_debug("CPU%d: highest or nominal performance missing\n", cpu);
> @@ -663,8 +670,44 @@ static u64 get_max_boost_ratio(unsigned int cpu)
>
>         return div_u64(highest_perf << SCHED_CAPACITY_SHIFT, nominal_perf);
>  }
> +
> +static void cpufreq_sched_itmt_work_fn(struct work_struct *work)

A similar comment applies here.

> +{
> +       sched_set_itmt_support();
> +}
> +
> +static DECLARE_WORK(sched_itmt_work, cpufreq_sched_itmt_work_fn);
> +
> +static void cpufreq_set_itmt_prio(int cpu)
> +{
> +       u64 highest_perf, nominal_perf;
> +       static u32 max_highest_perf = 0, min_highest_perf = U32_MAX;
> +
> +       cpufreq_get_core_perf(cpu, &highest_perf, &nominal_perf);
> +
> +       sched_set_itmt_core_prio(highest_perf, cpu);
> +
> +       if (max_highest_perf <= min_highest_perf) {
> +               if (highest_perf > max_highest_perf)
> +                       max_highest_perf = highest_perf;
> +
> +               if (highest_perf < min_highest_perf)
> +                       min_highest_perf = highest_perf;
> +
> +               if (max_highest_perf > min_highest_perf) {
> +                       /*
> +                        * This code can be run during CPU online under the
> +                        * CPU hotplug locks, so sched_set_itmt_support()
> +                        * cannot be called from here.  Queue up a work item
> +                        * to invoke it.
> +                        */
> +                       schedule_work(&sched_itmt_work);
> +               }

This potentially runs before ITMT priorities are set for all CPUs.
Isn't it a problem?

> +       }
> +}
>  #else
>  static inline u64 get_max_boost_ratio(unsigned int cpu) { return 0; }
> +static void cpufreq_set_itmt_prio(int cpu) { return; }
>  #endif
>
>  static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> @@ -870,6 +913,8 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
>         /* notify BIOS that we exist */
>         acpi_processor_notify_smm(THIS_MODULE);
>
> +       cpufreq_set_itmt_prio(cpu);
> +
>         pr_debug("CPU%u - ACPI performance management activated.\n", cpu);
>         for (i = 0; i < perf->state_count; i++)
>                 pr_debug("     %cP%d: %d MHz, %d mW, %d uS\n",
> --