linux-kernel - Re: [PATCH v2] thermal: intel_powerclamp: Fix cur

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 2 Feb 2023 15:51:28 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc:     rafael@...nel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, daniel.lezcano@...aro.org,
        rui.zhang@...el.com, stable@...r.kernel.org
Subject: Re: [PATCH v2] thermal: intel_powerclamp: Fix cur_state for multi
 package system

On Wed, Feb 1, 2023 at 9:39 PM Srinivas Pandruvada
<srinivas.pandruvada@...ux.intel.com> wrote:
>
> The powerclamp cooling device cur_state shows actual idle observed by
> package C-state idle counters. But the implementation is not sufficient
> for multi package or multi die system. The cur_state value is incorrect.
> On these systems, these counters must be read from each package/die and
> somehow aggregate them. But there is no good method for aggregation.
>
> It was not a problem when explicit CPU model addition was required to
> enable intel powerclamp. In this way certain CPU models could have
> been avoided. But with the removal of CPU model check with the
> availability of Package C-state counters, the driver is loaded on most
> of the recent systems.
>
> For multi package/die systems, just show the actual target idle state,
> the system is trying to achieve. In powerclamp this is the user set
> state minus one.
>
> Also there is no use of starting a worker thread for polling package
> C-state counters and applying any compensation for multiple package
> or multiple die systems.
>
> Fixes: b721ca0d1927 ("thermal/powerclamp: remove cpu whitelist")
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> Cc: stable@...r.kernel.org # 4.14+
> ---
> v2:
> Changed: (true == clamping) to (clamping)
> Updated commit description for the last paragraph
>
>  drivers/thermal/intel/intel_powerclamp.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c
> index b80e25ec1261..2f4cbfdf26a0 100644
> --- a/drivers/thermal/intel/intel_powerclamp.c
> +++ b/drivers/thermal/intel/intel_powerclamp.c
> @@ -57,6 +57,7 @@
>
>  static unsigned int target_mwait;
>  static struct dentry *debug_dir;
> +static bool poll_pkg_cstate_enable;
>
>  /* user selected target */
>  static unsigned int set_target_ratio;
> @@ -261,6 +262,9 @@ static unsigned int get_compensation(int ratio)
>  {
>         unsigned int comp = 0;
>
> +       if (!poll_pkg_cstate_enable)
> +               return 0;
> +
>         /* we only use compensation if all adjacent ones are good */
>         if (ratio == 1 &&
>                 cal_data[ratio].confidence >= CONFIDENCE_OK &&
> @@ -519,7 +523,8 @@ static int start_power_clamp(void)
>         control_cpu = cpumask_first(cpu_online_mask);
>
>         clamping = true;
> -       schedule_delayed_work(&poll_pkg_cstate_work, 0);
> +       if (poll_pkg_cstate_enable)
> +               schedule_delayed_work(&poll_pkg_cstate_work, 0);
>
>         /* start one kthread worker per online cpu */
>         for_each_online_cpu(cpu) {
> @@ -585,11 +590,15 @@ static int powerclamp_get_max_state(struct thermal_cooling_device *cdev,
>  static int powerclamp_get_cur_state(struct thermal_cooling_device *cdev,
>                                  unsigned long *state)
>  {
> -       if (true == clamping)
> -               *state = pkg_cstate_ratio_cur;
> -       else
> +       if (clamping) {
> +               if (poll_pkg_cstate_enable)
> +                       *state = pkg_cstate_ratio_cur;
> +               else
> +                       *state = set_target_ratio;
> +       } else {
>                 /* to save power, do not poll idle ratio while not clamping */
>                 *state = -1; /* indicates invalid state */
> +       }
>
>         return 0;
>  }
> @@ -712,6 +721,9 @@ static int __init powerclamp_init(void)
>                 goto exit_unregister;
>         }
>
> +       if (topology_max_packages() == 1 && topology_max_die_per_package() == 1)
> +               poll_pkg_cstate_enable = true;
> +
>         cooling_dev = thermal_cooling_device_register("intel_powerclamp", NULL,
>                                                 &powerclamp_cooling_ops);
>         if (IS_ERR(cooling_dev)) {
> --

Applied as 6.3 material, thanks!