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;
Message-ID: <7616fd9d-aa0d-2ecd-8751-894b1c9073c0@linaro.org>
Date:   Mon, 12 Jun 2023 10:17:51 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 1/1] thermal: sysfs: avoid actual readings from sysfs
Content-Language: en-US
To:     Eduardo Valentin <evalenti@kernel.org>
Cc:     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>
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>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <ZIITZINvtPfjuhS6@uf8f119305bce5e.ant.amazon.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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.

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.

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

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.


[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