[PATCH v3 0/3] Improve IPA mechanisms in low temperature state

[PATCH v3 0/3] Improve IPA mechanisms in low temperature state

[Lukasz Luba]

Hi all,

This v3 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/3 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.
The last patch 3/3 is co-developed by Daniel, who presented the code
during v2 review. I have created a helper function based on his idea,
which can now be used inside IPA governor lock protected code. 

changelog:
v3:
- new patch 3/3 co-developed with Daniel
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 (3):
  thermal: power_allocator: maintain the device statistics from going
    stale
  thermal: power_allocator: update once cooling devices when temp is low
  thermal: create a helper __thermal_cdev_update() without a lock

 drivers/thermal/gov_power_allocator.c | 21 ++++++++++++++++----
 drivers/thermal/thermal_core.h        |  1 +
 drivers/thermal/thermal_helpers.c     | 28 +++++++++++++++++----------
 3 files changed, 36 insertions(+), 14 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] 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 v3 2/3] 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

[PATCH v3 3/3] thermal: create a helper __thermal_cdev_update() without a lock

[Lukasz Luba]

There is a need to have a helper function which updates cooling device
state from the governors code. With this change governor can use
lock and unlock while calling helper function. This avoid unnecessary
second time lock/unlock which was in previous solution present in
governor implementation. This new helper function must be called
with mutex 'cdev->lock' hold.

The changed been discussed and part of code presented in thread:
https://lore.kernel.org/linux-pm/20210419084536.25000-1-lukasz.luba@arm.com/

Co-developed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c |  5 +----
 drivers/thermal/thermal_core.h        |  1 +
 drivers/thermal/thermal_helpers.c     | 28 +++++++++++++++++----------
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index f379f1aaa3b5..a6cdb2e892da 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -595,12 +595,9 @@ static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
 		cdev->ops->get_requested_power(cdev, &req_power);
 
 		if (update)
-			instance->cdev->updated = false;
+			__thermal_cdev_update(instance->cdev);
 
 		mutex_unlock(&instance->cdev->lock);
-
-		if (update)
-			thermal_cdev_update(instance->cdev);
 	}
 	mutex_unlock(&tz->lock);
 }
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 86b8cef7310e..726e327b4205 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -66,6 +66,7 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
 }
 
 void thermal_cdev_update(struct thermal_cooling_device *);
+void __thermal_cdev_update(struct thermal_cooling_device *cdev);
 
 /**
  * struct thermal_trip - representation of a point in temperature domain
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index 7f50f412e02a..3d7fd46104de 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -192,18 +192,12 @@ static void thermal_cdev_set_cur_state(struct thermal_cooling_device *cdev,
 	thermal_cooling_device_stats_update(cdev, target);
 }
 
-void thermal_cdev_update(struct thermal_cooling_device *cdev)
+
+void __thermal_cdev_update(struct thermal_cooling_device *cdev)
 {
 	struct thermal_instance *instance;
 	unsigned long target = 0;
 
-	mutex_lock(&cdev->lock);
-	/* cooling device is updated*/
-	if (cdev->updated) {
-		mutex_unlock(&cdev->lock);
-		return;
-	}
-
 	/* Make sure cdev enters the deepest cooling state */
 	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
 		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
@@ -216,11 +210,25 @@ void thermal_cdev_update(struct thermal_cooling_device *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);
 }
+
+/**
+ * thermal_cdev_update - update cooling device state if needed
+ * @cdev:	pointer to struct thermal_cooling_device
+ *
+ * Update the cooling device state if there is a need.
+ */
+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);
+}
 EXPORT_SYMBOL(thermal_cdev_update);
 
 /**
-- 
2.17.1

Re: [PATCH v3 3/3] thermal: create a helper __thermal_cdev_update() without a lock

[Daniel Lezcano]

On 21/04/2021 19:41, Lukasz Luba wrote:
> There is a need to have a helper function which updates cooling device
> state from the governors code. With this change governor can use
> lock and unlock while calling helper function. This avoid unnecessary
> second time lock/unlock which was in previous solution present in
> governor implementation. This new helper function must be called
> with mutex 'cdev->lock' hold.
> 
> The changed been discussed and part of code presented in thread:
> https://lore.kernel.org/linux-pm/20210419084536.25000-1-lukasz.luba@arm.com/
> 
> Co-developed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/gov_power_allocator.c |  5 +----
>  drivers/thermal/thermal_core.h        |  1 +
>  drivers/thermal/thermal_helpers.c     | 28 +++++++++++++++++----------

Why not add this patch first (without the ipa changes) and then replace
patch 2 by using the new function ? That will prevent to go back and forth.


>  3 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index f379f1aaa3b5..a6cdb2e892da 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -595,12 +595,9 @@ static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
>  		cdev->ops->get_requested_power(cdev, &req_power);
>  
>  		if (update)
> -			instance->cdev->updated = false;
> +			__thermal_cdev_update(instance->cdev);
>  
>  		mutex_unlock(&instance->cdev->lock);
> -
> -		if (update)
> -			thermal_cdev_update(instance->cdev);
>  	}
>  	mutex_unlock(&tz->lock);
>  }
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 86b8cef7310e..726e327b4205 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -66,6 +66,7 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
>  }
>  
>  void thermal_cdev_update(struct thermal_cooling_device *);
> +void __thermal_cdev_update(struct thermal_cooling_device *cdev);
>  
>  /**
>   * struct thermal_trip - representation of a point in temperature domain
> diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
> index 7f50f412e02a..3d7fd46104de 100644
> --- a/drivers/thermal/thermal_helpers.c
> +++ b/drivers/thermal/thermal_helpers.c
> @@ -192,18 +192,12 @@ static void thermal_cdev_set_cur_state(struct thermal_cooling_device *cdev,
>  	thermal_cooling_device_stats_update(cdev, target);
>  }
>  
> -void thermal_cdev_update(struct thermal_cooling_device *cdev)
> +
> +void __thermal_cdev_update(struct thermal_cooling_device *cdev)
>  {
>  	struct thermal_instance *instance;
>  	unsigned long target = 0;
>  
> -	mutex_lock(&cdev->lock);
> -	/* cooling device is updated*/
> -	if (cdev->updated) {
> -		mutex_unlock(&cdev->lock);
> -		return;
> -	}
> -
>  	/* Make sure cdev enters the deepest cooling state */
>  	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
>  		dev_dbg(&cdev->device, "zone%d->target=%lu\n",
> @@ -216,11 +210,25 @@ void thermal_cdev_update(struct thermal_cooling_device *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);
>  }
> +
> +/**
> + * thermal_cdev_update - update cooling device state if needed
> + * @cdev:	pointer to struct thermal_cooling_device
> + *
> + * Update the cooling device state if there is a need.
> + */
> +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);
> +}
>  EXPORT_SYMBOL(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 v3 3/3] thermal: create a helper __thermal_cdev_update() without a lock

[Lukasz Luba]

On 4/22/21 8:58 AM, Daniel Lezcano wrote:
> On 21/04/2021 19:41, Lukasz Luba wrote:
>> There is a need to have a helper function which updates cooling device
>> state from the governors code. With this change governor can use
>> lock and unlock while calling helper function. This avoid unnecessary
>> second time lock/unlock which was in previous solution present in
>> governor implementation. This new helper function must be called
>> with mutex 'cdev->lock' hold.
>>
>> The changed been discussed and part of code presented in thread:
>> https://lore.kernel.org/linux-pm/20210419084536.25000-1-lukasz.luba@arm.com/
>>
>> Co-developed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/thermal/gov_power_allocator.c |  5 +----
>>   drivers/thermal/thermal_core.h        |  1 +
>>   drivers/thermal/thermal_helpers.c     | 28 +++++++++++++++++----------
> 
> Why not add this patch first (without the ipa changes) and then replace
> patch 2 by using the new function ? That will prevent to go back and forth.

I thought that it would show also the motivation and usage in the
governor. I can had this patch as first in the set, but then I thought
about this example.
I can change it if you like in v4.

Re: [PATCH v3 3/3] thermal: create a helper __thermal_cdev_update() without a lock

[Daniel Lezcano]

On 22/04/2021 10:21, Lukasz Luba wrote:
> 
> 
> On 4/22/21 8:58 AM, Daniel Lezcano wrote:
>> On 21/04/2021 19:41, Lukasz Luba wrote:
>>> There is a need to have a helper function which updates cooling device
>>> state from the governors code. With this change governor can use
>>> lock and unlock while calling helper function. This avoid unnecessary
>>> second time lock/unlock which was in previous solution present in
>>> governor implementation. This new helper function must be called
>>> with mutex 'cdev->lock' hold.
>>>
>>> The changed been discussed and part of code presented in thread:
>>> https://lore.kernel.org/linux-pm/20210419084536.25000-1-lukasz.luba@arm.com/
>>>
>>>
>>> Co-developed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>> ---
>>>   drivers/thermal/gov_power_allocator.c |  5 +----
>>>   drivers/thermal/thermal_core.h        |  1 +
>>>   drivers/thermal/thermal_helpers.c     | 28 +++++++++++++++++----------
>>
>> Why not add this patch first (without the ipa changes) and then replace
>> patch 2 by using the new function ? That will prevent to go back and
>> forth.
> 
> I thought that it would show also the motivation and usage in the
> governor. I can had this patch as first in the set, but then I thought
> about this example.
> I can change it if you like in v4.

Yes, please. I think it is more logical.


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