| Message ID | 20231206113138.3576492-2-lukasz.luba@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add callback for cooling list update to speed-up IPA | expand |
On Wed, Dec 6, 2023 at 12:30 PM Lukasz Luba <lukasz.luba@arm.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@arm.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? 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 >
On 12/11/23 20:41, Rafael J. Wysocki wrote: > On Wed, Dec 6, 2023 at 12:30 PM Lukasz Luba <lukasz.luba@arm.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@arm.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
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); struct list_head governor_list; };
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@arm.com> --- drivers/thermal/thermal_core.c | 14 ++++++++++++++ include/linux/thermal.h | 4 ++++ 2 files changed, 18 insertions(+)