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:   Tue, 20 Jun 2023 22:06:50 -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: [PATCH 1/1] thermal: sysfs: avoid actual readings from sysfs
Message-ID: <ZJKFar/U75+PGCRt@uf8f119305bce5e.ant.amazon.com>
References: <20230607003721.834038-1-evalenti@kernel.org>
 <f26ac9a9-60af-a0fe-fccc-25bcd306f5a1@linaro.org>
 <ZICybSuZELhR1Ni5@uf8f119305bce5e.ant.amazon.com>
 <b2e93db5-e6f8-a9d8-53de-af5ea750f0f0@linaro.org>
 <ZIITZINvtPfjuhS6@uf8f119305bce5e.ant.amazon.com>
 <7616fd9d-aa0d-2ecd-8751-894b1c9073c0@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <7616fd9d-aa0d-2ecd-8751-894b1c9073c0@linaro.org>
Precedence: bulk

On Mon, Jun 12, 2023 at 10:17:51AM +0200, Daniel Lezcano wrote:
> 
> 
> 
> Hi Eduardo,
> 
> On 08/06/2023 19:44, Eduardo Valentin wrote:
> 
> [ ... ]
> 
> > > Do you have a use case with some measurements to spot an issue or is it
> > > a potential issue you identified ?
> > 
> > 
> > yes, a governor that is using I2C device as input, behind I2C fast mode (100KHz)
> > and needs to update the zone every 100ms. Each read in this bus, if done alone
> > would be around 500us, takes 10bytes to read the device, it is 10 clocks per byte,
> > well technically 9, but rounding for the sake of the example, which gets you
> > 50 / 100KHz = 500 us. That is for a single read. You add one single extra
> > userspace read triggering an unused device update, that is already a 1ms drift.
> > Basically you looking at 0.5% for each extra userspace read competing in this
> > sysfs node. You add extra devices in the same I2C bus, your governor is looking
> > at more than 1% overhead. And I am talking also about a main CPU of ~800MHz.
> > I did not even include the lock overhead considered for this CPU ;-)
> > 
> > Again, this is not about controlling the DIE temperature of the CPU you
> > are running the thermal subsystem. This is about controlling
> > a target device.
> 
> Ok. The target device is on a bus which is slow and prone to contention.
> 
> This hardware is not designed to be monitored with a high precision, so
> reading the temperature at a high rate does not really make sense.

On the contrary, it needs even more precision and any extra delay adds to
loss on accuracy :-)

> 
> Moreover (putting apart a potential contention), the delayed read does
> not change the time interval, which remains the same from the governor
> point of view.

It does not change the governor update interval and that is a property of
the thermal zone. Correct. And that is the intention of the change.
The actual temperature updates driven by the governor will always
result in a driver call. While a userspace call will not be in the way
of the governor update.

Sysfs reads, However, with the current code as is, it may cause
jittering on the actual execution of the governor throttle function.
 causing the computation of the desired outcome cooling device being skewed.

> 
> In addition, i2c sensors are usually handled in the hwmon subsystem
> which are registered in the thermal framework from there. Those have
> most of their 'read' callback with a cached value in a jiffies based way
> eg. [1].

I guess what you are really saying is: go read the hwmon sysfs node,
or, hwmon solves this for us, which unfortunately is not true for all devices.


> 
> So the feature already exists for slow devices and are handled in the
> drivers directly via the hwmon subsystem.
> 
> From my POV, the feature is not needed in the thermal framework.

The fact that hwmon does it in some way is another evidence of the
actual problem. Telling that this has to be solved by another subsystem
for a sysfs node that is part of thermal subsystem does not really solve
the problem. Also as I mentioned, this is not common on all hwmon
devices, and not all I2C devices are hwmon devices. In fact, I2C
was just one example of a slow device. There are more I can quote
that are not necessarily under the hwmon case.


Not sure if you missed, but an alternative for the difference of
opinion on how this should behave is to have caching for response
of sysfs read of tz/temp  as an option/configuration. Then we let
userspace to choose which behavior it wants.

> 
> 
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/hwmon/lm95234.c#n163
> 
> 
> 
> --
> <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