Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Jun 2023 09:38:05 -0700
From:   Eduardo Valentin <evalenti@kernel.org>
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     Eduardo Valentin <evalenti@kernel.org>, eduval@amazon.com,
        rafael@kernel.org, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>
Subject: Re: [PATCH 1/1] thermal: sysfs: avoid actual readings from sysfs
Message-ID: <ZICybSuZELhR1Ni5@uf8f119305bce5e.ant.amazon.com>
References: <20230607003721.834038-1-evalenti@kernel.org>
 <f26ac9a9-60af-a0fe-fccc-25bcd306f5a1@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <f26ac9a9-60af-a0fe-fccc-25bcd306f5a1@linaro.org>
Precedence: bulk

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@amazon.com>
> > 
> > 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.

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

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.

> 
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> (supporter:THERMAL)
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:THERMAL)
> > Cc: Amit Kucheria <amitk@kernel.org> (reviewer:THERMAL)
> > Cc: Zhang Rui <rui.zhang@intel.com> (reviewer:THERMAL)
> > Cc: linux-pm@vger.kernel.org (open list:THERMAL)
> > Cc: linux-kernel@vger.kernel.org (open list)
> > 
> > Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> > ---
> >   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()?


Let me know.

> 
> >       return sprintf(buf, "%d\n", temperature);
> >   }
> 
> --
> <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
> 

-- 
All the best,
Eduardo Valentin