[PATCH v2 0/8] Add callback for cooling list update to speed-up IPA

[PATCH v2 0/8] Add callback for cooling list update to speed-up IPA

[Lukasz Luba]

Hi all,

The patch set a new callback for thermal governors and implementation for
Intelligent Power Allocator.

The goal is to move some heavy operarions like the memory allocations and heavy
computations (multiplications) out of throttle() callback hot path.

The new callback is generic enough to handle other imporants update events.
It re-uses existing thermal_notify_event definitions.

In addition there are some small clean-ups for IPA code.

changes:
v2:
- change callback name to update_tz() and add parameter (Rafael)
- added new event to trigger this callback - instance 'weight' update

Regards,
Lukasz

Lukasz Luba (8):
  thermal: core: Add governor callback for thermal zone change
  thermal: gov_power_allocator: Refactor check_power_actors()
  thermal: gov_power_allocator: Move memory allocation out of throttle()
  thermal: gov_power_allocator: Simplify checks for valid power actor
  thermal: gov_power_allocator: Refactor checks in divvy_up_power()
  thermal/sysfs: Update instance->weight under tz lock
  thermal/sysfs: Update governors when the 'weight' has changed
  thermal: gov_power_allocator: Support new update callback of weights

 drivers/thermal/gov_power_allocator.c | 216 ++++++++++++++++++--------
 drivers/thermal/thermal_core.c        |  13 ++
 drivers/thermal/thermal_sysfs.c       |  15 ++
 include/linux/thermal.h               |   6 +
 4 files changed, 182 insertions(+), 68 deletions(-)

-- 
2.25.1

[PATCH v2 1/8] thermal: core: Add governor callback for thermal zone change

[Lukasz Luba]

Add a new callback which can update governors when there is a change in
the thermal zone internals, e.g. thermal cooling instance list changed.

That makes possible to move some heavy operations like memory allocations
related to the number of cooling instances out of the throttle() callback.

Reuse the 'enum thermal_notify_event' and extend it with a new event:
THERMAL_INSTANCE_LIST_UPDATE.

Both callback code paths (throttle() and update_tz()) are protected with
the same thermal zone lock, which guaranties the consistency.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/thermal_core.c | 13 +++++++++++++
 include/linux/thermal.h        |  5 +++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 625ba07cbe2f..592c956f6fd5 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -314,6 +314,14 @@ static void handle_non_critical_trips(struct thermal_zone_device *tz,
 		       def_governor->throttle(tz, trip);
 }
 
+static void handle_instances_list_update(struct thermal_zone_device *tz)
+{
+	if (!tz->governor || !tz->governor->update_tz)
+		return;
+
+	tz->governor->update_tz(tz, THERMAL_INSTANCE_LIST_UPDATE);
+}
+
 void thermal_zone_device_critical(struct thermal_zone_device *tz)
 {
 	/*
@@ -723,6 +731,8 @@ int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
 		list_add_tail(&dev->tz_node, &tz->thermal_instances);
 		list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
 		atomic_set(&tz->need_update, 1);
+
+		handle_instances_list_update(tz);
 	}
 	mutex_unlock(&cdev->lock);
 	mutex_unlock(&tz->lock);
@@ -781,6 +791,9 @@ int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
 		if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
 			list_del(&pos->tz_node);
 			list_del(&pos->cdev_node);
+
+			handle_instances_list_update(tz);
+
 			mutex_unlock(&cdev->lock);
 			mutex_unlock(&tz->lock);
 			goto unbind;
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index c7190e2dfcb4..9fd0d3fb234a 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -51,6 +51,7 @@ enum thermal_notify_event {
 	THERMAL_DEVICE_POWER_CAPABILITY_CHANGED, /* power capability changed */
 	THERMAL_TABLE_CHANGED, /* Thermal table(s) changed */
 	THERMAL_EVENT_KEEP_ALIVE, /* Request for user space handler to respond */
+	THERMAL_INSTANCE_LIST_UPDATE, /* List of thermal instances changed */
 };
 
 /**
@@ -195,6 +196,8 @@ struct thermal_zone_device {
  *			thermal zone.
  * @throttle:	callback called for every trip point even if temperature is
  *		below the trip point temperature
+ * @update_tz:	callback called when thermal zone internals have changed, e.g.
+ *		thermal cooling instance was added/removed
  * @governor_list:	node in thermal_governor_list (in thermal_core.c)
  */
 struct thermal_governor {
@@ -203,6 +206,8 @@ struct thermal_governor {
 	void (*unbind_from_tz)(struct thermal_zone_device *tz);
 	int (*throttle)(struct thermal_zone_device *tz,
 			const struct thermal_trip *trip);
+	void (*update_tz)(struct thermal_zone_device *tz,
+			  enum thermal_notify_event reason);
 	struct list_head	governor_list;
 };
 
-- 
2.25.1

Re: [PATCH v2 1/8] thermal: core: Add governor callback for thermal zone change

[Rafael J. Wysocki]

On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Add a new callback which can update governors when there is a change in
> the thermal zone internals, e.g. thermal cooling instance list changed.

I would say what struct type the callback is going to be added to.

> That makes possible to move some heavy operations like memory allocations
> related to the number of cooling instances out of the throttle() callback.
>
> Reuse the 'enum thermal_notify_event' and extend it with a new event:
> THERMAL_INSTANCE_LIST_UPDATE.

I think that this is a bit too low-level (see below).

> Both callback code paths (throttle() and update_tz()) are protected with
> the same thermal zone lock, which guaranties the consistency.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/thermal_core.c | 13 +++++++++++++
>  include/linux/thermal.h        |  5 +++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 625ba07cbe2f..592c956f6fd5 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -314,6 +314,14 @@ static void handle_non_critical_trips(struct thermal_zone_device *tz,
>                        def_governor->throttle(tz, trip);
>  }
>

I needed a bit more time to think about this.

> +static void handle_instances_list_update(struct thermal_zone_device *tz)
> +{
> +       if (!tz->governor || !tz->governor->update_tz)
> +               return;
> +
> +       tz->governor->update_tz(tz, THERMAL_INSTANCE_LIST_UPDATE);
> +}

So I would call the above something more generic, like
thermal_governor_update_tz() and I would pass the "reason" argument to
it.

> +
>  void thermal_zone_device_critical(struct thermal_zone_device *tz)
>  {
>         /*
> @@ -723,6 +731,8 @@ int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
>                 list_add_tail(&dev->tz_node, &tz->thermal_instances);
>                 list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
>                 atomic_set(&tz->need_update, 1);
> +
> +               handle_instances_list_update(tz);

In particular for this, I would use a special "reason" value, like
THERMAL_TZ_BIND_CDEV.

Yes, the list of instances will change as a result of the binding, but
that is an internal detail specific to the current implementation.

>         }
>         mutex_unlock(&cdev->lock);
>         mutex_unlock(&tz->lock);
> @@ -781,6 +791,9 @@ int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
>                 if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
>                         list_del(&pos->tz_node);
>                         list_del(&pos->cdev_node);
> +
> +                       handle_instances_list_update(tz);

Analogously, I'd use something like THERMAL_TZ_UNBIND_CDEV here.

> +
>                         mutex_unlock(&cdev->lock);
>                         mutex_unlock(&tz->lock);
>                         goto unbind;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index c7190e2dfcb4..9fd0d3fb234a 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -51,6 +51,7 @@ enum thermal_notify_event {
>         THERMAL_DEVICE_POWER_CAPABILITY_CHANGED, /* power capability changed */
>         THERMAL_TABLE_CHANGED, /* Thermal table(s) changed */
>         THERMAL_EVENT_KEEP_ALIVE, /* Request for user space handler to respond */
> +       THERMAL_INSTANCE_LIST_UPDATE, /* List of thermal instances changed */

So I would add THERMAL_TZ_BIND_CDEV and THERMAL_TZ_UNBIND_CDEV to the list.

>  };
>
>  /**
> @@ -195,6 +196,8 @@ struct thermal_zone_device {
>   *                     thermal zone.
>   * @throttle:  callback called for every trip point even if temperature is
>   *             below the trip point temperature
> + * @update_tz: callback called when thermal zone internals have changed, e.g.
> + *             thermal cooling instance was added/removed
>   * @governor_list:     node in thermal_governor_list (in thermal_core.c)
>   */
>  struct thermal_governor {
> @@ -203,6 +206,8 @@ struct thermal_governor {
>         void (*unbind_from_tz)(struct thermal_zone_device *tz);
>         int (*throttle)(struct thermal_zone_device *tz,
>                         const struct thermal_trip *trip);
> +       void (*update_tz)(struct thermal_zone_device *tz,
> +                         enum thermal_notify_event reason);
>         struct list_head        governor_list;
>  };
>
> --

Re: [PATCH v2 1/8] thermal: core: Add governor callback for thermal zone change

[Lukasz Luba]

On 12/20/23 13:51, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Add a new callback which can update governors when there is a change in
>> the thermal zone internals, e.g. thermal cooling instance list changed.
> 
> I would say what struct type the callback is going to be added to.

OK, I'll add that.

> 
>> That makes possible to move some heavy operations like memory allocations
>> related to the number of cooling instances out of the throttle() callback.
>>
>> Reuse the 'enum thermal_notify_event' and extend it with a new event:
>> THERMAL_INSTANCE_LIST_UPDATE.
> 
> I think that this is a bit too low-level (see below).

Yes, I agree (based on below).

> 
>> Both callback code paths (throttle() and update_tz()) are protected with
>> the same thermal zone lock, which guaranties the consistency.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/thermal/thermal_core.c | 13 +++++++++++++
>>   include/linux/thermal.h        |  5 +++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index 625ba07cbe2f..592c956f6fd5 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -314,6 +314,14 @@ static void handle_non_critical_trips(struct thermal_zone_device *tz,
>>                         def_governor->throttle(tz, trip);
>>   }
>>
> 
> I needed a bit more time to think about this.

OK.

> 
>> +static void handle_instances_list_update(struct thermal_zone_device *tz)
>> +{
>> +       if (!tz->governor || !tz->governor->update_tz)
>> +               return;
>> +
>> +       tz->governor->update_tz(tz, THERMAL_INSTANCE_LIST_UPDATE);
>> +}
> 
> So I would call the above something more generic, like
> thermal_governor_update_tz() and I would pass the "reason" argument to
> it.

That sounds better, I agree.

> 
>> +
>>   void thermal_zone_device_critical(struct thermal_zone_device *tz)
>>   {
>>          /*
>> @@ -723,6 +731,8 @@ int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
>>                  list_add_tail(&dev->tz_node, &tz->thermal_instances);
>>                  list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
>>                  atomic_set(&tz->need_update, 1);
>> +
>> +               handle_instances_list_update(tz);
> 
> In particular for this, I would use a special "reason" value, like
> THERMAL_TZ_BIND_CDEV.
> 
> Yes, the list of instances will change as a result of the binding, but
> that is an internal detail specific to the current implementation.

I see. With that new more generic thermal_governor_update_tz() would
be better then, right?

> 
>>          }
>>          mutex_unlock(&cdev->lock);
>>          mutex_unlock(&tz->lock);
>> @@ -781,6 +791,9 @@ int thermal_unbind_cdev_from_trip(struct thermal_zone_device *tz,
>>                  if (pos->tz == tz && pos->trip == trip && pos->cdev == cdev) {
>>                          list_del(&pos->tz_node);
>>                          list_del(&pos->cdev_node);
>> +
>> +                       handle_instances_list_update(tz);
> 
> Analogously, I'd use something like THERMAL_TZ_UNBIND_CDEV here.

OK

> 
>> +
>>                          mutex_unlock(&cdev->lock);
>>                          mutex_unlock(&tz->lock);
>>                          goto unbind;
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index c7190e2dfcb4..9fd0d3fb234a 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -51,6 +51,7 @@ enum thermal_notify_event {
>>          THERMAL_DEVICE_POWER_CAPABILITY_CHANGED, /* power capability changed */
>>          THERMAL_TABLE_CHANGED, /* Thermal table(s) changed */
>>          THERMAL_EVENT_KEEP_ALIVE, /* Request for user space handler to respond */
>> +       THERMAL_INSTANCE_LIST_UPDATE, /* List of thermal instances changed */
> 
> So I would add THERMAL_TZ_BIND_CDEV and THERMAL_TZ_UNBIND_CDEV to the list.

OK

> 
>>   };
>>
>>   /**
>> @@ -195,6 +196,8 @@ struct thermal_zone_device {
>>    *                     thermal zone.
>>    * @throttle:  callback called for every trip point even if temperature is
>>    *             below the trip point temperature
>> + * @update_tz: callback called when thermal zone internals have changed, e.g.
>> + *             thermal cooling instance was added/removed
>>    * @governor_list:     node in thermal_governor_list (in thermal_core.c)
>>    */
>>   struct thermal_governor {
>> @@ -203,6 +206,8 @@ struct thermal_governor {
>>          void (*unbind_from_tz)(struct thermal_zone_device *tz);
>>          int (*throttle)(struct thermal_zone_device *tz,
>>                          const struct thermal_trip *trip);
>> +       void (*update_tz)(struct thermal_zone_device *tz,
>> +                         enum thermal_notify_event reason);
>>          struct list_head        governor_list;
>>   };
>>
>> --

Re: [PATCH v2 1/8] thermal: core: Add governor callback for thermal zone change

[Rafael J. Wysocki]

On Wed, Dec 20, 2023 at 5:16 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 12/20/23 13:51, Rafael J. Wysocki wrote:
> > On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >> Add a new callback which can update governors when there is a change in
> >> the thermal zone internals, e.g. thermal cooling instance list changed.
> >
> > I would say what struct type the callback is going to be added to.
>
> OK, I'll add that.
>
> >
> >> That makes possible to move some heavy operations like memory allocations
> >> related to the number of cooling instances out of the throttle() callback.
> >>
> >> Reuse the 'enum thermal_notify_event' and extend it with a new event:
> >> THERMAL_INSTANCE_LIST_UPDATE.
> >
> > I think that this is a bit too low-level (see below).
>
> Yes, I agree (based on below).
>
> >
> >> Both callback code paths (throttle() and update_tz()) are protected with
> >> the same thermal zone lock, which guaranties the consistency.
> >>
> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> >> ---
> >>   drivers/thermal/thermal_core.c | 13 +++++++++++++
> >>   include/linux/thermal.h        |  5 +++++
> >>   2 files changed, 18 insertions(+)
> >>
> >> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> >> index 625ba07cbe2f..592c956f6fd5 100644
> >> --- a/drivers/thermal/thermal_core.c
> >> +++ b/drivers/thermal/thermal_core.c
> >> @@ -314,6 +314,14 @@ static void handle_non_critical_trips(struct thermal_zone_device *tz,
> >>                         def_governor->throttle(tz, trip);
> >>   }
> >>
> >
> > I needed a bit more time to think about this.
>
> OK.
>
> >
> >> +static void handle_instances_list_update(struct thermal_zone_device *tz)
> >> +{
> >> +       if (!tz->governor || !tz->governor->update_tz)
> >> +               return;
> >> +
> >> +       tz->governor->update_tz(tz, THERMAL_INSTANCE_LIST_UPDATE);
> >> +}
> >
> > So I would call the above something more generic, like
> > thermal_governor_update_tz() and I would pass the "reason" argument to
> > it.
>
> That sounds better, I agree.
>
> >
> >> +
> >>   void thermal_zone_device_critical(struct thermal_zone_device *tz)
> >>   {
> >>          /*
> >> @@ -723,6 +731,8 @@ int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
> >>                  list_add_tail(&dev->tz_node, &tz->thermal_instances);
> >>                  list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
> >>                  atomic_set(&tz->need_update, 1);
> >> +
> >> +               handle_instances_list_update(tz);
> >
> > In particular for this, I would use a special "reason" value, like
> > THERMAL_TZ_BIND_CDEV.
> >
> > Yes, the list of instances will change as a result of the binding, but
> > that is an internal detail specific to the current implementation.
>
> I see. With that new more generic thermal_governor_update_tz() would
> be better then, right?

I think so, IIUC.

Re: [PATCH v2 1/8] thermal: core: Add governor callback for thermal zone change

[Lukasz Luba]

On 12/20/23 17:44, Rafael J. Wysocki wrote:
> On Wed, Dec 20, 2023 at 5:16 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
>> On 12/20/23 13:51, Rafael J. Wysocki wrote:
>>> On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>
>>>> Add a new callback which can update governors when there is a change in
>>>> the thermal zone internals, e.g. thermal cooling instance list changed.
>>>
>>> I would say what struct type the callback is going to be added to.
>>
>> OK, I'll add that.
>>
>>>
>>>> That makes possible to move some heavy operations like memory allocations
>>>> related to the number of cooling instances out of the throttle() callback.
>>>>
>>>> Reuse the 'enum thermal_notify_event' and extend it with a new event:
>>>> THERMAL_INSTANCE_LIST_UPDATE.
>>>
>>> I think that this is a bit too low-level (see below).
>>
>> Yes, I agree (based on below).
>>
>>>
>>>> Both callback code paths (throttle() and update_tz()) are protected with
>>>> the same thermal zone lock, which guaranties the consistency.
>>>>
>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>> ---
>>>>    drivers/thermal/thermal_core.c | 13 +++++++++++++
>>>>    include/linux/thermal.h        |  5 +++++
>>>>    2 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>>> index 625ba07cbe2f..592c956f6fd5 100644
>>>> --- a/drivers/thermal/thermal_core.c
>>>> +++ b/drivers/thermal/thermal_core.c
>>>> @@ -314,6 +314,14 @@ static void handle_non_critical_trips(struct thermal_zone_device *tz,
>>>>                          def_governor->throttle(tz, trip);
>>>>    }
>>>>
>>>
>>> I needed a bit more time to think about this.
>>
>> OK.
>>
>>>
>>>> +static void handle_instances_list_update(struct thermal_zone_device *tz)
>>>> +{
>>>> +       if (!tz->governor || !tz->governor->update_tz)
>>>> +               return;
>>>> +
>>>> +       tz->governor->update_tz(tz, THERMAL_INSTANCE_LIST_UPDATE);
>>>> +}
>>>
>>> So I would call the above something more generic, like
>>> thermal_governor_update_tz() and I would pass the "reason" argument to
>>> it.
>>
>> That sounds better, I agree.
>>
>>>
>>>> +
>>>>    void thermal_zone_device_critical(struct thermal_zone_device *tz)
>>>>    {
>>>>           /*
>>>> @@ -723,6 +731,8 @@ int thermal_bind_cdev_to_trip(struct thermal_zone_device *tz,
>>>>                   list_add_tail(&dev->tz_node, &tz->thermal_instances);
>>>>                   list_add_tail(&dev->cdev_node, &cdev->thermal_instances);
>>>>                   atomic_set(&tz->need_update, 1);
>>>> +
>>>> +               handle_instances_list_update(tz);
>>>
>>> In particular for this, I would use a special "reason" value, like
>>> THERMAL_TZ_BIND_CDEV.
>>>
>>> Yes, the list of instances will change as a result of the binding, but
>>> that is an internal detail specific to the current implementation.
>>
>> I see. With that new more generic thermal_governor_update_tz() would
>> be better then, right?
> 
> I think so, IIUC.

OK, thanks!

[PATCH v2 2/8] thermal: gov_power_allocator: Refactor check_power_actors()

[Lukasz Luba]

Refactor check_power_actors() to make it possible for re-use in the
upcoming new callback.

No intentional functional impact.

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

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 785fff14223d..38e1e89ba10c 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -581,8 +581,9 @@ static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
  * power actor API. The warning should help to investigate the issue, which
  * could be e.g. lack of Energy Model for a given device.
  *
- * Return: 0 on success, -EINVAL if any cooling device does not implement
- * the power actor API.
+ * Return number of cooling devices or -EINVAL if any cooling device does not
+ * implement the power actor API. Return value 0 is also valid since cooling
+ * devices might be attached later.
  */
 static int check_power_actors(struct thermal_zone_device *tz,
 			      struct power_allocator_params *params)
@@ -597,8 +598,9 @@ static int check_power_actors(struct thermal_zone_device *tz,
 		if (!cdev_is_power_actor(instance->cdev)) {
 			dev_warn(&tz->device, "power_allocator: %s is not a power actor\n",
 				 instance->cdev->type);
-			ret = -EINVAL;
+			return -EINVAL;
 		}
+		ret++;
 	}
 
 	return ret;
@@ -631,7 +633,7 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 	}
 
 	ret = check_power_actors(tz, params);
-	if (ret) {
+	if (ret < 0) {
 		dev_warn(&tz->device, "power_allocator: binding failed\n");
 		kfree(params);
 		return ret;
-- 
2.25.1

[PATCH v2 3/8] thermal: gov_power_allocator: Move memory allocation out of throttle()

[Lukasz Luba]

The new thermal callback allows to react to the change of cooling
instances in the thermal zone. Move the memory allocation to that new
callback and save CPU cycles in the throttle() code path.

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

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 38e1e89ba10c..3328c3ec71f2 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -61,6 +61,13 @@ 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.
+ * @num_actors:		number of cooling devices supporting IPA callbacks
+ * @buffer_size:	IPA internal buffer size
+ * @req_power:		IPA buffer for requested power
+ * @max_power:		IPA buffer for max allocatable power
+ * @granted_power:	IPA buffer for granted power
+ * @extra_actor_power:	IPA buffer for extra power
+ * @weighted_req_power:	IPA buffer for weighted requested power
  */
 struct power_allocator_params {
 	bool allocated_tzp;
@@ -69,6 +76,13 @@ struct power_allocator_params {
 	u32 sustainable_power;
 	const struct thermal_trip *trip_switch_on;
 	const struct thermal_trip *trip_max;
+	int num_actors;
+	int buffer_size;
+	u32 *req_power;
+	u32 *max_power;
+	u32 *granted_power;
+	u32 *extra_actor_power;
+	u32 *weighted_req_power;
 };
 
 /**
@@ -387,39 +401,24 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 	u32 *weighted_req_power;
 	u32 power_range, weight;
 	int total_weight = 0;
-	int num_actors = 0;
 	int i = 0;
 
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+	if (!params->num_actors)
+		return -ENODEV;
+
+	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
 		if ((instance->trip == params->trip_max) &&
-		    cdev_is_power_actor(instance->cdev)) {
-			num_actors++;
+		    cdev_is_power_actor(instance->cdev))
 			total_weight += instance->weight;
-		}
-	}
-
-	if (!num_actors)
-		return -ENODEV;
 
-	/*
-	 * We need to allocate five arrays of the same size:
-	 * req_power, max_power, granted_power, extra_actor_power and
-	 * weighted_req_power.  They are going to be needed until this
-	 * function returns.  Allocate them all in one go to simplify
-	 * the allocation and deallocation logic.
-	 */
-	BUILD_BUG_ON(sizeof(*req_power) != sizeof(*max_power));
-	BUILD_BUG_ON(sizeof(*req_power) != sizeof(*granted_power));
-	BUILD_BUG_ON(sizeof(*req_power) != sizeof(*extra_actor_power));
-	BUILD_BUG_ON(sizeof(*req_power) != sizeof(*weighted_req_power));
-	req_power = kcalloc(num_actors * 5, sizeof(*req_power), GFP_KERNEL);
-	if (!req_power)
-		return -ENOMEM;
+	/* Clean all buffers for new power estimations */
+	memset(params->req_power, 0, params->buffer_size);
 
-	max_power = &req_power[num_actors];
-	granted_power = &req_power[2 * num_actors];
-	extra_actor_power = &req_power[3 * num_actors];
-	weighted_req_power = &req_power[4 * num_actors];
+	req_power = params->req_power;
+	max_power = params->max_power;
+	granted_power = params->granted_power;
+	extra_actor_power = params->extra_actor_power;
+	weighted_req_power = params->weighted_req_power;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		cdev = instance->cdev;
@@ -453,7 +452,7 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 
 	power_range = pid_controller(tz, control_temp, max_allocatable_power);
 
-	divvy_up_power(weighted_req_power, max_power, num_actors,
+	divvy_up_power(weighted_req_power, max_power, params->num_actors,
 		       total_weighted_req_power, power_range, granted_power,
 		       extra_actor_power);
 
@@ -474,12 +473,10 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 
 	trace_thermal_power_allocator(tz, req_power, total_req_power,
 				      granted_power, total_granted_power,
-				      num_actors, power_range,
+				      params->num_actors, power_range,
 				      max_allocatable_power, tz->temperature,
 				      control_temp - tz->temperature);
 
-	kfree(req_power);
-
 	return 0;
 }
 
@@ -606,6 +603,81 @@ static int check_power_actors(struct thermal_zone_device *tz,
 	return ret;
 }
 
+static void _power_buffers_init(struct power_allocator_params *params,
+				u32 *req_power, u32 *max_power,
+				u32 *granted_power, u32 *extra_actor_power,
+				u32 *weighted_req_power)
+
+{
+	/* Setup internal buffers for power calculations. */
+	params->req_power = req_power;
+	params->max_power = max_power;
+	params->granted_power = granted_power;
+	params->extra_actor_power = extra_actor_power;
+	params->weighted_req_power = weighted_req_power;
+}
+
+static int allocate_actors_buffer(struct power_allocator_params *params,
+				  int num_actors)
+{
+	u32 *req_power;
+	int ret;
+
+	kfree(params->req_power);
+
+	/* There might be no cooling devices yet. */
+	if (!num_actors) {
+		ret = -EINVAL;
+		goto clean_buffers;
+	}
+
+	req_power = kcalloc(num_actors * 5, sizeof(u32), GFP_KERNEL);
+	if (!req_power) {
+		ret = -ENOMEM;
+		goto clean_buffers;
+	}
+
+	params->num_actors = num_actors;
+	params->buffer_size = num_actors * 5 * sizeof(u32);
+
+	_power_buffers_init(params, req_power, &req_power[params->num_actors],
+			    &req_power[2 * params->num_actors],
+			    &req_power[3 * params->num_actors],
+			    &req_power[4 * params->num_actors]);
+
+	return 0;
+
+clean_buffers:
+	params->num_actors = -EINVAL;
+	params->buffer_size = 0;
+	_power_buffers_init(params, NULL, NULL, NULL, NULL, NULL);
+	return ret;
+}
+
+static void power_allocator_update_tz(struct thermal_zone_device *tz,
+				      enum thermal_notify_event reason)
+{
+	struct power_allocator_params *params = tz->governor_data;
+	struct thermal_instance *instance;
+	int num_actors = 0;
+
+	switch (reason) {
+	case THERMAL_INSTANCE_LIST_UPDATE:
+		list_for_each_entry(instance, &tz->thermal_instances, tz_node)
+			if ((instance->trip == params->trip_max) &&
+			    cdev_is_power_actor(instance->cdev))
+				num_actors++;
+
+		if (num_actors == params->num_actors)
+			return;
+
+		allocate_actors_buffer(params, num_actors);
+		break;
+	default:
+		break;
+	}
+}
+
 /**
  * power_allocator_bind() - bind the power_allocator governor to a thermal zone
  * @tz:	thermal zone to bind it to
@@ -639,6 +711,13 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 		return ret;
 	}
 
+	ret = allocate_actors_buffer(params, ret);
+	if (ret) {
+		dev_warn(&tz->device, "power_allocator: allocation failed\n");
+		kfree(params);
+		return ret;
+	}
+
 	if (!tz->tzp) {
 		tz->tzp = kzalloc(sizeof(*tz->tzp), GFP_KERNEL);
 		if (!tz->tzp) {
@@ -663,6 +742,7 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 	return 0;
 
 free_params:
+	kfree(params->req_power);
 	kfree(params);
 
 	return ret;
@@ -679,6 +759,7 @@ static void power_allocator_unbind(struct thermal_zone_device *tz)
 		tz->tzp = NULL;
 	}
 
+	kfree(params->req_power);
 	kfree(tz->governor_data);
 	tz->governor_data = NULL;
 }
@@ -717,5 +798,6 @@ static struct thermal_governor thermal_gov_power_allocator = {
 	.bind_to_tz	= power_allocator_bind,
 	.unbind_from_tz	= power_allocator_unbind,
 	.throttle	= power_allocator_throttle,
+	.update_tz	= power_allocator_update_tz,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
-- 
2.25.1

Re: [PATCH v2 3/8] thermal: gov_power_allocator: Move memory allocation out of throttle()

[Rafael J. Wysocki]

On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> The new thermal callback allows to react to the change of cooling
> instances in the thermal zone. Move the memory allocation to that new
> callback and save CPU cycles in the throttle() code path.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/gov_power_allocator.c | 144 ++++++++++++++++++++------
>  1 file changed, 113 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 38e1e89ba10c..3328c3ec71f2 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -61,6 +61,13 @@ 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.
> + * @num_actors:                number of cooling devices supporting IPA callbacks
> + * @buffer_size:       IPA internal buffer size
> + * @req_power:         IPA buffer for requested power
> + * @max_power:         IPA buffer for max allocatable power
> + * @granted_power:     IPA buffer for granted power
> + * @extra_actor_power: IPA buffer for extra power
> + * @weighted_req_power:        IPA buffer for weighted requested power
>   */
>  struct power_allocator_params {
>         bool allocated_tzp;
> @@ -69,6 +76,13 @@ struct power_allocator_params {
>         u32 sustainable_power;
>         const struct thermal_trip *trip_switch_on;
>         const struct thermal_trip *trip_max;
> +       int num_actors;
> +       int buffer_size;

None of the above can be negative, so maybe consider using unsigned int?

> +       u32 *req_power;
> +       u32 *max_power;
> +       u32 *granted_power;
> +       u32 *extra_actor_power;
> +       u32 *weighted_req_power;
>  };
>
>  /**
> @@ -387,39 +401,24 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
>         u32 *weighted_req_power;
>         u32 power_range, weight;
>         int total_weight = 0;
> -       int num_actors = 0;

You could retain this local var and set it to params->num_actors.  It
is kind of inconsistent to drop it and still use the local variables
above.

>         int i = 0;
>
> -       list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> +       if (!params->num_actors)
> +               return -ENODEV;
> +
> +       list_for_each_entry(instance, &tz->thermal_instances, tz_node)
>                 if ((instance->trip == params->trip_max) &&
> -                   cdev_is_power_actor(instance->cdev)) {
> -                       num_actors++;
> +                   cdev_is_power_actor(instance->cdev))
>                         total_weight += instance->weight;
> -               }
> -       }
> -
> -       if (!num_actors)
> -               return -ENODEV;
>
> -       /*
> -        * We need to allocate five arrays of the same size:
> -        * req_power, max_power, granted_power, extra_actor_power and
> -        * weighted_req_power.  They are going to be needed until this
> -        * function returns.  Allocate them all in one go to simplify
> -        * the allocation and deallocation logic.
> -        */
> -       BUILD_BUG_ON(sizeof(*req_power) != sizeof(*max_power));
> -       BUILD_BUG_ON(sizeof(*req_power) != sizeof(*granted_power));
> -       BUILD_BUG_ON(sizeof(*req_power) != sizeof(*extra_actor_power));
> -       BUILD_BUG_ON(sizeof(*req_power) != sizeof(*weighted_req_power));
> -       req_power = kcalloc(num_actors * 5, sizeof(*req_power), GFP_KERNEL);
> -       if (!req_power)
> -               return -ENOMEM;
> +       /* Clean all buffers for new power estimations */
> +       memset(params->req_power, 0, params->buffer_size);
>
> -       max_power = &req_power[num_actors];
> -       granted_power = &req_power[2 * num_actors];
> -       extra_actor_power = &req_power[3 * num_actors];
> -       weighted_req_power = &req_power[4 * num_actors];
> +       req_power = params->req_power;
> +       max_power = params->max_power;
> +       granted_power = params->granted_power;
> +       extra_actor_power = params->extra_actor_power;
> +       weighted_req_power = params->weighted_req_power;
>
>         list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>                 cdev = instance->cdev;
> @@ -453,7 +452,7 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
>
>         power_range = pid_controller(tz, control_temp, max_allocatable_power);
>
> -       divvy_up_power(weighted_req_power, max_power, num_actors,
> +       divvy_up_power(weighted_req_power, max_power, params->num_actors,
>                        total_weighted_req_power, power_range, granted_power,
>                        extra_actor_power);
>
> @@ -474,12 +473,10 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
>
>         trace_thermal_power_allocator(tz, req_power, total_req_power,
>                                       granted_power, total_granted_power,
> -                                     num_actors, power_range,
> +                                     params->num_actors, power_range,
>                                       max_allocatable_power, tz->temperature,
>                                       control_temp - tz->temperature);
>
> -       kfree(req_power);
> -
>         return 0;
>  }
>
> @@ -606,6 +603,81 @@ static int check_power_actors(struct thermal_zone_device *tz,
>         return ret;
>  }
>
> +static void _power_buffers_init(struct power_allocator_params *params,
> +                               u32 *req_power, u32 *max_power,
> +                               u32 *granted_power, u32 *extra_actor_power,
> +                               u32 *weighted_req_power)
> +
> +{
> +       /* Setup internal buffers for power calculations. */
> +       params->req_power = req_power;
> +       params->max_power = max_power;
> +       params->granted_power = granted_power;
> +       params->extra_actor_power = extra_actor_power;
> +       params->weighted_req_power = weighted_req_power;
> +}
> +
> +static int allocate_actors_buffer(struct power_allocator_params *params,
> +                                 int num_actors)
> +{
> +       u32 *req_power;
> +       int ret;
> +
> +       kfree(params->req_power);
> +
> +       /* There might be no cooling devices yet. */
> +       if (!num_actors) {
> +               ret = -EINVAL;
> +               goto clean_buffers;
> +       }
> +
> +       req_power = kcalloc(num_actors * 5, sizeof(u32), GFP_KERNEL);

I'd use sizeof(*req_power) instead of sizeof(u32) here and below.

> +       if (!req_power) {
> +               ret = -ENOMEM;
> +               goto clean_buffers;
> +       }
> +
> +       params->num_actors = num_actors;
> +       params->buffer_size = num_actors * 5 * sizeof(u32);
> +
> +       _power_buffers_init(params, req_power, &req_power[params->num_actors],
> +                           &req_power[2 * params->num_actors],
> +                           &req_power[3 * params->num_actors],
> +                           &req_power[4 * params->num_actors]);

Why don't you use the local var in this instead of the struct member?
It would be easier to read then IMO.

Also, I would rather use pointer arithmetic, so it would become

_power_buffers_init(params, req_power, req_power + num_actors,
req_power + 2 * num_actors, req_power + 3 * num_actors, req_power + 4
* num_actors);

And note that you could introduce something like

struct power_actor {
        u32 req_power;
        u32 max_power;
        u32 granted_power;
        u32 extra_actor_power;
        u32 weighted_req_power;
};

and use a single array of these instead of 5 different arrays of u32,
which would result in more straightforward code if I'm not mistaken.

> +
> +       return 0;
> +
> +clean_buffers:
> +       params->num_actors = -EINVAL;
> +       params->buffer_size = 0;
> +       _power_buffers_init(params, NULL, NULL, NULL, NULL, NULL);
> +       return ret;
> +}
> +
> +static void power_allocator_update_tz(struct thermal_zone_device *tz,
> +                                     enum thermal_notify_event reason)
> +{
> +       struct power_allocator_params *params = tz->governor_data;
> +       struct thermal_instance *instance;
> +       int num_actors = 0;
> +
> +       switch (reason) {
> +       case THERMAL_INSTANCE_LIST_UPDATE:
> +               list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> +                       if ((instance->trip == params->trip_max) &&
> +                           cdev_is_power_actor(instance->cdev))
> +                               num_actors++;
> +
> +               if (num_actors == params->num_actors)
> +                       return;
> +
> +               allocate_actors_buffer(params, num_actors);
> +               break;
> +       default:
> +               break;
> +       }
> +}
> +
>  /**
>   * power_allocator_bind() - bind the power_allocator governor to a thermal zone
>   * @tz:        thermal zone to bind it to
> @@ -639,6 +711,13 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
>                 return ret;
>         }
>
> +       ret = allocate_actors_buffer(params, ret);
> +       if (ret) {
> +               dev_warn(&tz->device, "power_allocator: allocation failed\n");
> +               kfree(params);
> +               return ret;
> +       }
> +
>         if (!tz->tzp) {
>                 tz->tzp = kzalloc(sizeof(*tz->tzp), GFP_KERNEL);
>                 if (!tz->tzp) {
> @@ -663,6 +742,7 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
>         return 0;
>
>  free_params:
> +       kfree(params->req_power);
>         kfree(params);
>
>         return ret;
> @@ -679,6 +759,7 @@ static void power_allocator_unbind(struct thermal_zone_device *tz)
>                 tz->tzp = NULL;
>         }
>
> +       kfree(params->req_power);
>         kfree(tz->governor_data);
>         tz->governor_data = NULL;
>  }
> @@ -717,5 +798,6 @@ static struct thermal_governor thermal_gov_power_allocator = {
>         .bind_to_tz     = power_allocator_bind,
>         .unbind_from_tz = power_allocator_unbind,
>         .throttle       = power_allocator_throttle,
> +       .update_tz      = power_allocator_update_tz,
>  };
>  THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> --

Re: [PATCH v2 3/8] thermal: gov_power_allocator: Move memory allocation out of throttle()

[Lukasz Luba]

On 12/20/23 14:35, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> The new thermal callback allows to react to the change of cooling
>> instances in the thermal zone. Move the memory allocation to that new
>> callback and save CPU cycles in the throttle() code path.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/thermal/gov_power_allocator.c | 144 ++++++++++++++++++++------
>>   1 file changed, 113 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
>> index 38e1e89ba10c..3328c3ec71f2 100644
>> --- a/drivers/thermal/gov_power_allocator.c
>> +++ b/drivers/thermal/gov_power_allocator.c
>> @@ -61,6 +61,13 @@ 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.
>> + * @num_actors:                number of cooling devices supporting IPA callbacks
>> + * @buffer_size:       IPA internal buffer size
>> + * @req_power:         IPA buffer for requested power
>> + * @max_power:         IPA buffer for max allocatable power
>> + * @granted_power:     IPA buffer for granted power
>> + * @extra_actor_power: IPA buffer for extra power
>> + * @weighted_req_power:        IPA buffer for weighted requested power
>>    */
>>   struct power_allocator_params {
>>          bool allocated_tzp;
>> @@ -69,6 +76,13 @@ struct power_allocator_params {
>>          u32 sustainable_power;
>>          const struct thermal_trip *trip_switch_on;
>>          const struct thermal_trip *trip_max;
>> +       int num_actors;
>> +       int buffer_size;
> 
> None of the above can be negative, so maybe consider using unsigned int?

True, I'll change them to unsigned.

> 
>> +       u32 *req_power;
>> +       u32 *max_power;
>> +       u32 *granted_power;
>> +       u32 *extra_actor_power;
>> +       u32 *weighted_req_power;
>>   };
>>
>>   /**
>> @@ -387,39 +401,24 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
>>          u32 *weighted_req_power;
>>          u32 power_range, weight;
>>          int total_weight = 0;
>> -       int num_actors = 0;
> 
> You could retain this local var and set it to params->num_actors.  It
> is kind of inconsistent to drop it and still use the local variables
> above.

OK, I'll do that.

[snip]

>> +
>> +       req_power = kcalloc(num_actors * 5, sizeof(u32), GFP_KERNEL);
> 
> I'd use sizeof(*req_power) instead of sizeof(u32) here and below.

OK

> 
>> +       if (!req_power) {
>> +               ret = -ENOMEM;
>> +               goto clean_buffers;
>> +       }
>> +
>> +       params->num_actors = num_actors;
>> +       params->buffer_size = num_actors * 5 * sizeof(u32);
>> +
>> +       _power_buffers_init(params, req_power, &req_power[params->num_actors],
>> +                           &req_power[2 * params->num_actors],
>> +                           &req_power[3 * params->num_actors],
>> +                           &req_power[4 * params->num_actors]);
> 
> Why don't you use the local var in this instead of the struct member?
> It would be easier to read then IMO.
> 
> Also, I would rather use pointer arithmetic, so it would become
> 
> _power_buffers_init(params, req_power, req_power + num_actors,
> req_power + 2 * num_actors, req_power + 3 * num_actors, req_power + 4
> * num_actors);
> 
> And note that you could introduce something like
> 
> struct power_actor {
>          u32 req_power;
>          u32 max_power;
>          u32 granted_power;
>          u32 extra_actor_power;
>          u32 weighted_req_power;
> };
> 
> and use a single array of these instead of 5 different arrays of u32,
> which would result in more straightforward code if I'm not mistaken.

That sounds like a good idea. Let me implement it and see - but it
should be a better way. Thanks!

[PATCH v2 4/8] thermal: gov_power_allocator: Simplify checks for valid power actor

[Lukasz Luba]

There is a need to check if the cooling device in the thermal zone
supports IPA callback and is set for control trip point.
Refactor the code which validates the power actor capabilities and
make it more consistent in all places.

No intentional functional impact.

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

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 3328c3ec71f2..1a605fd9c8c6 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -85,6 +85,13 @@ struct power_allocator_params {
 	u32 *weighted_req_power;
 };
 
+static bool power_actor_is_valid(struct power_allocator_params *params,
+				 struct thermal_instance *instance)
+{
+	return ((instance->trip == params->trip_max) &&
+		 cdev_is_power_actor(instance->cdev));
+}
+
 /**
  * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone
  * @tz: thermal zone we are operating in
@@ -105,14 +112,10 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
 	u32 min_power;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		cdev = instance->cdev;
-
-		if (instance->trip != params->trip_max)
-			continue;
-
-		if (!cdev_is_power_actor(cdev))
+		if (!power_actor_is_valid(params, instance))
 			continue;
 
+		cdev = instance->cdev;
 		if (cdev->ops->state2power(cdev, instance->upper, &min_power))
 			continue;
 
@@ -407,8 +410,7 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 		return -ENODEV;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-		if ((instance->trip == params->trip_max) &&
-		    cdev_is_power_actor(instance->cdev))
+		if (power_actor_is_valid(params, instance))
 			total_weight += instance->weight;
 
 	/* Clean all buffers for new power estimations */
@@ -421,14 +423,10 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 	weighted_req_power = params->weighted_req_power;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		cdev = instance->cdev;
-
-		if (instance->trip != params->trip_max)
-			continue;
-
-		if (!cdev_is_power_actor(cdev))
+		if (!power_actor_is_valid(params, instance))
 			continue;
 
+		cdev = instance->cdev;
 		if (cdev->ops->get_requested_power(cdev, &req_power[i]))
 			continue;
 
@@ -458,10 +456,7 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
 
 	i = 0;
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		if (instance->trip != params->trip_max)
-			continue;
-
-		if (!cdev_is_power_actor(instance->cdev))
+		if (!power_actor_is_valid(params, instance))
 			continue;
 
 		power_actor_set_power(instance->cdev, instance,
@@ -546,12 +541,11 @@ static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
 	u32 req_power;
 
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
-		cdev = instance->cdev;
-
-		if (instance->trip != params->trip_max ||
-		    !cdev_is_power_actor(instance->cdev))
+		if (!power_actor_is_valid(params, instance))
 			continue;
 
+		cdev = instance->cdev;
+
 		instance->target = 0;
 		mutex_lock(&cdev->lock);
 		/*
@@ -664,8 +658,7 @@ static void power_allocator_update_tz(struct thermal_zone_device *tz,
 	switch (reason) {
 	case THERMAL_INSTANCE_LIST_UPDATE:
 		list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-			if ((instance->trip == params->trip_max) &&
-			    cdev_is_power_actor(instance->cdev))
+			if (power_actor_is_valid(params, instance))
 				num_actors++;
 
 		if (num_actors == params->num_actors)
-- 
2.25.1

Re: [PATCH v2 4/8] thermal: gov_power_allocator: Simplify checks for valid power actor

[Rafael J. Wysocki]

On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> There is a need to check if the cooling device in the thermal zone
> supports IPA callback and is set for control trip point.
> Refactor the code which validates the power actor capabilities and
> make it more consistent in all places.

This really is about reducing code duplication which is worth
mentioning, so I would say

"Add a helper to check if a given cooling device in a thermal zone
supports the IPA callback and is bound to the control trip point and
use it wherever that check is needed to reduce code duplication."

>
> No intentional functional impact.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/gov_power_allocator.c | 41 +++++++++++----------------
>  1 file changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 3328c3ec71f2..1a605fd9c8c6 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -85,6 +85,13 @@ struct power_allocator_params {
>         u32 *weighted_req_power;
>  };
>
> +static bool power_actor_is_valid(struct power_allocator_params *params,
> +                                struct thermal_instance *instance)
> +{
> +       return ((instance->trip == params->trip_max) &&

The inner parens are redundant.

> +                cdev_is_power_actor(instance->cdev));
> +}

The part below LGTM.

Re: [PATCH v2 4/8] thermal: gov_power_allocator: Simplify checks for valid power actor

[Lukasz Luba]

On 12/20/23 14:40, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> There is a need to check if the cooling device in the thermal zone
>> supports IPA callback and is set for control trip point.
>> Refactor the code which validates the power actor capabilities and
>> make it more consistent in all places.
> 
> This really is about reducing code duplication which is worth
> mentioning, so I would say
> 
> "Add a helper to check if a given cooling device in a thermal zone
> supports the IPA callback and is bound to the control trip point and
> use it wherever that check is needed to reduce code duplication."
> 

Thanks, I'll use it.

>>
>> No intentional functional impact.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/thermal/gov_power_allocator.c | 41 +++++++++++----------------
>>   1 file changed, 17 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
>> index 3328c3ec71f2..1a605fd9c8c6 100644
>> --- a/drivers/thermal/gov_power_allocator.c
>> +++ b/drivers/thermal/gov_power_allocator.c
>> @@ -85,6 +85,13 @@ struct power_allocator_params {
>>          u32 *weighted_req_power;
>>   };
>>
>> +static bool power_actor_is_valid(struct power_allocator_params *params,
>> +                                struct thermal_instance *instance)
>> +{
>> +       return ((instance->trip == params->trip_max) &&
> 
> The inner parens are redundant.

OK

> 
>> +                cdev_is_power_actor(instance->cdev));
>> +}
> 
> The part below LGTM.

[PATCH v2 5/8] thermal: gov_power_allocator: Refactor checks in divvy_up_power()

[Lukasz Luba]

Simplify the code and remove one extra 'if' block.

No intentional functional impact.

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

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 1a605fd9c8c6..574aa5822112 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -349,7 +349,8 @@ static void divvy_up_power(u32 *req_power, u32 *max_power, int num_actors,
 			   u32 total_req_power, u32 power_range,
 			   u32 *granted_power, u32 *extra_actor_power)
 {
-	u32 extra_power, capped_extra_power;
+	u32 capped_extra_power = 0;
+	u32 extra_power = 0;
 	int i;
 
 	/*
@@ -358,8 +359,6 @@ static void divvy_up_power(u32 *req_power, u32 *max_power, int num_actors,
 	if (!total_req_power)
 		total_req_power = 1;
 
-	capped_extra_power = 0;
-	extra_power = 0;
 	for (i = 0; i < num_actors; i++) {
 		u64 req_range = (u64)req_power[i] * power_range;
 
@@ -375,7 +374,7 @@ static void divvy_up_power(u32 *req_power, u32 *max_power, int num_actors,
 		capped_extra_power += extra_actor_power[i];
 	}
 
-	if (!extra_power)
+	if (!extra_power || !capped_extra_power)
 		return;
 
 	/*
@@ -383,12 +382,13 @@ static void divvy_up_power(u32 *req_power, u32 *max_power, int num_actors,
 	 * how far they are from the max
 	 */
 	extra_power = min(extra_power, capped_extra_power);
-	if (capped_extra_power > 0)
-		for (i = 0; i < num_actors; i++) {
-			u64 extra_range = (u64)extra_actor_power[i] * extra_power;
-			granted_power[i] += DIV_ROUND_CLOSEST_ULL(extra_range,
-							 capped_extra_power);
-		}
+
+	for (i = 0; i < num_actors; i++) {
+		u64 extra_range = (u64)extra_actor_power[i] * extra_power;
+
+		granted_power[i] += DIV_ROUND_CLOSEST_ULL(extra_range,
+						 capped_extra_power);
+	}
 }
 
 static int allocate_power(struct thermal_zone_device *tz, int control_temp)
-- 
2.25.1

Re: [PATCH v2 5/8] thermal: gov_power_allocator: Refactor checks in divvy_up_power()

[Rafael J. Wysocki]

On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Simplify the code and remove one extra 'if' block.
>
> No intentional functional impact.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/gov_power_allocator.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 1a605fd9c8c6..574aa5822112 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -349,7 +349,8 @@ static void divvy_up_power(u32 *req_power, u32 *max_power, int num_actors,
>                            u32 total_req_power, u32 power_range,
>                            u32 *granted_power, u32 *extra_actor_power)
>  {
> -       u32 extra_power, capped_extra_power;
> +       u32 capped_extra_power = 0;
> +       u32 extra_power = 0;
>         int i;
>
>         /*
> @@ -358,8 +359,6 @@ static void divvy_up_power(u32 *req_power, u32 *max_power, int num_actors,
>         if (!total_req_power)
>                 total_req_power = 1;
>
> -       capped_extra_power = 0;
> -       extra_power = 0;
>         for (i = 0; i < num_actors; i++) {
>                 u64 req_range = (u64)req_power[i] * power_range;
>
> @@ -375,7 +374,7 @@ static void divvy_up_power(u32 *req_power, u32 *max_power, int num_actors,
>                 capped_extra_power += extra_actor_power[i];
>         }
>
> -       if (!extra_power)
> +       if (!extra_power || !capped_extra_power)
>                 return;
>
>         /*
> @@ -383,12 +382,13 @@ static void divvy_up_power(u32 *req_power, u32 *max_power, int num_actors,
>          * how far they are from the max
>          */
>         extra_power = min(extra_power, capped_extra_power);
> -       if (capped_extra_power > 0)
> -               for (i = 0; i < num_actors; i++) {
> -                       u64 extra_range = (u64)extra_actor_power[i] * extra_power;
> -                       granted_power[i] += DIV_ROUND_CLOSEST_ULL(extra_range,
> -                                                        capped_extra_power);
> -               }
> +
> +       for (i = 0; i < num_actors; i++) {
> +               u64 extra_range = (u64)extra_actor_power[i] * extra_power;
> +
> +               granted_power[i] += DIV_ROUND_CLOSEST_ULL(extra_range,
> +                                                capped_extra_power);
> +       }
>  }
>
>  static int allocate_power(struct thermal_zone_device *tz, int control_temp)
> --

Looks good to me.

[PATCH v2 6/8] thermal/sysfs: Update instance->weight under tz lock

[Lukasz Luba]

The user-space can change thermal instance weight value while the
throtte() callback is running for a governor. The IPA governor uses the
'weight' value for power calculation and also keeps it in 'total_weight'.
Therefore, the 'weight' value must not change during the throttle()
callback. Use 'tz->lock' mutex which also guards the throttle() to make
the update value safe.

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

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index eef40d4f3063..df85df7d4a88 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -961,6 +961,7 @@ ssize_t weight_store(struct device *dev, struct device_attribute *attr,
 		     const char *buf, size_t count)
 {
 	struct thermal_instance *instance;
+	struct thermal_zone_device *tz;
 	int ret, weight;
 
 	ret = kstrtoint(buf, 0, &weight);
@@ -968,7 +969,12 @@ ssize_t weight_store(struct device *dev, struct device_attribute *attr,
 		return ret;
 
 	instance = container_of(attr, struct thermal_instance, weight_attr);
+	tz = instance->tz;
+
+	/* Don't race with governors using the 'weight' value */
+	mutex_lock(&tz->lock);
 	instance->weight = weight;
+	mutex_unlock(&tz->lock);
 
 	return count;
 }
-- 
2.25.1

Re: [PATCH v2 6/8] thermal/sysfs: Update instance->weight under tz lock

[Rafael J. Wysocki]

On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> The user-space can change thermal instance weight value while the
> throtte() callback is running for a governor.

This needs to be more precise IMV.  Something like

"User space can change the weight of a thermal instance via sysfs
while the .throttle() callback is running for a governor, because
weight_store() does not use the zone lock."

> The IPA governor uses the
> 'weight' value for power calculation and also keeps it in 'total_weight'.
> Therefore, the 'weight' value must not change during the throttle()
> callback. Use 'tz->lock' mutex which also guards the throttle() to make
> the update value safe.

"The IPA governor uses instance weight values for power calculations
and caches the sum of them as total_weight, so it gets confused when
one of them changes while its .throttle() callback is running.

To prevent that from happening, use thermal zone locking in weight_store()."

>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/thermal_sysfs.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index eef40d4f3063..df85df7d4a88 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -961,6 +961,7 @@ ssize_t weight_store(struct device *dev, struct device_attribute *attr,
>                      const char *buf, size_t count)
>  {
>         struct thermal_instance *instance;
> +       struct thermal_zone_device *tz;

IMO the local var doesn't help much.  You may as well use
instance->tz->lock directly below.

>         int ret, weight;
>
>         ret = kstrtoint(buf, 0, &weight);
> @@ -968,7 +969,12 @@ ssize_t weight_store(struct device *dev, struct device_attribute *attr,
>                 return ret;
>
>         instance = container_of(attr, struct thermal_instance, weight_attr);
> +       tz = instance->tz;
> +
> +       /* Don't race with governors using the 'weight' value */
> +       mutex_lock(&tz->lock);
>         instance->weight = weight;
> +       mutex_unlock(&tz->lock);
>
>         return count;
>  }
> --

Re: [PATCH v2 6/8] thermal/sysfs: Update instance->weight under tz lock

[Lukasz Luba]

On 12/20/23 14:53, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> The user-space can change thermal instance weight value while the
>> throtte() callback is running for a governor.
> 
> This needs to be more precise IMV.  Something like
> 
> "User space can change the weight of a thermal instance via sysfs
> while the .throttle() callback is running for a governor, because
> weight_store() does not use the zone lock."

Agree, I'll use it.

> 
>> The IPA governor uses the
>> 'weight' value for power calculation and also keeps it in 'total_weight'.
>> Therefore, the 'weight' value must not change during the throttle()
>> callback. Use 'tz->lock' mutex which also guards the throttle() to make
>> the update value safe.
> 
> "The IPA governor uses instance weight values for power calculations
> and caches the sum of them as total_weight, so it gets confused when
> one of them changes while its .throttle() callback is running.
> 
> To prevent that from happening, use thermal zone locking in weight_store()."

Agree, thanks.

> 
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/thermal/thermal_sysfs.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
>> index eef40d4f3063..df85df7d4a88 100644
>> --- a/drivers/thermal/thermal_sysfs.c
>> +++ b/drivers/thermal/thermal_sysfs.c
>> @@ -961,6 +961,7 @@ ssize_t weight_store(struct device *dev, struct device_attribute *attr,
>>                       const char *buf, size_t count)
>>   {
>>          struct thermal_instance *instance;
>> +       struct thermal_zone_device *tz;
> 
> IMO the local var doesn't help much.  You may as well use
> instance->tz->lock directly below.

OK, I'll change that.

> 
>>          int ret, weight;
>>
>>          ret = kstrtoint(buf, 0, &weight);
>> @@ -968,7 +969,12 @@ ssize_t weight_store(struct device *dev, struct device_attribute *attr,
>>                  return ret;
>>
>>          instance = container_of(attr, struct thermal_instance, weight_attr);
>> +       tz = instance->tz;
>> +
>> +       /* Don't race with governors using the 'weight' value */
>> +       mutex_lock(&tz->lock);
>>          instance->weight = weight;
>> +       mutex_unlock(&tz->lock);
>>
>>          return count;
>>   }
>> --

[PATCH v2 7/8] thermal/sysfs: Update governors when the 'weight' has changed

[Lukasz Luba]

Support governors update when the thermal instance's weight has changed.
This allows to adjust internal state for the governor.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/thermal_sysfs.c | 9 +++++++++
 include/linux/thermal.h         | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index df85df7d4a88..9afa2e2b76b9 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -957,6 +957,14 @@ weight_show(struct device *dev, struct device_attribute *attr, char *buf)
 	return sprintf(buf, "%d\n", instance->weight);
 }
 
+static void handle_weight_update(struct thermal_zone_device *tz)
+{
+	if (!tz->governor || !tz->governor->update_tz)
+		return;
+
+	tz->governor->update_tz(tz, THERMAL_INSTANCE_WEIGHT_UPDATE);
+}
+
 ssize_t weight_store(struct device *dev, struct device_attribute *attr,
 		     const char *buf, size_t count)
 {
@@ -974,6 +982,7 @@ ssize_t weight_store(struct device *dev, struct device_attribute *attr,
 	/* Don't race with governors using the 'weight' value */
 	mutex_lock(&tz->lock);
 	instance->weight = weight;
+	handle_weight_update(tz);
 	mutex_unlock(&tz->lock);
 
 	return count;
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 9fd0d3fb234a..24176f075fbf 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -52,6 +52,7 @@ enum thermal_notify_event {
 	THERMAL_TABLE_CHANGED, /* Thermal table(s) changed */
 	THERMAL_EVENT_KEEP_ALIVE, /* Request for user space handler to respond */
 	THERMAL_INSTANCE_LIST_UPDATE, /* List of thermal instances changed */
+	THERMAL_INSTANCE_WEIGHT_UPDATE, /* Thermal instance weight changed */
 };
 
 /**
-- 
2.25.1

Re: [PATCH v2 7/8] thermal/sysfs: Update governors when the 'weight' has changed

[Rafael J. Wysocki]

On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Support governors update when the thermal instance's weight has changed.
> This allows to adjust internal state for the governor.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/thermal_sysfs.c | 9 +++++++++
>  include/linux/thermal.h         | 1 +
>  2 files changed, 10 insertions(+)
>
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index df85df7d4a88..9afa2e2b76b9 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -957,6 +957,14 @@ weight_show(struct device *dev, struct device_attribute *attr, char *buf)
>         return sprintf(buf, "%d\n", instance->weight);
>  }
>
> +static void handle_weight_update(struct thermal_zone_device *tz)
> +{
> +       if (!tz->governor || !tz->governor->update_tz)
> +               return;
> +
> +       tz->governor->update_tz(tz, THERMAL_INSTANCE_WEIGHT_UPDATE);
> +}

So if you follow my advice from the comment on the first patch, you'll
be able to use the helper as is without introducing a new one.

> +
>  ssize_t weight_store(struct device *dev, struct device_attribute *attr,
>                      const char *buf, size_t count)
>  {
> @@ -974,6 +982,7 @@ ssize_t weight_store(struct device *dev, struct device_attribute *attr,
>         /* Don't race with governors using the 'weight' value */
>         mutex_lock(&tz->lock);
>         instance->weight = weight;
> +       handle_weight_update(tz);

So this will become

thermal_governor_update_tz(tz, THERMAL_INSTANCE_WEIGHT_UPDATE);

>         mutex_unlock(&tz->lock);
>
>         return count;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 9fd0d3fb234a..24176f075fbf 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -52,6 +52,7 @@ enum thermal_notify_event {
>         THERMAL_TABLE_CHANGED, /* Thermal table(s) changed */
>         THERMAL_EVENT_KEEP_ALIVE, /* Request for user space handler to respond */
>         THERMAL_INSTANCE_LIST_UPDATE, /* List of thermal instances changed */
> +       THERMAL_INSTANCE_WEIGHT_UPDATE, /* Thermal instance weight changed */

And I'd slightly prefer THERMAL_INSTANCE_WEIGHT_CHANGE.

>  };
>
>  /**
> --

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

[Lukasz Luba]

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

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

[Rafael J. Wysocki]

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.

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

[Lukasz Luba]

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

Re: [PATCH v2 0/8] Add callback for cooling list update to speed-up IPA

[Lukasz Luba]

Hi Daniel, Rafael,

On 12/12/23 13:48, Lukasz Luba wrote:
> Hi all,
> 
> The patch set a new callback for thermal governors and implementation for
> Intelligent Power Allocator.
> 
> The goal is to move some heavy operarions like the memory allocations and heavy
> computations (multiplications) out of throttle() callback hot path.
> 
> The new callback is generic enough to handle other imporants update events.
> It re-uses existing thermal_notify_event definitions.
> 
> In addition there are some small clean-ups for IPA code.
> 
> changes:
> v2:
> - change callback name to update_tz() and add parameter (Rafael)
> - added new event to trigger this callback - instance 'weight' update
> 
> Regards,
> Lukasz
> 
> Lukasz Luba (8):
>    thermal: core: Add governor callback for thermal zone change
>    thermal: gov_power_allocator: Refactor check_power_actors()
>    thermal: gov_power_allocator: Move memory allocation out of throttle()
>    thermal: gov_power_allocator: Simplify checks for valid power actor
>    thermal: gov_power_allocator: Refactor checks in divvy_up_power()
>    thermal/sysfs: Update instance->weight under tz lock
>    thermal/sysfs: Update governors when the 'weight' has changed
>    thermal: gov_power_allocator: Support new update callback of weights
> 
>   drivers/thermal/gov_power_allocator.c | 216 ++++++++++++++++++--------
>   drivers/thermal/thermal_core.c        |  13 ++
>   drivers/thermal/thermal_sysfs.c       |  15 ++
>   include/linux/thermal.h               |   6 +
>   4 files changed, 182 insertions(+), 68 deletions(-)
> 

I know it's a bit late in time period...
You probably missed that patch set in your mailbox.
This patch set can probably just wait to the next window, or
should I resend it later in 2024?

Regards,
Lukasz

Re: [PATCH v2 0/8] Add callback for cooling list update to speed-up IPA

[Rafael J. Wysocki]

On Wed, Dec 20, 2023 at 1:53 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Daniel, Rafael,
>
> On 12/12/23 13:48, Lukasz Luba wrote:
> > Hi all,
> >
> > The patch set a new callback for thermal governors and implementation for
> > Intelligent Power Allocator.
> >
> > The goal is to move some heavy operarions like the memory allocations and heavy
> > computations (multiplications) out of throttle() callback hot path.
> >
> > The new callback is generic enough to handle other imporants update events.
> > It re-uses existing thermal_notify_event definitions.
> >
> > In addition there are some small clean-ups for IPA code.
> >
> > changes:
> > v2:
> > - change callback name to update_tz() and add parameter (Rafael)
> > - added new event to trigger this callback - instance 'weight' update
> >
> > Regards,
> > Lukasz
> >
> > Lukasz Luba (8):
> >    thermal: core: Add governor callback for thermal zone change
> >    thermal: gov_power_allocator: Refactor check_power_actors()
> >    thermal: gov_power_allocator: Move memory allocation out of throttle()
> >    thermal: gov_power_allocator: Simplify checks for valid power actor
> >    thermal: gov_power_allocator: Refactor checks in divvy_up_power()
> >    thermal/sysfs: Update instance->weight under tz lock
> >    thermal/sysfs: Update governors when the 'weight' has changed
> >    thermal: gov_power_allocator: Support new update callback of weights
> >
> >   drivers/thermal/gov_power_allocator.c | 216 ++++++++++++++++++--------
> >   drivers/thermal/thermal_core.c        |  13 ++
> >   drivers/thermal/thermal_sysfs.c       |  15 ++
> >   include/linux/thermal.h               |   6 +
> >   4 files changed, 182 insertions(+), 68 deletions(-)
> >
>
> I know it's a bit late in time period...
> You probably missed that patch set in your mailbox.
> This patch set can probably just wait to the next window, or
> should I resend it later in 2024?

Not really, I was about to comment one the first patch.

I'll do that shortly.

Re: [PATCH v2 0/8] Add callback for cooling list update to speed-up IPA

[Lukasz Luba]

On 12/20/23 13:37, Rafael J. Wysocki wrote:
> On Wed, Dec 20, 2023 at 1:53 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Daniel, Rafael,
>>
>> On 12/12/23 13:48, Lukasz Luba wrote:
>>> Hi all,
>>>
>>> The patch set a new callback for thermal governors and implementation for
>>> Intelligent Power Allocator.
>>>
>>> The goal is to move some heavy operarions like the memory allocations and heavy
>>> computations (multiplications) out of throttle() callback hot path.
>>>
>>> The new callback is generic enough to handle other imporants update events.
>>> It re-uses existing thermal_notify_event definitions.
>>>
>>> In addition there are some small clean-ups for IPA code.
>>>
>>> changes:
>>> v2:
>>> - change callback name to update_tz() and add parameter (Rafael)
>>> - added new event to trigger this callback - instance 'weight' update
>>>
>>> Regards,
>>> Lukasz
>>>
>>> Lukasz Luba (8):
>>>     thermal: core: Add governor callback for thermal zone change
>>>     thermal: gov_power_allocator: Refactor check_power_actors()
>>>     thermal: gov_power_allocator: Move memory allocation out of throttle()
>>>     thermal: gov_power_allocator: Simplify checks for valid power actor
>>>     thermal: gov_power_allocator: Refactor checks in divvy_up_power()
>>>     thermal/sysfs: Update instance->weight under tz lock
>>>     thermal/sysfs: Update governors when the 'weight' has changed
>>>     thermal: gov_power_allocator: Support new update callback of weights
>>>
>>>    drivers/thermal/gov_power_allocator.c | 216 ++++++++++++++++++--------
>>>    drivers/thermal/thermal_core.c        |  13 ++
>>>    drivers/thermal/thermal_sysfs.c       |  15 ++
>>>    include/linux/thermal.h               |   6 +
>>>    4 files changed, 182 insertions(+), 68 deletions(-)
>>>
>>
>> I know it's a bit late in time period...
>> You probably missed that patch set in your mailbox.
>> This patch set can probably just wait to the next window, or
>> should I resend it later in 2024?
> 
> Not really, I was about to comment one the first patch.
> 
> I'll do that shortly.
> 

Thank you for the comments. I'll send a v3 with changes.