Re: [PATCH 1/5] thermal: core: Add callback for governors with cooling instances change

[Rafael J. Wysocki]

On Wed, Dec 6, 2023 at 12:30 PM Lukasz Luba <lukasz.luba@xxxxxxx> 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@xxxxxxx>

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?

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.

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

>         struct list_head        governor_list;
>  };
>
> --
> 2.25.1
>