| Message ID | 20231212134844.1213381-7-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| 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: > > 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; > } > --
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; >> } >> --
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; }
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(+)