diff mbox series

[v2,8/8] thermal: gov_power_allocator: Support new update callback of weights

Message ID 20231212134844.1213381-9-lukasz.luba@arm.com
State New
Headers show
Series Add callback for cooling list update to speed-up IPA | expand

Commit Message

Lukasz Luba Dec. 12, 2023, 1:48 p.m. UTC
When the thermal instance's weight is updated from the sysfs the governor
update_tz() callback is triggered. Implement proper reaction to this
event in the IPA, which would save CPU cycles spent in throttle().
This will speed-up the main throttle() IPA function and clean it up
a bit.

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

Comments

Rafael J. Wysocki Dec. 20, 2023, 2:59 p.m. UTC | #1
On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> When the thermal instance's weight is updated from the sysfs the governor
> update_tz() callback is triggered. Implement proper reaction to this
> event in the IPA, which would save CPU cycles spent in throttle().
> This will speed-up the main throttle() IPA function and clean it up
> a bit.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/gov_power_allocator.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 574aa5822112..a9f1549e6355 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -61,6 +61,7 @@ static inline s64 div_frac(s64 x, s64 y)
>   *                     @trip_switch_on should be NULL.
>   * @trip_max:          last passive trip point of the thermal zone. The
>   *                     temperature we are controlling for.
> + * @total_weight:      Sum of all thermal instances weights
>   * @num_actors:                number of cooling devices supporting IPA callbacks
>   * @buffer_size:       IPA internal buffer size
>   * @req_power:         IPA buffer for requested power
> @@ -76,6 +77,7 @@ struct power_allocator_params {
>         u32 sustainable_power;
>         const struct thermal_trip *trip_switch_on;
>         const struct thermal_trip *trip_max;
> +       int total_weight;
>         int num_actors;
>         int buffer_size;
>         u32 *req_power;
> @@ -403,16 +405,11 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
>         u32 total_req_power = 0;
>         u32 *weighted_req_power;
>         u32 power_range, weight;
> -       int total_weight = 0;
>         int i = 0;
>
>         if (!params->num_actors)
>                 return -ENODEV;
>
> -       list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> -               if (power_actor_is_valid(params, instance))
> -                       total_weight += instance->weight;
> -
>         /* Clean all buffers for new power estimations */
>         memset(params->req_power, 0, params->buffer_size);
>
> @@ -430,7 +427,7 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
>                 if (cdev->ops->get_requested_power(cdev, &req_power[i]))
>                         continue;
>
> -               if (!total_weight)
> +               if (!params->total_weight)
>                         weight = 1 << FRAC_BITS;
>                 else
>                         weight = instance->weight;
> @@ -666,6 +663,12 @@ static void power_allocator_update_tz(struct thermal_zone_device *tz,
>
>                 allocate_actors_buffer(params, num_actors);
>                 break;
> +       case THERMAL_INSTANCE_WEIGHT_UPDATE:
> +               params->total_weight = 0;
> +               list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> +                       if (power_actor_is_valid(params, instance))
> +                               params->total_weight += instance->weight;
> +               break;
>         default:
>                 break;
>         }
> --

This one looks good to me, but if you decide to follow my advice from
the previous comments, it will need to be adjusted.
Lukasz Luba Dec. 20, 2023, 4:14 p.m. UTC | #2
Hi Rafael,

On 12/20/23 14:59, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> When the thermal instance's weight is updated from the sysfs the governor
>> update_tz() callback is triggered. Implement proper reaction to this
>> event in the IPA, which would save CPU cycles spent in throttle().
>> This will speed-up the main throttle() IPA function and clean it up
>> a bit.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/thermal/gov_power_allocator.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
>> index 574aa5822112..a9f1549e6355 100644
>> --- a/drivers/thermal/gov_power_allocator.c
>> +++ b/drivers/thermal/gov_power_allocator.c
>> @@ -61,6 +61,7 @@ static inline s64 div_frac(s64 x, s64 y)
>>    *                     @trip_switch_on should be NULL.
>>    * @trip_max:          last passive trip point of the thermal zone. The
>>    *                     temperature we are controlling for.
>> + * @total_weight:      Sum of all thermal instances weights
>>    * @num_actors:                number of cooling devices supporting IPA callbacks
>>    * @buffer_size:       IPA internal buffer size
>>    * @req_power:         IPA buffer for requested power
>> @@ -76,6 +77,7 @@ struct power_allocator_params {
>>          u32 sustainable_power;
>>          const struct thermal_trip *trip_switch_on;
>>          const struct thermal_trip *trip_max;
>> +       int total_weight;
>>          int num_actors;
>>          int buffer_size;
>>          u32 *req_power;
>> @@ -403,16 +405,11 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
>>          u32 total_req_power = 0;
>>          u32 *weighted_req_power;
>>          u32 power_range, weight;
>> -       int total_weight = 0;
>>          int i = 0;
>>
>>          if (!params->num_actors)
>>                  return -ENODEV;
>>
>> -       list_for_each_entry(instance, &tz->thermal_instances, tz_node)
>> -               if (power_actor_is_valid(params, instance))
>> -                       total_weight += instance->weight;
>> -
>>          /* Clean all buffers for new power estimations */
>>          memset(params->req_power, 0, params->buffer_size);
>>
>> @@ -430,7 +427,7 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
>>                  if (cdev->ops->get_requested_power(cdev, &req_power[i]))
>>                          continue;
>>
>> -               if (!total_weight)
>> +               if (!params->total_weight)
>>                          weight = 1 << FRAC_BITS;
>>                  else
>>                          weight = instance->weight;
>> @@ -666,6 +663,12 @@ static void power_allocator_update_tz(struct thermal_zone_device *tz,
>>
>>                  allocate_actors_buffer(params, num_actors);
>>                  break;
>> +       case THERMAL_INSTANCE_WEIGHT_UPDATE:
>> +               params->total_weight = 0;
>> +               list_for_each_entry(instance, &tz->thermal_instances, tz_node)
>> +                       if (power_actor_is_valid(params, instance))
>> +                               params->total_weight += instance->weight;
>> +               break;
>>          default:
>>                  break;
>>          }
>> --
> 
> This one looks good to me, but if you decide to follow my advice from
> the previous comments, it will need to be adjusted.

Yes, I will follow your recommendations, so this will be adjusted.

Thank you for the review. I will respond to your other comments in
patches as well.

Regards,
Lukasz
diff mbox series

Patch

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 574aa5822112..a9f1549e6355 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -61,6 +61,7 @@  static inline s64 div_frac(s64 x, s64 y)
  *			@trip_switch_on should be NULL.
  * @trip_max:		last passive trip point of the thermal zone. The
  *			temperature we are controlling for.
+ * @total_weight:	Sum of all thermal instances weights
  * @num_actors:		number of cooling devices supporting IPA callbacks
  * @buffer_size:	IPA internal buffer size
  * @req_power:		IPA buffer for requested power
@@ -76,6 +77,7 @@  struct power_allocator_params {
 	u32 sustainable_power;
 	const struct thermal_trip *trip_switch_on;
 	const struct thermal_trip *trip_max;
+	int total_weight;
 	int num_actors;
 	int buffer_size;
 	u32 *req_power;
@@ -403,16 +405,11 @@  static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 	u32 total_req_power = 0;
 	u32 *weighted_req_power;
 	u32 power_range, weight;
-	int total_weight = 0;
 	int i = 0;
 
 	if (!params->num_actors)
 		return -ENODEV;
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-		if (power_actor_is_valid(params, instance))
-			total_weight += instance->weight;
-
 	/* Clean all buffers for new power estimations */
 	memset(params->req_power, 0, params->buffer_size);
 
@@ -430,7 +427,7 @@  static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 		if (cdev->ops->get_requested_power(cdev, &req_power[i]))
 			continue;
 
-		if (!total_weight)
+		if (!params->total_weight)
 			weight = 1 << FRAC_BITS;
 		else
 			weight = instance->weight;
@@ -666,6 +663,12 @@  static void power_allocator_update_tz(struct thermal_zone_device *tz,
 
 		allocate_actors_buffer(params, num_actors);
 		break;
+	case THERMAL_INSTANCE_WEIGHT_UPDATE:
+		params->total_weight = 0;
+		list_for_each_entry(instance, &tz->thermal_instances, tz_node)
+			if (power_actor_is_valid(params, instance))
+				params->total_weight += instance->weight;
+		break;
 	default:
 		break;
 	}