diff mbox series

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

Message ID 20210419084536.25000-3-lukasz.luba@arm.com (mailing list archive)
State New
Delegated to: Daniel Lezcano
Headers show
Series Improve IPA mechanisms in low temperature state | expand

Commit Message

Lukasz Luba April 19, 2021, 8:45 a.m. UTC
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(-)

Comments

Daniel Lezcano April 20, 2021, 1:30 p.m. UTC | #1
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;
>  	}
>  
>
Lukasz Luba April 20, 2021, 2:21 p.m. UTC | #2
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
Daniel Lezcano April 20, 2021, 3:24 p.m. UTC | #3
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() ?
Lukasz Luba April 20, 2021, 8:01 p.m. UTC | #4
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?
Daniel Lezcano April 20, 2021, 9:03 p.m. UTC | #5
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
Lukasz Luba April 21, 2021, 8:46 a.m. UTC | #6
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
diff mbox series

Patch

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;
 	}