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

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

On Thu, 2024-08-22 at 12:11 +0000, Pandruvada, Srinivas wrote:
> On Thu, 2024-08-22 at 11:41 +0200, Rafael J. Wysocki wrote:
> > On Thu, Aug 22, 2024 at 12:16 AM Pandruvada, Srinivas
> > <srinivas.pandruvada@intel.com> wrote:
> > > 
> > > 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
> > > > > > whichcan't
> > > > > > 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.
> > 
> > Right, and I think that this is the driving use case.
> > 
> > > But some sensors like CPU and board are not like that.
> > 
> > Sure, so this interface will not be for them.
> But when we design an interface, I think this shouldn't be sensor
> dependent.
> 
> > 
> > > 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
> > 
> > So did you apply the Daniel's patches and run the test or did you
> > do
> > something else?
> We already use netlink to send notification to user space via
> writable
> trip. So didn't apply any patches. I don't see these patches will do
> any different.

I mean without rate limit here all 224 notifications will be passed to
user space (not 16). Temperature goes up and down over threshold for
100s of times before settling.

I can give  a try with this series to confirm.

Thanks,
Srinivas

> 
> 
> > 
> > > > 
> > > > > 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.
> > 
> > I think that the addition and deletion of a user threshold should
> > be
> > privileged operations.
> > 
> > If that is the case, it all boils down to proper coordination in
> > user
> > space.
> > 
> Some kernel API use some cookie as parameter. Here also something
> like
> that can be used to match.
> 
> > > But in embedded space, there may be more than one. Hence they
> > > needed
> > > netlink multicast instead of just one char device.
> > 
> > I see.
> 
> Thanks,
> Srinivas
>