Re: [PATCH 1/1] thermal: sysfs: avoid actual readings from sysfs

From: Daniel Lezcano
Date: Wed Jun 07 2023 - 14:23:48 EST

On 07/06/2023 18:38, Eduardo Valentin wrote:
Hey Daniel,

Thanks for taking the time to read the patch.

On Wed, Jun 07, 2023 at 11:24:21AM +0200, Daniel Lezcano wrote:



Hi Eduardo,

On 07/06/2023 02:37, Eduardo Valentin wrote:
From: Eduardo Valentin <eduval@xxxxxxxxxx>

As the thermal zone caches the current and last temperature
value, the sysfs interface can use that instead of
forcing an actual update or read from the device.

If the read fails, userspace can handle that by using the previous
value. Do we really want to hide driver dysfunctions?

Good point.

In fact I thought of this exact problem. I sent only this patch,
but it has more changes to come.

The next changes will replicate the current design of
storing last_temperature in the thermal zone to also store
the last return value, success or error, on the thermal zone
too so that we can use here at the front end to report back
to userspace when the reads are failing.
>
But yes, you are right, we do not want to keep reporting
a successful read when the thermal zone thread has been
failing to update the value, that needs to be reported
up back to userspace.

IIUC, you want the temperature to be updated only by the polling thread and when the userspace reads the temperature, it reads a cached value, is that correct ?

This way, if multiple userspace requests are coming
in, we avoid storming the device with multiple reads
and potentially clogging the timing requirement
for the governors.

Sorry, I'm not convinced :/

Can you elaborate 'the timing requirement for the governors' ? I'm
missing the point


The point is to avoid contention on the device update path.
Governor that use differential equations on temperature over time
will be very time sensitive. Step wise, power allocator, or any
PID will be very sensitive to time. So, If userspace is hitting
this API too often we can see cases where the updates needed to
service userspace may defer/delay the execution of the governor
logic.

AFAIK, reading a temperature value is usually between less than 1us and 10us (depending on the sampling configuration in the driver).

I've done some measurements to read a temperature through sysfs and netlink. It is between 2us and 7us on a platforms where reading a 900ns latency sensor, sysfs being faster.

The time sensitive governor is the power allocator and usually, the sampling period is between 100ms and 250ms.

The thermal zones with fast thermal transitions may need faster sampling period but the hardware offloads the mitigation in this case by sampling every 100*us*, IIRC.

Given that, I'm not sure we are facing a design issue with thermal framework.

Do you have a use case with some measurements to spot an issue or is it a potential issue you identified ?

Despite that, there is really no point to have more updates than
what was configured for the thermal zone to support. Say that
we configure a thermal zone to update itself every 500ms, yet
userspace keeps sending reads every 100ms, we do not need necessarily
to do a trip to the device every single time to update the temperature,
as per the design for the thermal zone.

Sorry, I do not agree. The thermal zone is configured with a monitoring sampling period to detect trip point crossing and it can be configured without sampling period at all, just basing on trip point crossing interrupt.

The userspace has the right to read the current temperature it is interested in. For instance, the 'thermometer' in tools/thermal/thermometer may want to read the temperature at a high rate in order to profile the thermal zones with/without the mitigation kicking in.

Caching the values just break the current behavior.

Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx> (supporter:THERMAL)
Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> (supporter:THERMAL)
Cc: Amit Kucheria <amitk@xxxxxxxxxx> (reviewer:THERMAL)
Cc: Zhang Rui <rui.zhang@xxxxxxxxx> (reviewer:THERMAL)
Cc: linux-pm@xxxxxxxxxxxxxxx (open list:THERMAL)
Cc: linux-kernel@xxxxxxxxxxxxxxx (open list)

Signed-off-by: Eduardo Valentin <eduval@xxxxxxxxxx>
---
drivers/thermal/thermal_sysfs.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index b6daea2398da..a240c58d9e08 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -35,12 +35,23 @@ static ssize_t
temp_show(struct device *dev, struct device_attribute *attr, char *buf)
{
struct thermal_zone_device *tz = to_thermal_zone(dev);
- int temperature, ret;
-
- ret = thermal_zone_get_temp(tz, &temperature);
+ int temperature;

- if (ret)
- return ret;
+ /*
+ * don't force new update from external reads
+ * This way we avoid messing up with time constraints.
+ */
+ if (tz->mode == THERMAL_DEVICE_DISABLED) {
+ int r;
+
+ r = thermal_zone_get_temp(tz, &temperature); /* holds tz->lock*/
+ if (r)
+ return r;
+ } else {
+ mutex_lock(&tz->lock);
+ temperature = tz->temperature;
+ mutex_unlock(&tz->lock);
+ }

No please, we are pushing since several weeks a lot of changes to
encapsulate the thermal zone device structure and prevent external core
components to use the internals directly. Even if we can consider the
thermal_sysfs as part of the core code, that changes is not sysfs related.

Can you clarify your concern, is it the direct access ? The lock ?
what is the concern?

What is your suggestion here? Do you want me to write a helper
function that gets tz->temperature without doing a ops->get_temp()?

The concern is the thermal framework code is not really in a good shape and the internals leaked around in the different drivers all around the subsystems, that led to drivers tampering with the thermal zone device structure data (including taking locks).

There are ongoing efforts to do some house cleaning around the thermal zone device structure encapsulation as well as getting ride of the different ops get_trip* by replacing that with a generic trip point structure to deal with in the thermal core code.

Those with the final objective to have the trip points ordered and handle the trip point crossing correctly detected (it is currently somehow broken).

Here the code does directly access tz->*.

Even not being convinced by the change proposal, I would have handle this value cache in the thermal_zone_get_temp() function directly in order to preserve the code self-encapsulation.


--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog