Message-ID: <CAJZ5v0icxkRyd_1T53JmX3XDQOC6_Ak9nOD65Yx-rhZbDa4Y_w@mail.gmail.com>
Date: Wed, 14 Aug 2024 19:26:26 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: "Zhang, Rui" <rui.zhang@...el.com>
Cc: "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>, "rjw@...ysocki.net" <rjw@...ysocki.net>, 
	"lukasz.luba@....com" <lukasz.luba@....com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>, "peter@...e.net" <peter@...e.net>
Subject: Re: [PATCH v1 4/4] thermal: gov_bang_bang: Use governor_data to
 reduce overhead

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.

Interestingly enough, the zone temperature is first checked by the
core when the zone is enabled and not when it is registered.

> Then we don't need .manage() callback. This makes more sense to me
> because bang_bang governor cares about trip point crossing only.

That's true, but this is all about a corner case in which no trip
points are crossed and the cooling devices need to be initialized
properly regardless.

> >  static struct thermal_governor thermal_gov_bang_bang = {
> >         .name           = "bang_bang",
> >         .trip_crossed   = bang_bang_control,
> >         .manage         = bang_bang_manage,
> > +       .update_tz      = bang_bang_update_tz,
> >  };
> >  THERMAL_GOVERNOR_DECLARE(thermal_gov_bang_bang);
> > Index: linux-pm/drivers/thermal/thermal_core.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.c
> > +++ linux-pm/drivers/thermal/thermal_core.c
> > @@ -1692,7 +1692,8 @@ static void thermal_zone_device_resume(s
> >
> >         thermal_debug_tz_resume(tz);
> >         thermal_zone_device_init(tz);
> > -       __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> > +       thermal_governor_update_tz(tz, THERMAL_TZ_RESUME);
> > +       __thermal_zone_device_update(tz, THERMAL_TZ_RESUME);
> >
> >         complete(&tz->resume);
> >         tz->resuming = false;
> > Index: linux-pm/include/linux/thermal.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/thermal.h
> > +++ linux-pm/include/linux/thermal.h
> > @@ -55,6 +55,7 @@ enum thermal_notify_event {
> >         THERMAL_TZ_BIND_CDEV, /* Cooling dev is bind to the thermal
> > zone */
> >         THERMAL_TZ_UNBIND_CDEV, /* Cooling dev is unbind from the
> > thermal zone */
> >         THERMAL_INSTANCE_WEIGHT_CHANGED, /* Thermal instance weight
> > changed */
> > +       THERMAL_TZ_RESUME, /* Thermal zone is resuming after system
> > sleep */
> >  };
> >
> >  /**
> >
> >
> >
>

Powered by blists - more mailing lists