Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Lukasz Luba]

Hi Rafael,


On 6/20/23 11:07, Rafael J. Wysocki wrote:
> On Tue, Jun 20, 2023 at 11:46 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > On Mon, Jun 19, 2023 at 8:36 AM Di Shen <di.shen@xxxxxxxxxx> wrote:
> > > When the thermal trip point is changed, the governor should
> > > be reset so that the policy algorithm be updated to adapt to the
> > > new trip point.
> > >
> > > This patch adds an ops for thermal the governor structure to reset
> > > the governor. The ops is called when the trip point is changed.
> > > For power allocator, the parameters of pid controller and the states
> > > of power cooling devices can be reset when the passive trip point
> > > is changed.
> > >
> > > Signed-off-by: Di Shen <di.shen@xxxxxxxxxx>
> > >
> > > ---
> > > V4:
> > > - Compared to V3, handle it in thermal core instead of in governor.
> > >
> > > - Add an ops to the governor structure, and call it when a trip
> > >    point is changed.
> > >
> > > - Define reset ops for power allocator.
> > >
> > > V3:
> > > - Add fix tag.
> > >
> > > V2:
> > > - Compared to v1, do not revert.
> > >
> > > - Add a variable(last_switch_on_temp) in power_allocator_params
> > >    to record the last switch_on_temp value.
> > >
> > > - Adds a function to renew the update flag and update the
> > >    last_switch_on_temp when thermal trips are writable.
> > >
> > > V1:
> > > - Revert commit 0952177f2a1f.
> > > ---
> > > ---
> > >   drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
> > >   drivers/thermal/thermal_trip.c        |  6 ++++++
> > >   include/linux/thermal.h               |  1 +
> > >   3 files changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> > > index 8642f1096b91..41d155adc616 100644
> > > --- a/drivers/thermal/gov_power_allocator.c
> > > +++ b/drivers/thermal/gov_power_allocator.c
> > > @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> > >          return allocate_power(tz, trip.temperature);
> > >   }
> > >
> > > +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
> > > +{
> > > +       int ret = 0;
> > > +       struct thermal_trip trip;
> > > +       struct power_allocator_params *params = tz->governor_data;
> > > +
> > > +       ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       /* Only need reset for passive trips */
> > > +       if (trip.type != THERMAL_TRIP_PASSIVE)
> > > +               return -EINVAL;
> > > +
> > > +       reset_pid_controller(params);
> > > +       allow_maximum_power(tz, true);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > >   static struct thermal_governor thermal_gov_power_allocator = {
> > >          .name           = "power_allocator",
> > >          .bind_to_tz     = power_allocator_bind,
> > >          .unbind_from_tz = power_allocator_unbind,
> > >          .throttle       = power_allocator_throttle,
> > > +       .reset          = power_allocator_reset,
> > >   };
> > >   THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> > > diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> > > index 907f3a4d7bc8..52eb768fada8 100644
> > > --- a/drivers/thermal/thermal_trip.c
> > > +++ b/drivers/thermal/thermal_trip.c
> > > @@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> > >          if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
> > >                  tz->trips[trip_id] = *trip;
> > >
> > > +       if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
> > > +               ret = tz->governor->reset(tz, trip_id);
> > > +               if (ret)
> > > +                       pr_warn_once("Failed to reset thermal governor\n");
> >
> > I'm not really sure if it is useful to print this message here.
> >
> > First off, the governors may print more precise diagnostic messages if
> > they care.
> >
> > Second, what is the sysadmin supposed to do in response to this message?
>
> In addition to the above, trip point temperatures may be updated in
> other places too, for instance in response to notifications from
> platform firmware and IMV this new callback should be also used in
> those cases.  However, in those cases multiple trip points may change
> at a time and the critical/hot trip point temperatures may be updated
> too AFAICS.

IIRC the critical/hot trip points are handled differently, not using the
governors. The governors' 'throttle' callback would be called only
after we pass the test of 'critical/hot' [1].

What Di is facing is in the issue under the bucket of
'handle_non_critical_trips()' when the governor just tries to
work on stale data - old trip temp.

For the 2nd case IIUC the code, we pass the 'trip.temperature'
and should be ready for what you said (modification of that value).
Furthermore, the critical/hot situation is handled w/o governor
assistance, so we should be safe:
tz->ops->critical(tz) or tz->ops->hot(tz) and not
tz->governor->throttle(tz, trip)

Would you agree Rafael?

Regards,
Lukasz

[1] 
https://elixir.bootlin.com/linux/v6.3/source/drivers/thermal/thermal_core.c#L370