[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 12 Dec 2023 08:36:36 +0000
From: Lukasz Luba <lukasz.luba@....com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: linux-kernel@...r.kernel.org, daniel.lezcano@...aro.org,
linux-pm@...r.kernel.org, rui.zhang@...el.com
Subject: Re: [PATCH 1/5] thermal: core: Add callback for governors with
cooling instances change
On 12/11/23 20:41, Rafael J. Wysocki wrote:
> On Wed, Dec 6, 2023 at 12:30 PM Lukasz Luba <lukasz.luba@....com> wrote:
>>
>> Allow governors to react to the changes in the cooling instances list.
>> That makes possible to move memory allocations related to the number of
>> cooling instances out of the throttle() callback. The throttle() callback
>> is called much more often thus the cost of managing allocations there is
>> an extra overhead. The list of cooling instances is not changed that often
>> and it can be handled in dedicated callback. That will save CPU cycles
>> in the throttle() code path. Both callbacks code paths are protected with
>> the same thermal zone lock, which guaranties the list of cooling instances
>> is consistent.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@....com>
>
> I agree with the direction, but I'm wondering if changes of the
> bindings between trip points and cooling devices are the only type of
> changes which can affect the IPA. For instance, what if the trip
> point temperatures are updated?
Yes, that example sounds also interesting for a callback. The trip
temperature update won't affect the allocation/free code, though.
>
> If it needs to react to other types of changes in general, it may be
> good to introduce a more general callback that can be made handle them
> in the future.
Fair enough.
>
>> ---
>> drivers/thermal/thermal_core.c | 14 ++++++++++++++
>> include/linux/thermal.h | 4 ++++
>> 2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index 625ba07cbe2f..c993b86f7fb5 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -314,6 +314,15 @@ 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->instances_update)
>> + return;
>> +
>> + tz->governor->instances_update(tz);
>> +}
>> +
>> void thermal_zone_device_critical(struct thermal_zone_device *tz)
>> {
>> /*
>> @@ -723,6 +732,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 +792,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..e7b2a1f4bab0 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -195,6 +195,9 @@ struct thermal_zone_device {
>> * thermal zone.
>> * @throttle: callback called for every trip point even if temperature is
>> * below the trip point temperature
>> + * @instances_update: callback called when thermal zone instances list
>> + * i has changed (e.g. added new or removed), which
>> + * may help to offload work for governor like allocations
>> * @governor_list: node in thermal_governor_list (in thermal_core.c)
>> */
>> struct thermal_governor {
>> @@ -203,6 +206,7 @@ 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 (*instances_update)(struct thermal_zone_device *tz);
>
> So this could be more general I think, something like (*update_tz)(),
> and it may take an additional argument representing the type of the
> change.
I agree. I'll send next version.
There is one candidate which could instantly be added to this
update reasons list:
cooling devices weight change via sysfs [1]
Thanks for the review comments!
[1]
https://elixir.bootlin.com/linux/latest/source/drivers/thermal/thermal_sysfs.c#L959
Powered by blists - more mailing lists