[PATCH v2 0/2] Improve IPA mechanisms in low temperature state

[PATCH v2 0/2] Improve IPA mechanisms in low temperature state

[Lukasz Luba]

Hi all,

This v2 patch set aims to address the issues present in IPA when the
temperature is below the first trip point and cooling devices are not
throttled.
The first patch adds a basic check of cooling devices power to keep the
internal statistics fresh. This allows to avoid issue when the statistics
cover very long period, because they were not maintained.
The second patch addresses an issue described in bugzilla [1], which is:
unneccessary updating cooling devices when their state has not changed
because they are not throttled. This update triggers sending an event,
which should be avoided. Thus, patch 2/2 adds a tracking mechanism if
the update was triggered and makes sure it will be done only once when
the temperature continue to stay below first trip point.

changelog:
v2:
- patch 2/2 uses now simple 'update' bool flag and information from
  'tz->last_temperature'
- patch 1/2 has small change in the comment
- re-based on top of today's thermal/next branch

Regards,
Lukasz Luba

[1] https://bugzilla.kernel.org/show_bug.cgi?id=212501

Lukasz Luba (2):
  thermal: power_allocator: maintain the device statistics from going
    stale
  thermal: power_allocator: update once cooling devices when temp is low

 drivers/thermal/gov_power_allocator.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] thermal: power_allocator: maintain the device statistics from going stale

[Lukasz Luba]

When the temperature is below the first activation trip point the cooling
devices are not checked, so they cannot maintain fresh statistics. It
leads into the situation, when temperature crosses first trip point, the
statistics are stale and show state for very long period. This has impact
on IPA algorithm calculation and wrong decisions. Thus, check the cooling
devices even when the temperature is low, to refresh these statistics.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 2802a0e13c88..d393409fb786 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -575,15 +575,25 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
 {
 	struct thermal_instance *instance;
 	struct power_allocator_params *params = tz->governor_data;
+	u32 req_power;
 
 	mutex_lock(&tz->lock);
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+		struct thermal_cooling_device *cdev = instance->cdev;
+
 		if ((instance->trip != params->trip_max_desired_temperature) ||
 		    (!cdev_is_power_actor(instance->cdev)))
 			continue;
 
 		instance->target = 0;
 		mutex_lock(&instance->cdev->lock);
+		/*
+		 * Call for updating the cooling devices local stats and avoid
+		 * periods of dozen of seconds when those have not been
+		 * maintained.
+		 */
+		cdev->ops->get_requested_power(cdev, &req_power);
+
 		instance->cdev->updated = false;
 		mutex_unlock(&instance->cdev->lock);
 		thermal_cdev_update(instance->cdev);
-- 
2.17.1

[PATCH v2 2/2] thermal: power_allocator: update once cooling devices when temp is low

[Lukasz Luba]

The cooling device state change generates an event, also when there is no
need, because temperature is low and device is not throttled. Avoid to
unnecessary update the cooling device which means also not sending event.
The cooling device state has not changed because the temperature is still
below the first activation trip point value, so we can do this.
Add a tracking mechanism to make sure it updates cooling devices only
once - when the temperature dropps below first trip point.

Reported-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index d393409fb786..f379f1aaa3b5 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -571,7 +571,7 @@ static void reset_pid_controller(struct power_allocator_params *params)
 	params->prev_err = 0;
 }
 
-static void allow_maximum_power(struct thermal_zone_device *tz)
+static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
 {
 	struct thermal_instance *instance;
 	struct power_allocator_params *params = tz->governor_data;
@@ -594,9 +594,13 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
 		 */
 		cdev->ops->get_requested_power(cdev, &req_power);
 
-		instance->cdev->updated = false;
+		if (update)
+			instance->cdev->updated = false;
+
 		mutex_unlock(&instance->cdev->lock);
-		thermal_cdev_update(instance->cdev);
+
+		if (update)
+			thermal_cdev_update(instance->cdev);
 	}
 	mutex_unlock(&tz->lock);
 }
@@ -710,6 +714,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
 	int ret;
 	int switch_on_temp, control_temp;
 	struct power_allocator_params *params = tz->governor_data;
+	bool update;
 
 	/*
 	 * We get called for every trip point but we only need to do
@@ -721,9 +726,10 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
 	ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
 				     &switch_on_temp);
 	if (!ret && (tz->temperature < switch_on_temp)) {
+		update = (tz->last_temperature >= switch_on_temp);
 		tz->passive = 0;
 		reset_pid_controller(params);
-		allow_maximum_power(tz);
+		allow_maximum_power(tz, update);
 		return 0;
 	}
 
-- 
2.17.1

Re: [PATCH v2 2/2] thermal: power_allocator: update once cooling devices when temp is low

[Daniel Lezcano]

On 19/04/2021 10:45, Lukasz Luba wrote:
> The cooling device state change generates an event, also when there is no
> need, because temperature is low and device is not throttled. Avoid to
> unnecessary update the cooling device which means also not sending event.
> The cooling device state has not changed because the temperature is still
> below the first activation trip point value, so we can do this.
> Add a tracking mechanism to make sure it updates cooling devices only
> once - when the temperature dropps below first trip point.
> 
> Reported-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/gov_power_allocator.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index d393409fb786..f379f1aaa3b5 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -571,7 +571,7 @@ static void reset_pid_controller(struct power_allocator_params *params)
>  	params->prev_err = 0;
>  }
>  
> -static void allow_maximum_power(struct thermal_zone_device *tz)
> +static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
>  {
>  	struct thermal_instance *instance;
>  	struct power_allocator_params *params = tz->governor_data;
> @@ -594,9 +594,13 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
>  		 */
>  		cdev->ops->get_requested_power(cdev, &req_power);
>  
> -		instance->cdev->updated = false;
> +		if (update)
> +			instance->cdev->updated = false;
> +
>  		mutex_unlock(&instance->cdev->lock);
> -		(instance->cdev);
> +
> +		if (update)
> +			thermal_cdev_update(instance->cdev);

This cdev update has something bad IMHO. It is protected by a mutex but
the 'updated' field is left unprotected before calling
thermal_cdev_update().

It is not the fault of this code but how the cooling device are updated
and how it interacts with the thermal instances.

IMO, part of the core code needs to revisited.

This change tight a bit more the knot.

Would it make sense to you if we create a function eg.
__thermal_cdev_update()

And then we have:

void thermal_cdev_update(struct thermal_cooling_device *cdev)
{
        mutex_lock(&cdev->lock);
        /* cooling device is updated*/
        if (cdev->updated) {
                mutex_unlock(&cdev->lock);
                return;
        }

	__thermal_cdev_update(cdev);

        thermal_cdev_set_cur_state(cdev, target);

        cdev->updated = true;
        mutex_unlock(&cdev->lock);
        trace_cdev_update(cdev, target);
        dev_dbg(&cdev->device, "set to state %lu\n", target);
}

And in this file we do instead:

-		instance->cdev->updated = false;
+		if (update)
+			__thermal_cdev_update(instance->cdev);
  		mutex_unlock(&instance->cdev->lock);
-		thermal_cdev_update(instance->cdev);

>  	}
>  	mutex_unlock(&tz->lock);
>  }
> @@ -710,6 +714,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
>  	int ret;
>  	int switch_on_temp, control_temp;
>  	struct power_allocator_params *params = tz->governor_data;
> +	bool update;
>  
>  	/*
>  	 * We get called for every trip point but we only need to do
> @@ -721,9 +726,10 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
>  	ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
>  				     &switch_on_temp);
>  	if (!ret && (tz->temperature < switch_on_temp)) {
> +		update = (tz->last_temperature >= switch_on_temp);
>  		tz->passive = 0;
>  		reset_pid_controller(params);
> -		allow_maximum_power(tz);
> +		allow_maximum_power(tz, update);
>  		return 0;
>  	}
>  
> 


-- 
<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 v2 2/2] thermal: power_allocator: update once cooling devices when temp is low

[Lukasz Luba]

Hi Daniel,

On 4/20/21 2:30 PM, Daniel Lezcano wrote:
> On 19/04/2021 10:45, Lukasz Luba wrote:

[snip]

>> -		instance->cdev->updated = false;
>> +		if (update)
>> +			instance->cdev->updated = false;
>> +
>>   		mutex_unlock(&instance->cdev->lock);
>> -		(instance->cdev);
>> +
>> +		if (update)
>> +			thermal_cdev_update(instance->cdev);
> 
> This cdev update has something bad IMHO. It is protected by a mutex but
> the 'updated' field is left unprotected before calling
> thermal_cdev_update().
> 
> It is not the fault of this code but how the cooling device are updated
> and how it interacts with the thermal instances.
> 
> IMO, part of the core code needs to revisited.

I agree, but please check my comments below.

> 
> This change tight a bit more the knot.
> 
> Would it make sense to you if we create a function eg.
> __thermal_cdev_update()

I'm not sure if I assume it right that the function would only have the:
list_for_each_entry(instance, &cdev->thermal_instances, cdev_node)

loop from the thermal_cdev_update(). But if it has only this loop then
it's too little.

> 
> And then we have:
> 
> void thermal_cdev_update(struct thermal_cooling_device *cdev)
> {
>          mutex_lock(&cdev->lock);
>          /* cooling device is updated*/
>          if (cdev->updated) {
>                  mutex_unlock(&cdev->lock);
>                  return;
>          }
> 
> 	__thermal_cdev_update(cdev);
> 
>          thermal_cdev_set_cur_state(cdev, target);

Here we are actually setting the 'target' state via:
cdev->ops->set_cur_state(cdev, target)

then we notify, then updating stats.

> 
>          cdev->updated = true;
>          mutex_unlock(&cdev->lock);
>          trace_cdev_update(cdev, target);

Also this trace is something that I'm using in my tests...

>          dev_dbg(&cdev->device, "set to state %lu\n", target);
> }
> 
> And in this file we do instead:
> 
> -		instance->cdev->updated = false;
> +		if (update)
> +			__thermal_cdev_update(instance->cdev);
>    		mutex_unlock(&instance->cdev->lock);
> -		thermal_cdev_update(instance->cdev);

Without the line above, we are not un-throttling the devices.

Regards,
Lukasz

Re: [PATCH v2 2/2] thermal: power_allocator: update once cooling devices when temp is low

[Daniel Lezcano]

On 20/04/2021 16:21, Lukasz Luba wrote:
> Hi Daniel,
> 
> On 4/20/21 2:30 PM, Daniel Lezcano wrote:
>> On 19/04/2021 10:45, Lukasz Luba wrote:
> 
> [snip]
> 
>>> -        instance->cdev->updated = false;
>>> +        if (update)
>>> +            instance->cdev->updated = false;
>>> +
>>>           mutex_unlock(&instance->cdev->lock);
>>> -        (instance->cdev);
>>> +
>>> +        if (update)
>>> +            thermal_cdev_update(instance->cdev);
>>
>> This cdev update has something bad IMHO. It is protected by a mutex but
>> the 'updated' field is left unprotected before calling
>> thermal_cdev_update().
>>
>> It is not the fault of this code but how the cooling device are updated
>> and how it interacts with the thermal instances.
>>
>> IMO, part of the core code needs to revisited.
> 
> I agree, but please check my comments below.
> 
>>
>> This change tight a bit more the knot.
>>
>> Would it make sense to you if we create a function eg.
>> __thermal_cdev_update()
> 
> I'm not sure if I assume it right that the function would only have the:
> list_for_each_entry(instance, &cdev->thermal_instances, cdev_node)
> 
> loop from the thermal_cdev_update(). But if it has only this loop then
> it's too little.
> 
>>
>> And then we have:
>>
>> void thermal_cdev_update(struct thermal_cooling_device *cdev)
>> {
>>          mutex_lock(&cdev->lock);
>>          /* cooling device is updated*/
>>          if (cdev->updated) {
>>                  mutex_unlock(&cdev->lock);
>>                  return;
>>          }
>>
>>     __thermal_cdev_update(cdev);
>>
>>          thermal_cdev_set_cur_state(cdev, target);
> 
> Here we are actually setting the 'target' state via:
> cdev->ops->set_cur_state(cdev, target)
> 
> then we notify, then updating stats.
> 
>>
>>          cdev->updated = true;
>>          mutex_unlock(&cdev->lock);
>>          trace_cdev_update(cdev, target);
> 
> Also this trace is something that I'm using in my tests...

Yeah, I noticed right after sending the comments. All that should be
moved in the lockless function.

So this function becomes:

void thermal_cdev_update(struct thermal_cooling_device *cdev)
{
	mutex_lock(&cdev->lock);
	if (!cdev->updated) {
		__thermal_cdev_update(cdev);
		cdev->updated = true;
	}
	mutex_unlock(&cdev->lock);

	dev_dbg(&cdev->device, "set to state %lu\n", target);
}

We end up with the trace_cdev_update(cdev, target) inside the mutex
section but that should be fine.

>>          dev_dbg(&cdev->device, "set to state %lu\n", target);
>> }
>>
>> And in this file we do instead:
>>
>> -        instance->cdev->updated = false;
>> +        if (update)
>> +            __thermal_cdev_update(instance->cdev);
>>            mutex_unlock(&instance->cdev->lock);
>> -        thermal_cdev_update(instance->cdev);
> 
> Without the line above, we are not un-throttling the devices.

Is it still true with the amended function thermal_cdev_update() ?


-- 
<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 v2 2/2] thermal: power_allocator: update once cooling devices when temp is low

[Lukasz Luba]

On 4/20/21 4:24 PM, Daniel Lezcano wrote:
> On 20/04/2021 16:21, Lukasz Luba wrote:
>> Hi Daniel,
>>
>> On 4/20/21 2:30 PM, Daniel Lezcano wrote:
>>> On 19/04/2021 10:45, Lukasz Luba wrote:
>>
>> [snip]
>>
>>>> -        instance->cdev->updated = false;
>>>> +        if (update)
>>>> +            instance->cdev->updated = false;
>>>> +
>>>>            mutex_unlock(&instance->cdev->lock);
>>>> -        (instance->cdev);
>>>> +
>>>> +        if (update)
>>>> +            thermal_cdev_update(instance->cdev);
>>>
>>> This cdev update has something bad IMHO. It is protected by a mutex but
>>> the 'updated' field is left unprotected before calling
>>> thermal_cdev_update().
>>>
>>> It is not the fault of this code but how the cooling device are updated
>>> and how it interacts with the thermal instances.
>>>
>>> IMO, part of the core code needs to revisited.
>>
>> I agree, but please check my comments below.
>>
>>>
>>> This change tight a bit more the knot.
>>>
>>> Would it make sense to you if we create a function eg.
>>> __thermal_cdev_update()
>>
>> I'm not sure if I assume it right that the function would only have the:
>> list_for_each_entry(instance, &cdev->thermal_instances, cdev_node)
>>
>> loop from the thermal_cdev_update(). But if it has only this loop then
>> it's too little.
>>
>>>
>>> And then we have:
>>>
>>> void thermal_cdev_update(struct thermal_cooling_device *cdev)
>>> {
>>>           mutex_lock(&cdev->lock);
>>>           /* cooling device is updated*/
>>>           if (cdev->updated) {
>>>                   mutex_unlock(&cdev->lock);
>>>                   return;
>>>           }
>>>
>>>      __thermal_cdev_update(cdev);
>>>
>>>           thermal_cdev_set_cur_state(cdev, target);
>>
>> Here we are actually setting the 'target' state via:
>> cdev->ops->set_cur_state(cdev, target)
>>
>> then we notify, then updating stats.
>>
>>>
>>>           cdev->updated = true;
>>>           mutex_unlock(&cdev->lock);
>>>           trace_cdev_update(cdev, target);
>>
>> Also this trace is something that I'm using in my tests...
> 
> Yeah, I noticed right after sending the comments. All that should be
> moved in the lockless function.

Agree

> 
> So this function becomes:
> 
> void thermal_cdev_update(struct thermal_cooling_device *cdev)
> {
> 	mutex_lock(&cdev->lock);
> 	if (!cdev->updated) {
> 		__thermal_cdev_update(cdev);
> 		cdev->updated = true;
> 	}
> 	mutex_unlock(&cdev->lock);
> 
> 	dev_dbg(&cdev->device, "set to state %lu\n", target);
> }
> 
> We end up with the trace_cdev_update(cdev, target) inside the mutex
> section but that should be fine.

True, this shouldn't be an issue.

> 
>>>           dev_dbg(&cdev->device, "set to state %lu\n", target);
>>> }
>>>
>>> And in this file we do instead:
>>>
>>> -        instance->cdev->updated = false;
>>> +        if (update)
>>> +            __thermal_cdev_update(instance->cdev);
>>>             mutex_unlock(&instance->cdev->lock);
>>> -        thermal_cdev_update(instance->cdev);
>>
>> Without the line above, we are not un-throttling the devices.
> 
> Is it still true with the amended function thermal_cdev_update() ?
> 
> 

That new approach should work. I can test your patch with this new
functions and re-base my work on top of it.
Or you like me to write such patch and send it?

Re: [PATCH v2 2/2] thermal: power_allocator: update once cooling devices when temp is low

[Daniel Lezcano]

On 20/04/2021 22:01, Lukasz Luba wrote:
> 
> 
> On 4/20/21 4:24 PM, Daniel Lezcano wrote:
>> On 20/04/2021 16:21, Lukasz Luba wrote:
>>> Hi Daniel,
>>>
>>> On 4/20/21 2:30 PM, Daniel Lezcano wrote:
>>>> On 19/04/2021 10:45, Lukasz Luba wrote:
>>>
>>> [snip]

[ ... ]

> 
> That new approach should work. I can test your patch with this new
> functions and re-base my work on top of it.
> Or you like me to write such patch and send it?

At your convenience. I'm pretty busy ATM with more patches to review, so
if you can handle that change that will be nice. Otherwise, I can take
care of it but later.

Thanks

  -- Daniel

-- 
<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 v2 2/2] thermal: power_allocator: update once cooling devices when temp is low

[Lukasz Luba]

On 4/20/21 10:03 PM, Daniel Lezcano wrote:
> On 20/04/2021 22:01, Lukasz Luba wrote:
>>
>>
>> On 4/20/21 4:24 PM, Daniel Lezcano wrote:
>>> On 20/04/2021 16:21, Lukasz Luba wrote:
>>>> Hi Daniel,
>>>>
>>>> On 4/20/21 2:30 PM, Daniel Lezcano wrote:
>>>>> On 19/04/2021 10:45, Lukasz Luba wrote:
>>>>
>>>> [snip]
> 
> [ ... ]
> 
>>
>> That new approach should work. I can test your patch with this new
>> functions and re-base my work on top of it.
>> Or you like me to write such patch and send it?
> 
> At your convenience. I'm pretty busy ATM with more patches to review, so
> if you can handle that change that will be nice. Otherwise, I can take
> care of it but later.
> 

OK, so I will create such patch and add your name in tags:
Co-developed-by: and Signed-off-by:
plus also a lore.kernel.org link in the patch commit message into this
discussion. Thanks for having a look at this.

Regards,
Lukasz