1. Patchew
  2. linux
  3. View series

[PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable

Rafael J. Wysocki posted 1 patch 1 year, 10 months ago 
drivers/thermal/gov_power_allocator.c |    9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
[PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable
Posted by Rafael J. Wysocki 1 year, 10 months ago 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Notice that the passive field in struct thermal_zone_device is not
used by the Power Allocator governor itself and so the ordering of
its updates with respect to allow_maximum_power() or allocate_power()
does not matter.

Accordingly, make power_allocator_manage() update that field right
before returning, which allows the current value of it to be passed
directly to allow_maximum_power() without using the additional update
variable that can be dropped.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_power_allocator.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/thermal/gov_power_allocator.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_power_allocator.c
+++ linux-pm/drivers/thermal/gov_power_allocator.c
@@ -747,21 +747,18 @@ static void power_allocator_manage(struc
 {
 	struct power_allocator_params *params = tz->governor_data;
 	const struct thermal_trip *trip = params->trip_switch_on;
-	bool update;
 
 	lockdep_assert_held(&tz->lock);
 
 	if (trip && tz->temperature < trip->temperature) {
-		update = tz->passive;
-		tz->passive = 0;
 		reset_pid_controller(params);
-		allow_maximum_power(tz, update);
+		allow_maximum_power(tz, tz->passive);
+		tz->passive = 0;
 		return;
 	}
 
-	tz->passive = 1;
-
 	allocate_power(tz, params->trip_max->temperature);
+	tz->passive = 1;
 }
 
 static struct thermal_governor thermal_gov_power_allocator = {
Re: [PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable
Posted by Daniel Lezcano 1 year, 9 months ago 
On 10/04/2024 18:12, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notice that the passive field in struct thermal_zone_device is not
> used by the Power Allocator governor itself and so the ordering of
> its updates with respect to allow_maximum_power() or allocate_power()
> does not matter.
> 
> Accordingly, make power_allocator_manage() update that field right
> before returning, which allows the current value of it to be passed
> directly to allow_maximum_power() without using the additional update
> variable that can be dropped.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

The step_wise and the power allocator are changing the tz->passive 
values, so telling the core to start and stop the passive mitigation timer.

It looks strange that a plugin controls the core internal and not the 
opposite.

I'm wondering if it would not make sense to have the following ops:

	.start
	.stop

.start is called when the first trip point is crossed the way up
.stop is called when the first trip point is crossed the way down

  - The core is responsible to start and stop the passive mitigation timer.

  - the governors do no longer us tz->passive

The reset of the governor can happen at start or stop, as well as the 
device cooling states.


>   drivers/thermal/gov_power_allocator.c |    9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -747,21 +747,18 @@ static void power_allocator_manage(struc
>   {
>   	struct power_allocator_params *params = tz->governor_data;
>   	const struct thermal_trip *trip = params->trip_switch_on;
> -	bool update;
>   
>   	lockdep_assert_held(&tz->lock);
>   
>   	if (trip && tz->temperature < trip->temperature) {
> -		update = tz->passive;
> -		tz->passive = 0;
>   		reset_pid_controller(params);
> -		allow_maximum_power(tz, update);
> +		allow_maximum_power(tz, tz->passive);
> +		tz->passive = 0;
>   		return;
>   	}
>   
> -	tz->passive = 1;
> -
>   	allocate_power(tz, params->trip_max->temperature);
> +	tz->passive = 1;
>   }
>   
>   static struct thermal_governor thermal_gov_power_allocator = {
> 
> 
> 

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Re: [PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable
Posted by Rafael J. Wysocki 1 year, 9 months ago 
On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Notice that the passive field in struct thermal_zone_device is not
> > used by the Power Allocator governor itself and so the ordering of
> > its updates with respect to allow_maximum_power() or allocate_power()
> > does not matter.
> >
> > Accordingly, make power_allocator_manage() update that field right
> > before returning, which allows the current value of it to be passed
> > directly to allow_maximum_power() without using the additional update
> > variable that can be dropped.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
>
> The step_wise and the power allocator are changing the tz->passive
> values, so telling the core to start and stop the passive mitigation timer.
>
> It looks strange that a plugin controls the core internal and not the
> opposite.
>
> I'm wondering if it would not make sense to have the following ops:
>
>         .start
>         .stop
>
> .start is called when the first trip point is crossed the way up
> .stop is called when the first trip point is crossed the way down
>
>   - The core is responsible to start and stop the passive mitigation timer.
>
>   - the governors do no longer us tz->passive
>
> The reset of the governor can happen at start or stop, as well as the
> device cooling states.

I have a patch that simply increments tz->passive when a passive trip
point is passed on the way up and decrements it when a passive trip
point is crossed on the way down.  It appears to work reasonably well.
Re: [PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable
Posted by Daniel Lezcano 1 year, 9 months ago 
On 23/04/2024 19:54, Rafael J. Wysocki wrote:
> On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Notice that the passive field in struct thermal_zone_device is not
>>> used by the Power Allocator governor itself and so the ordering of
>>> its updates with respect to allow_maximum_power() or allocate_power()
>>> does not matter.
>>>
>>> Accordingly, make power_allocator_manage() update that field right
>>> before returning, which allows the current value of it to be passed
>>> directly to allow_maximum_power() without using the additional update
>>> variable that can be dropped.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>
>> The step_wise and the power allocator are changing the tz->passive
>> values, so telling the core to start and stop the passive mitigation timer.
>>
>> It looks strange that a plugin controls the core internal and not the
>> opposite.
>>
>> I'm wondering if it would not make sense to have the following ops:
>>
>>          .start
>>          .stop
>>
>> .start is called when the first trip point is crossed the way up
>> .stop is called when the first trip point is crossed the way down
>>
>>    - The core is responsible to start and stop the passive mitigation timer.
>>
>>    - the governors do no longer us tz->passive
>>
>> The reset of the governor can happen at start or stop, as well as the
>> device cooling states.
> 
> I have a patch that simply increments tz->passive when a passive trip
> point is passed on the way up and decrements it when a passive trip
> point is crossed on the way down.  It appears to work reasonably well.

Does it make the governors getting ride of it ? Or at least not changing 
its value ?

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Re: [PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable
Posted by Rafael J. Wysocki 1 year, 9 months ago 
On Tue, Apr 23, 2024 at 8:00 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 23/04/2024 19:54, Rafael J. Wysocki wrote:
> > On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> Notice that the passive field in struct thermal_zone_device is not
> >>> used by the Power Allocator governor itself and so the ordering of
> >>> its updates with respect to allow_maximum_power() or allocate_power()
> >>> does not matter.
> >>>
> >>> Accordingly, make power_allocator_manage() update that field right
> >>> before returning, which allows the current value of it to be passed
> >>> directly to allow_maximum_power() without using the additional update
> >>> variable that can be dropped.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>> ---
> >>
> >> The step_wise and the power allocator are changing the tz->passive
> >> values, so telling the core to start and stop the passive mitigation timer.
> >>
> >> It looks strange that a plugin controls the core internal and not the
> >> opposite.
> >>
> >> I'm wondering if it would not make sense to have the following ops:
> >>
> >>          .start
> >>          .stop
> >>
> >> .start is called when the first trip point is crossed the way up
> >> .stop is called when the first trip point is crossed the way down
> >>
> >>    - The core is responsible to start and stop the passive mitigation timer.
> >>
> >>    - the governors do no longer us tz->passive
> >>
> >> The reset of the governor can happen at start or stop, as well as the
> >> device cooling states.
> >
> > I have a patch that simply increments tz->passive when a passive trip
> > point is passed on the way up and decrements it when a passive trip
> > point is crossed on the way down.  It appears to work reasonably well.
>
> Does it make the governors getting ride of it ? Or at least not changing
> its value ?

Not yet, but I'm going to update it this way.  The governors should
not mess up with tz->passive IMV.
Re: [PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable
Posted by Daniel Lezcano 1 year, 9 months ago 
On 23/04/2024 20:05, Rafael J. Wysocki wrote:
> On Tue, Apr 23, 2024 at 8:00 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 23/04/2024 19:54, Rafael J. Wysocki wrote:
>>> On Tue, Apr 23, 2024 at 7:35 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 10/04/2024 18:12, Rafael J. Wysocki wrote:
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>>>> Notice that the passive field in struct thermal_zone_device is not
>>>>> used by the Power Allocator governor itself and so the ordering of
>>>>> its updates with respect to allow_maximum_power() or allocate_power()
>>>>> does not matter.
>>>>>
>>>>> Accordingly, make power_allocator_manage() update that field right
>>>>> before returning, which allows the current value of it to be passed
>>>>> directly to allow_maximum_power() without using the additional update
>>>>> variable that can be dropped.
>>>>>
>>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>> ---
>>>>
>>>> The step_wise and the power allocator are changing the tz->passive
>>>> values, so telling the core to start and stop the passive mitigation timer.
>>>>
>>>> It looks strange that a plugin controls the core internal and not the
>>>> opposite.
>>>>
>>>> I'm wondering if it would not make sense to have the following ops:
>>>>
>>>>           .start
>>>>           .stop
>>>>
>>>> .start is called when the first trip point is crossed the way up
>>>> .stop is called when the first trip point is crossed the way down
>>>>
>>>>     - The core is responsible to start and stop the passive mitigation timer.
>>>>
>>>>     - the governors do no longer us tz->passive
>>>>
>>>> The reset of the governor can happen at start or stop, as well as the
>>>> device cooling states.
>>>
>>> I have a patch that simply increments tz->passive when a passive trip
>>> point is passed on the way up and decrements it when a passive trip
>>> point is crossed on the way down.  It appears to work reasonably well.
>>
>> Does it make the governors getting ride of it ? Or at least not changing
>> its value ?
> 
> Not yet, but I'm going to update it this way.  The governors should
> not mess up with tz->passive IMV.

+1

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Re: [PATCH v1 07/16] thermal: gov_power_allocator: Eliminate a redundant variable
Posted by Lukasz Luba 1 year, 9 months ago 

On 4/10/24 17:12, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notice that the passive field in struct thermal_zone_device is not
> used by the Power Allocator governor itself and so the ordering of
> its updates with respect to allow_maximum_power() or allocate_power()
> does not matter.
> 
> Accordingly, make power_allocator_manage() update that field right
> before returning, which allows the current value of it to be passed
> directly to allow_maximum_power() without using the additional update
> variable that can be dropped.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_power_allocator.c |    9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -747,21 +747,18 @@ static void power_allocator_manage(struc
>   {
>   	struct power_allocator_params *params = tz->governor_data;
>   	const struct thermal_trip *trip = params->trip_switch_on;
> -	bool update;
>   
>   	lockdep_assert_held(&tz->lock);
>   
>   	if (trip && tz->temperature < trip->temperature) {
> -		update = tz->passive;
> -		tz->passive = 0;
>   		reset_pid_controller(params);
> -		allow_maximum_power(tz, update);
> +		allow_maximum_power(tz, tz->passive);
> +		tz->passive = 0;
>   		return;
>   	}
>   
> -	tz->passive = 1;
> -
>   	allocate_power(tz, params->trip_max->temperature);
> +	tz->passive = 1;
>   }
>   
>   static struct thermal_governor thermal_gov_power_allocator = {
> 
> 
> 


LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.