linux-kernel - Re: [PATCH v1 4/4] thermal: gov_bang_bang: Use governor

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJZ5v0jZvOiJMGEt8FwOztQfjfBTGqdrt8_oWug+GeZ4DR6EBw@mail.gmail.com>
Date: Thu, 15 Aug 2024 14:35:18 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: "Zhang, Rui" <rui.zhang@...el.com>
Cc: "rafael@...nel.org" <rafael@...nel.org>, "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, 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.