| 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: <e6ae0cd5-2ca3-ebda-f7ec-3c71f517c924@arm.com>
Date: Thu, 26 Nov 2020 16:26:09 +0000
From: Lukasz Luba <lukasz.luba@....com>
To: Ionela Voinescu <ionela.voinescu@....com>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
daniel.lezcano@...aro.org, amitk@...nel.org,
Dietmar.Eggemann@....com
Subject: Re: [PATCH v4 2/3] thermal: power allocator: refactor sustainable
power estimation
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@....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@....com>
>
> Thank you,
> Ionela.
>
Thank you for the review.
Regards,
Lukasz
[1] https://lore.kernel.org/lkml/X7P4lA1nITo58eFT@kroah.com/
Powered by blists - more mailing lists