* Re: [PATCH v1] thermal: gov_step_wise: Go straight to instance->lower when mitigation is over
2024-06-22 12:26 [PATCH v1] thermal: gov_step_wise: Go straight to instance->lower when mitigation is over Rafael J. Wysocki
@ 2024-06-24 18:43 ` Rafael J. Wysocki
2024-06-24 19:18 ` Steev Klimaszewski
2024-06-25 7:34 ` Johan Hovold
2 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2024-06-24 18:43 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM, LKML, Daniel Lezcano, Lukasz Luba, Srinivas Pandruvada,
Zhang Rui, Jens Glathe, Steev Klimaszewski, Johan Hovold
On Sat, Jun 22, 2024 at 2:28 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Commit b6846826982b ("thermal: gov_step_wise: Restore passive polling
> management") attempted to fix a Step-Wise thermal governor issue
> introduced by commit 042a3d80f118 ("thermal: core: Move passive polling
> management to the core"), which caused the governor to leave cooling
> devices in high states, by partially revering that commit.
>
> However, this turns out to be insufficient on some systems due to
> interactions between the governor code restored by commit b6846826982b
> and the passive polling management in the thermal core.
>
> For this reason, revert commit b6846826982b and make the governor set
> the target cooling device state to the "lower" one as soon as the zone
> temperature falls below the threshold of the trip point corresponding
> to the given thermal instance, which means that thermal mitigation is
> not necessary any more.
>
> Before this change the "lower" cooling device state would be reached in
> steps through the passive polling mechanism which was questionable for
> three reasons: (1) cooling device were kept in high states when that was
> not necessary (and it could adversely impact performance), (2) it only
> worked for thermal zones with nonzero passive_delay_jiffies value, and
> (3) passive polling belongs to the core and should not be hijacked by
> governors for their internal purposes.
>
> Fixes: b6846826982b ("thermal: gov_step_wise: Restore passive polling management")
> Closes: https://lore.kernel.org/linux-pm/6759ce9f-281d-4fcd-bb4c-b784a1cc5f6e@oldschoolsolutions.biz
> Reported-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> Tested-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/gov_step_wise.c | 23 +++++------------------
> 1 file changed, 5 insertions(+), 18 deletions(-)
>
> Index: linux-pm/drivers/thermal/gov_step_wise.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_step_wise.c
> +++ linux-pm/drivers/thermal/gov_step_wise.c
> @@ -55,7 +55,11 @@ static unsigned long get_target_state(st
> if (cur_state <= instance->lower)
> return THERMAL_NO_TARGET;
>
> - return clamp(cur_state - 1, instance->lower, instance->upper);
> + /*
> + * If 'throttle' is false, no mitigation is necessary, so
> + * request the lower state for this instance.
> + */
> + return instance->lower;
> }
>
> return instance->target;
> @@ -93,23 +97,6 @@ static void thermal_zone_trip_update(str
> if (instance->initialized && old_target == instance->target)
> continue;
>
> - if (trip->type == THERMAL_TRIP_PASSIVE) {
> - /*
> - * If the target state for this thermal instance
> - * changes from THERMAL_NO_TARGET to something else,
> - * ensure that the zone temperature will be updated
> - * (assuming enabled passive cooling) until it becomes
> - * THERMAL_NO_TARGET again, or the cooling device may
> - * not be reset to its initial state.
> - */
> - if (old_target == THERMAL_NO_TARGET &&
> - instance->target != THERMAL_NO_TARGET)
> - tz->passive++;
> - else if (old_target != THERMAL_NO_TARGET &&
> - instance->target == THERMAL_NO_TARGET)
> - tz->passive--;
> - }
> -
> instance->initialized = true;
>
> mutex_lock(&instance->cdev->lock);
>
If there is no feedback, I'm going to assume that this is fine with everybody.
Thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] thermal: gov_step_wise: Go straight to instance->lower when mitigation is over
2024-06-22 12:26 [PATCH v1] thermal: gov_step_wise: Go straight to instance->lower when mitigation is over Rafael J. Wysocki
2024-06-24 18:43 ` Rafael J. Wysocki
@ 2024-06-24 19:18 ` Steev Klimaszewski
2024-06-25 7:34 ` Johan Hovold
2 siblings, 0 replies; 5+ messages in thread
From: Steev Klimaszewski @ 2024-06-24 19:18 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM, LKML, Daniel Lezcano, Lukasz Luba, Srinivas Pandruvada,
Zhang Rui, Jens Glathe, Johan Hovold
Hi Rafael,
On Sat, Jun 22, 2024 at 7:28 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Commit b6846826982b ("thermal: gov_step_wise: Restore passive polling
> management") attempted to fix a Step-Wise thermal governor issue
> introduced by commit 042a3d80f118 ("thermal: core: Move passive polling
> management to the core"), which caused the governor to leave cooling
> devices in high states, by partially revering that commit.
>
> However, this turns out to be insufficient on some systems due to
> interactions between the governor code restored by commit b6846826982b
> and the passive polling management in the thermal core.
>
> For this reason, revert commit b6846826982b and make the governor set
> the target cooling device state to the "lower" one as soon as the zone
> temperature falls below the threshold of the trip point corresponding
> to the given thermal instance, which means that thermal mitigation is
> not necessary any more.
>
> Before this change the "lower" cooling device state would be reached in
> steps through the passive polling mechanism which was questionable for
> three reasons: (1) cooling device were kept in high states when that was
> not necessary (and it could adversely impact performance), (2) it only
> worked for thermal zones with nonzero passive_delay_jiffies value, and
> (3) passive polling belongs to the core and should not be hijacked by
> governors for their internal purposes.
>
> Fixes: b6846826982b ("thermal: gov_step_wise: Restore passive polling management")
> Closes: https://lore.kernel.org/linux-pm/6759ce9f-281d-4fcd-bb4c-b784a1cc5f6e@oldschoolsolutions.biz
> Reported-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> Tested-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/gov_step_wise.c | 23 +++++------------------
> 1 file changed, 5 insertions(+), 18 deletions(-)
>
> Index: linux-pm/drivers/thermal/gov_step_wise.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_step_wise.c
> +++ linux-pm/drivers/thermal/gov_step_wise.c
> @@ -55,7 +55,11 @@ static unsigned long get_target_state(st
> if (cur_state <= instance->lower)
> return THERMAL_NO_TARGET;
>
> - return clamp(cur_state - 1, instance->lower, instance->upper);
> + /*
> + * If 'throttle' is false, no mitigation is necessary, so
> + * request the lower state for this instance.
> + */
> + return instance->lower;
> }
>
> return instance->target;
> @@ -93,23 +97,6 @@ static void thermal_zone_trip_update(str
> if (instance->initialized && old_target == instance->target)
> continue;
>
> - if (trip->type == THERMAL_TRIP_PASSIVE) {
> - /*
> - * If the target state for this thermal instance
> - * changes from THERMAL_NO_TARGET to something else,
> - * ensure that the zone temperature will be updated
> - * (assuming enabled passive cooling) until it becomes
> - * THERMAL_NO_TARGET again, or the cooling device may
> - * not be reset to its initial state.
> - */
> - if (old_target == THERMAL_NO_TARGET &&
> - instance->target != THERMAL_NO_TARGET)
> - tz->passive++;
> - else if (old_target != THERMAL_NO_TARGET &&
> - instance->target == THERMAL_NO_TARGET)
> - tz->passive--;
> - }
> -
> instance->initialized = true;
>
> mutex_lock(&instance->cdev->lock);
>
>
>
I've tested this here against 6.10.0-rc5 and it looks like it does
what it says on the tin now. Locks to low speeds at thermal and
(rather quickly) unlocks once it's no longer in the "danger zone"
Tested-by: Steev Klimaszewski <steev@kali.org>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] thermal: gov_step_wise: Go straight to instance->lower when mitigation is over
2024-06-22 12:26 [PATCH v1] thermal: gov_step_wise: Go straight to instance->lower when mitigation is over Rafael J. Wysocki
2024-06-24 18:43 ` Rafael J. Wysocki
2024-06-24 19:18 ` Steev Klimaszewski
@ 2024-06-25 7:34 ` Johan Hovold
2024-06-25 12:33 ` Rafael J. Wysocki
2 siblings, 1 reply; 5+ messages in thread
From: Johan Hovold @ 2024-06-25 7:34 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM, LKML, Daniel Lezcano, Lukasz Luba, Srinivas Pandruvada,
Zhang Rui, Jens Glathe, Steev Klimaszewski, Johan Hovold
On Sat, Jun 22, 2024 at 02:26:33PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Commit b6846826982b ("thermal: gov_step_wise: Restore passive polling
> management") attempted to fix a Step-Wise thermal governor issue
> introduced by commit 042a3d80f118 ("thermal: core: Move passive polling
> management to the core"), which caused the governor to leave cooling
> devices in high states, by partially revering that commit.
typo: reverting
> However, this turns out to be insufficient on some systems due to
> interactions between the governor code restored by commit b6846826982b
> and the passive polling management in the thermal core.
Care to elaborate on what went wrong here? In my test of the previous
fix I saw the frequency ramping up in steps as expected when the
temperature dropped. Under what circumstances would that fail to happen?
> For this reason, revert commit b6846826982b and make the governor set
> the target cooling device state to the "lower" one as soon as the zone
> temperature falls below the threshold of the trip point corresponding
> to the given thermal instance, which means that thermal mitigation is
> not necessary any more.
>
> Before this change the "lower" cooling device state would be reached in
> steps through the passive polling mechanism which was questionable for
> three reasons: (1) cooling device were kept in high states when that was
> not necessary (and it could adversely impact performance), (2) it only
> worked for thermal zones with nonzero passive_delay_jiffies value, and
> (3) passive polling belongs to the core and should not be hijacked by
> governors for their internal purposes.
I've tested this patch on the Lenovo ThinkPad X13s, where I could
reproduce the rc1 regression, and things works as intended with the
fix applied to rc5:
Tested-by: Johan Hovold <johan+linaro@kernel.org>
The CPU frequency still oscillates heavily but now with a more
sawtoothed curve.
Not sure if it helps with performance, though, as running the CPU at
full speed as soon as we drop below the threshold (with hysteresis)
also means that we get back to running at the lowest frequency even
faster.
Johan
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] thermal: gov_step_wise: Go straight to instance->lower when mitigation is over
2024-06-25 7:34 ` Johan Hovold
@ 2024-06-25 12:33 ` Rafael J. Wysocki
0 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2024-06-25 12:33 UTC (permalink / raw)
To: Johan Hovold
Cc: Rafael J. Wysocki, Linux PM, LKML, Daniel Lezcano, Lukasz Luba,
Srinivas Pandruvada, Zhang Rui, Jens Glathe, Steev Klimaszewski,
Johan Hovold
On Tue, Jun 25, 2024 at 9:34 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Sat, Jun 22, 2024 at 02:26:33PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Commit b6846826982b ("thermal: gov_step_wise: Restore passive polling
> > management") attempted to fix a Step-Wise thermal governor issue
> > introduced by commit 042a3d80f118 ("thermal: core: Move passive polling
> > management to the core"), which caused the governor to leave cooling
> > devices in high states, by partially revering that commit.
>
> typo: reverting
Thanks!
> > However, this turns out to be insufficient on some systems due to
> > interactions between the governor code restored by commit b6846826982b
> > and the passive polling management in the thermal core.
>
> Care to elaborate on what went wrong here? In my test of the previous
> fix I saw the frequency ramping up in steps as expected when the
> temperature dropped. Under what circumstances would that fail to happen?
System suspend-resume would interfere with that as it would call
thermal_zone_device_init().
> > For this reason, revert commit b6846826982b and make the governor set
> > the target cooling device state to the "lower" one as soon as the zone
> > temperature falls below the threshold of the trip point corresponding
> > to the given thermal instance, which means that thermal mitigation is
> > not necessary any more.
> >
> > Before this change the "lower" cooling device state would be reached in
> > steps through the passive polling mechanism which was questionable for
> > three reasons: (1) cooling device were kept in high states when that was
> > not necessary (and it could adversely impact performance), (2) it only
> > worked for thermal zones with nonzero passive_delay_jiffies value, and
> > (3) passive polling belongs to the core and should not be hijacked by
> > governors for their internal purposes.
>
> I've tested this patch on the Lenovo ThinkPad X13s, where I could
> reproduce the rc1 regression, and things works as intended with the
> fix applied to rc5:
>
> Tested-by: Johan Hovold <johan+linaro@kernel.org>
>
> The CPU frequency still oscillates heavily but now with a more
> sawtoothed curve.
>
> Not sure if it helps with performance, though, as running the CPU at
> full speed as soon as we drop below the threshold (with hysteresis)
> also means that we get back to running at the lowest frequency even
> faster.
True, but assuming that the reason for the mitigation is still there.
If it's actually gone, it's better to stop cooling as soon as it can
be done.
Thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread