| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ba0d605b90ca91f36697f86f15e09e229ac82465.camel@intel.com>
Date: Fri, 16 Aug 2024 02:59:20 +0000
From: "Zhang, Rui" <rui.zhang@...el.com>
To: "rafael@...nel.org" <rafael@...nel.org>
CC: "lukasz.luba@....com" <lukasz.luba@....com>, "linux-pm@...r.kernel.org"
<linux-pm@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "daniel.lezcano@...aro.org"
<daniel.lezcano@...aro.org>, "rjw@...ysocki.net" <rjw@...ysocki.net>,
"peter@...e.net" <peter@...e.net>
Subject: Re: [PATCH v1 4/4] thermal: gov_bang_bang: Use governor_data to
reduce overhead
On Thu, 2024-08-15 at 14:35 +0200, Rafael J. Wysocki wrote:
> On Thu, Aug 15, 2024 at 5:26 AM Zhang, Rui <rui.zhang@...el.com>
> wrote:
> >
> > On Wed, 2024-08-14 at 19:26 +0200, Rafael J. Wysocki wrote:
> > > On Wed, Aug 14, 2024 at 8:09 AM Zhang, Rui <rui.zhang@...el.com>
> > > wrote:
> > > >
> > > > On Tue, 2024-08-13 at 16:29 +0200, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > > >
> > > > > After running once, the for_each_trip_desc() loop in
> > > > > bang_bang_manage() is pure needless overhead because it is
> > > > > not
> > > > > going
> > > > > to
> > > > > make any changes unless a new cooling device has been bound
> > > > > to
> > > > > one of
> > > > > the trips in the thermal zone or the system is resuming from
> > > > > sleep.
> > > > >
> > > > > For this reason, make bang_bang_manage() set governor_data
> > > > > for
> > > > > the
> > > > > thermal zone and check it upfront to decide whether or not it
> > > > > needs
> > > > > to
> > > > > do anything.
> > > > >
> > > > > However, governor_data needs to be reset in some cases to let
> > > > > bang_bang_manage() know that it should walk the trips again,
> > > > > so
> > > > > add
> > > > > an
> > > > > .update_tz() callback to the governor and make the core
> > > > > additionally
> > > > > invoke it during system resume.
> > > > >
> > > > > To avoid affecting the other users of that callback
> > > > > unnecessarily,
> > > > > add
> > > > > a special notification reason for system resume,
> > > > > THERMAL_TZ_RESUME,
> > > > > and
> > > > > also pass it to __thermal_zone_device_update() called during
> > > > > system
> > > > > resume for consistency.
> > > > >
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > > > ---
> > > > > drivers/thermal/gov_bang_bang.c | 18 ++++++++++++++++++
> > > > > drivers/thermal/thermal_core.c | 3 ++-
> > > > > include/linux/thermal.h | 1 +
> > > > > 3 files changed, 21 insertions(+), 1 deletion(-)
> > > > >
> > > > > Index: linux-pm/drivers/thermal/gov_bang_bang.c
> > > > > =============================================================
> > > > > ====
> > > > > ==
> > > > > --- linux-pm.orig/drivers/thermal/gov_bang_bang.c
> > > > > +++ linux-pm/drivers/thermal/gov_bang_bang.c
> > > > > @@ -86,6 +86,10 @@ static void bang_bang_manage(struct ther
> > > > > const struct thermal_trip_desc *td;
> > > > > struct thermal_instance *instance;
> > > > >
> > > > > + /* If the code below has run already, nothing needs
> > > > > to be
> > > > > done. */
> > > > > + if (tz->governor_data)
> > > > > + return;
> > > > > +
> > > > > for_each_trip_desc(tz, td) {
> > > > > const struct thermal_trip *trip = &td->trip;
> > > > >
> > > > > @@ -107,11 +111,25 @@ static void bang_bang_manage(struct
> > > > > ther
> > > > >
> > > > > bang_bang_set_instance_target(instanc
> > > > > e, 0);
> > > > > }
> > > > > }
> > > > > +
> > > > > + tz->governor_data = (void *)true;
> > > > > +}
> > > > > +
> > > > > +static void bang_bang_update_tz(struct thermal_zone_device
> > > > > *tz,
> > > > > + enum thermal_notify_event
> > > > > reason)
> > > > > +{
> > > > > + /*
> > > > > + * Let bang_bang_manage() know that it needs to walk
> > > > > trips
> > > > > after binding
> > > > > + * a new cdev and after system resume.
> > > > > + */
> > > > > + if (reason == THERMAL_TZ_BIND_CDEV || reason ==
> > > > > THERMAL_TZ_RESUME)
> > > > > + tz->governor_data = NULL;
> > > > > }
> > > >
> > > > can we do the cdev initialization for BIND_CDEV and RESUME
> > > > notification
> > > > in .update_tz() directly?
> > >
> > > That would be viable if the zone temperature was known at the
> > > time
> > > .update_tz() runs, but it isn't. See this message:
> > >
> > > https://lore.kernel.org/linux-pm/CAJZ5v0ji_7Z-24iCO_Xxu4Zm4jgVFmR9jVp8QNiCOxzV9gqSnA@mail.gmail.com/
> > >
> > > As long as the zone temperature is not known, it is not clear
> > > which
> > > way to initialize the cooling devices.
> >
> > right. Then the patch LGTM.
>
> Great!
>
> > BTW, what do you think if we add handling for first temperature
> > read in
> > handle_thermal_trip(), say, some draft code like below,
> >
> > if (tz->last_temperature == THERMAL_TEMP_INIT) {
> > if (tz->temperature < trip->temperature)
> > list_add(&td->notify_list_node, way_down_list);
> > else
> > list_add_tail(&td->notify_list_node, way_up_list);
> > }
> >
> > This should handle both the init and the resume case.
>
> I have considered doing something similar, but there are quite a few
> arguments against it.
>
> First, it would cause spurious notifications to be sent to user
> space.
> Second, the debug code would need to be modified to take this case
> into account explicitly. Moreover, this would be extra overhead for
> the other governors.
>
> IMV it's better to limit the impact to the Bang-bang governor where
> the problem is.
Agreed.
Reviewed-by: Zhang Rui <rui.zhang@...el.com>
thanks,
rui
Powered by blists - more mailing lists