Messages in this thread | | | Subject | Re: [PATCH v4 2/3] thermal: power allocator: refactor sustainable power estimation | From | Lukasz Luba <> | Date | Thu, 26 Nov 2020 16:26:09 +0000 |
| |
Hi Ionela,
On 11/26/20 3:59 PM, Ionela Voinescu wrote: > Hey, > > Mostly trivial nits (added in case you want to consider them for this code > or future changes). > > On Tuesday 24 Nov 2020 at 16:10:24 (+0000), Lukasz Luba wrote: >> The sustainable power value might come from the Device Tree or can be >> estimated in run time. The sustainable power might be updated by the user >> via sysfs interface, which should trigger new estimation of PID >> coefficients. There is no need to estimate it every time when the >> governor is called and temperature is high. Instead, store the estimated >> value and make it available via standard sysfs interface, so it can be >> checked from the user-space. >> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> --- >> drivers/thermal/gov_power_allocator.c | 52 ++++++++++++++++++++------- >> 1 file changed, 40 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c >> index 2e20085ed217..d7e4b9f6af60 100644 >> --- a/drivers/thermal/gov_power_allocator.c >> +++ b/drivers/thermal/gov_power_allocator.c >> @@ -63,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y) >> * @trip_max_desired_temperature: last passive trip point of the thermal >> * zone. The temperature we are >> * controlling for. >> + * @sustainable_power: Sustainable power (heat) that this thermal zone can >> + * dissipate >> */ >> struct power_allocator_params { >> bool allocated_tzp; >> @@ -70,6 +72,7 @@ struct power_allocator_params { >> s32 prev_err; >> int trip_switch_on; >> int trip_max_desired_temperature; >> + u32 sustainable_power; >> }; >> >> /** >> @@ -118,10 +121,6 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz) >> * >> * This function is used to update the estimation of the PID >> * controller constants in struct thermal_zone_parameters. >> - * Sustainable power is provided in case it was estimated. The >> - * estimated sustainable_power should not be stored in the >> - * thermal_zone_parameters so it has to be passed explicitly to this >> - * function. >> * >> * If @force is not set, the values in the thermal zone's parameters >> * are preserved if they are not zero. If @force is set, the values >> @@ -171,6 +170,42 @@ static void estimate_pid_constants(struct thermal_zone_device *tz, >> */ >> } >> >> +/** >> + * get_sustainable_power() - Get the right sustainable power > ^^^^^^^^^ > Nit: I would not say there is a right sustainable power. I would remove > this.
I meant the 'right' at that moment in time (because value can change).
> >> + * @tz: thermal zone for which to estimate the constants >> + * @params: parameters for the power allocator governor >> + * @control_temp: target temperature for the power allocator governor >> + * >> + * This function is used for getting the proper sustainable power value based >> + * on variables which might be updated by the user sysfs interface. If that > ^^ > through >> + * happen the new value is going to be estimated and updated. It is also used > > Nit: "If that happens, the new.." > >> + * after thermal zone binding, where the initial values where set to 0. > ^^^^^^^^^^^^^^^^^^^^^ > value could be 0.
I meant many variables in the struct which was kzalloc'ed
>> + */ > > Nit: I think the code is self explanatory so you might not need to go > into so many details in the description. > >> +static u32 get_sustainable_power(struct thermal_zone_device *tz, >> + struct power_allocator_params *params, >> + int control_temp) >> +{ >> + u32 sustainable_power; >> + > > Given that we call this every time the controller kicks in, it might > help to add unlikely to both conditions. I think the most likely > scenario is for our stored params->sustainable_power and > tz->tzp->sustainable_power to match.
No, because it returned recently and Greg was explicit [1]:
'Unless you can benchmark the benifit of using likely/unlikely, do not use it, as the compiler/CPU will do it better for you.'
> >> + if (!tz->tzp->sustainable_power) >> + sustainable_power = estimate_sustainable_power(tz); >> + else >> + sustainable_power = tz->tzp->sustainable_power; >> + >> + /* Check if it's init value 0 or there was update via sysfs */ >> + if (sustainable_power != params->sustainable_power) { >> + estimate_pid_constants(tz, sustainable_power, >> + params->trip_switch_on, control_temp, >> + true); >> + >> + /* Do the estimation only once and make available in sysfs */ >> + tz->tzp->sustainable_power = sustainable_power; >> + params->sustainable_power = sustainable_power; >> + } >> + >> + return sustainable_power; >> +} >> + >> /** >> * pid_controller() - PID controller >> * @tz: thermal zone we are operating in >> @@ -200,14 +235,7 @@ static u32 pid_controller(struct thermal_zone_device *tz, >> >> max_power_frac = int_to_frac(max_allocatable_power); >> >> - if (tz->tzp->sustainable_power) { >> - sustainable_power = tz->tzp->sustainable_power; >> - } else { >> - sustainable_power = estimate_sustainable_power(tz); >> - estimate_pid_constants(tz, sustainable_power, >> - params->trip_switch_on, control_temp, >> - true); >> - } >> + sustainable_power = get_sustainable_power(tz, params, control_temp); >> >> err = control_temp - tz->temperature; >> err = int_to_frac(err); >> -- > > The logic seems sane so: > > Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com> > > Thank you, > Ionela. >
Thank you for the review.
Regards, Lukasz
[1] https://lore.kernel.org/lkml/X7P4lA1nITo58eFT@kroah.com/
| |