| Message ID | 20231212134844.1213381-2-lukasz.luba@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add callback for cooling list update to speed-up IPA | expand |
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; > }; > > --
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; >> }; >> >> --
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.
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!
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; };
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(+)