Re: [PATCH v2 0/7] Add thermal thresholds support

["Pandruvada, Srinivas" <srinivas.pandruvada@intel.com>]

On Wed, 2024-08-21 at 22:20 +0200, Rafael J. Wysocki wrote:
> Hi Srinivas,
> 
> On Wed, Aug 21, 2024 at 10:04 PM Pandruvada, Srinivas
> <srinivas.pandruvada@intel.com> wrote:
> > 
> > Hi Daniel,
> > 
> > On Fri, 2024-08-16 at 10:12 +0200, Daniel Lezcano wrote:
> > > The trip points are a firmware description of the temperature
> > > limits
> > > of a specific thermal zone where we associate an action which is
> > > done
> > > by the kernel. The time resolution is low.
> > > 
> > > The userspace has to deal with a more complex thermal management
> > > based
> > > on heuristics from different information coming from different
> > > places. The logic is much more complex but based on a bigger time
> > > resolution, usually one second based.
> > > 
> > > The purpose of the userspace is to monitor the temperatures from
> > > different places and take actions. However, it can not be
> > > constantly
> > > reading the temperature to detect when a temperature threshold
> > > has
> > > been reached. This is especially bad for mobile or embedded
> > > system as
> > > that will lead to an unacceptable number of wakeup to check the
> > > temperature with nothing to do.
> > > 
> > > On the other side, the sensors are now most of the time interrupt
> > > driven. That means the thermal framework will use the temperature
> > > trip
> > > points to program the sensor to trigger an interrupt when a
> > > temperature limit is crossed.
> > > 
> > > Unfortunately, the userspace can not benefit this feature and
> > > current
> > > solutions found here and there, iow out-of-tree, are to add fake
> > > trip
> > > points in the firmware and enable the writable trip points.
> > > 
> > > This is bad for different reasons, the trip points are for in-
> > > kernel
> > > actions, the semantic of their types is used by the thermal
> > > framework
> > > and by adding trip points in the device tree is a way to overcome
> > > the
> > > current limitation but tampering with how the thermal framework
> > > is
> > > supposed to work. The writable trip points is a way to adjust a
> > > temperature limit given a specific platform if the firmware is
> > > not
> > > accurate enough and TBH it is more a debug feature from my POV.
> > > 
> > > The thresholds mechanism is a way to have the userspace to tell
> > > thermal framework to send a notification when a temperature limit
> > > is
> > > crossed. There is no id, no hysteresis, just the temperature and
> > > the
> > > direction of the limit crossing. That means we can be notified
> > > when a
> > > threshold is crossed the way up only, or the way down only or
> > > both
> > > ways. That allows to create hysteresis values if it is needed.
> > > 
> > > A threshold can be added, deleted or flushed. The latter means
> > > all
> > > thresholds belonging to a thermal zone will be deleted.
> > > 
> > 
> > So you are proposing to add threshold via netlink, not adding any
> > new
> > sysfs attribute? That is not clear here.
> > 
> > I think you are adding"
> > THERMAL_GENL_CMD_THRESHOLD_GET
> > THERMAL_GENL_CMD_THRESHOLD_ADD
> > THERMAL_GENL_CMD_THRESHOLD_DELETE
> > THERMAL_GENL_CMD_THRESHOLD_FLUSH
> > 
> > We need to document our netlink messages including old ones.
> > 
> > Also we should add "MODIFY" as we tend to change them quite often.
> > 
> > Also no hysteresis, that is practically we can't use.
> 
> The direction thing is equivalent to hysteresis though.
> 
> Instead of using one threshold with a given hysteresis value, use two
> of them with different temperature values and different directions.
> 
> > Temperature changes so much that that will flood user space. You
> > will get 100s of
> > events on CPU temperature if you set temperature threshold in CPU.
> 
> Events only trigger when thresholds are crossed in a specific
> direction, but overall you have a point.

This is good enough for some sensor which changes slowly like skin. But
some sensors like CPU and board are not like that. We publish both raw
and filtered event count in debugfs for x86 package temp.
For example, I just ran a sample on a client for one threshold.

Total notification from hardware: 224
Notified to user space: 16



> 
> > We have a whole filtering in driver to avoid this.
> > You need a rate limit here.
> > 
> > There are multiple user processes can add threshold and there is no
> > ownership. So one process can cause too much noise to others as it
> > is
> > multicast.
> > 
> > We worked on a change to filer these as discussed during last LPC,
> > but
> > not posted yet. This will really need this as this will be too many
> > messages.
> 
> Unless there is a way to limit your subscription to events regarding
> a
> specific thermal zone, for instance.
That's what this change does.

> 
> Anyway, I have to admit ignorance regarding the user space usage
> model
> related to this.  For example, is it expected that there will be one
> user space entity managing the thresholds or there can be many.

Good question. That's why we didn't send this change. If there is one
agent this is fine.

But in embedded space, there may be more than one. Hence they needed
netlink multicast instead of just one char device.

Thanks,
Srinivas