Received-SPF: pass (google.com: domain of linux-kernel+bounces-180810-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <13518388.uLZWGnKmhe@kreacher> <3304112.44csPzL39Z@kreacher> <39e15eef-f7fd-4e16-bc74-7f1c6820fe6a@arm.com>
In-Reply-To: <39e15eef-f7fd-4e16-bc74-7f1c6820fe6a@arm.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Thu, 16 May 2024 11:04:25 +0200
Message-ID: <CAJZ5v0gZJE6jfa8_9LgDdjYotY+crLH1JJXHdAWREPz4SJ305A@mail.gmail.com>
Subject: Re: [PATCH v1 1/6] thermal: sysfs: Trigger zone temperature updates
 on sysfs reads
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>, LKML <linux-kernel@vger.kernel.org>, 
	Linux PM <linux-pm@vger.kernel.org>, "Rafael J. Wysocki" <rafael@kernel.org>, 
	Daniel Lezcano <daniel.lezcano@linaro.org>, 
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>, Zhang Rui <rui.zhang@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Lukasz,

On Mon, May 13, 2024 at 9:11=E2=80=AFAM Lukasz Luba <lukasz.luba@arm.com> w=
rote:
>
> Hi Rafael,
>
> On 5/10/24 15:13, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Reading the zone temperature via sysfs causes the driver callback to
> > be invoked, but it does not cause the thermal zone object to be updated=
.
> >
> > This is problematic if the zone temperature read via sysfs differs from
> > the temperature value stored in the thermal zone object as it may cause
> > the kernel and user space to act against each other in some cases.
> >
> > For this reason, make temp_show() trigger a zone temperature update if
> > the temperature returned by thermal_zone_get_temp() is different from
> > the temperature value stored in the thermal zone object.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/thermal/thermal_core.c  |    2 +-
> >   drivers/thermal/thermal_sysfs.c |    3 +++
> >   2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_sysfs.c
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> > +++ linux-pm/drivers/thermal/thermal_sysfs.c
> > @@ -42,6 +42,9 @@ temp_show(struct device *dev, struct dev
> >       if (ret)
> >               return ret;
> >
> > +     if (temperature !=3D READ_ONCE(tz->temperature))
> > +             thermal_zone_device_update(tz, THERMAL_EVENT_TEMP_SAMPLE)=
;
>
> That's a bit problematic because it will trigger
> governor->manage()
>
> In case of IPA governor we relay on constant polling
> period. We estimate the past power usage and current
> thermal budget, to derive the next period power budget
> for devices. I don't know if the internal PID algorithm
> will be resilient enough to compensate this asynchronous
> trigger caused from user-space.
>
> We choose the period to be at least 1 frame (e.g. ~16ms)
> to have good avg usage of CPUs and GPU. TBH I don't know
> what would happen if someone reads the temp after e.g. 1ms
> of last IPA trigger, but some devices (e.g. GPU) wasn't
> loaded in that last 1ms delta...
> I'm a bit more relaxed about CPUs because we use utilization
> signal from runqueues (like the TEO util gov). That's a moving
> avg signal which should keep some history, like low-pass
> filter, so information is more resilient in that case.
>
> Could we think about that IPA constant period usage?
> I think I understand the proposal of your patch.
> We might add a filter inside IPA to ignore such async
> triggers in the .manage() callback.
> What do you think?

Thanks for pointing this out.

Actually, the target audience for this change are thermal zones that
are not updated on a regular basis, so what about storing the last
zone temperature update time in the zone object and only running an
update from temp_show() if sufficient time has elapsed since the last
update (say 1 sec)?