| 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: <d1565986-77d1-1e63-f8b5-05027460c263@arm.com>
Date: Fri, 9 Oct 2020 12:57:22 +0100
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 v2 2/2] thermal: power allocator: change how estimation
code is called
Hi Ionela,
On 10/9/20 12:19 PM, Ionela Voinescu wrote:
> Hi Lukasz,
>
> On Thursday 08 Oct 2020 at 18:04:26 (+0100), Lukasz Luba wrote:
>> The sustainable power value might come from the Device Tree or can be
>> estimated in run time. There is no need to estimate 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. Re-invoke the estimation only in case the
>> sustainable power was set to 0. Apart from that the PID coefficients
>> are not going to be force updated thus can better handle sysfs settings.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@....com>
>> ---
>> drivers/thermal/gov_power_allocator.c | 56 +++++++++++++--------------
>> 1 file changed, 26 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
>> index aa35aa6c561c..1ad8d9c2685f 100644
>> --- a/drivers/thermal/gov_power_allocator.c
>> +++ b/drivers/thermal/gov_power_allocator.c
>> @@ -96,6 +96,9 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
>> if (instance->trip != params->trip_max_desired_temperature)
>> continue;
>>
>> + if (!cdev_is_power_actor(cdev))
>> + continue;
>> +
>> if (cdev->ops->state2power(cdev, tz, instance->upper,
>> &min_power))
>> continue;
>> @@ -109,31 +112,28 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
>> /**
>> * estimate_pid_constants() - Estimate the constants for the PID controller
> ^^^^^^^^^^^^^^^^^^^^^^
> estimate_tzp_constants()?
>
> When called in pid_controller() it feels strange that we check for
> sustainable_power, then we call estimate_pid_constants() and then we
> magically have an non-zero sustainable_power. Therefore, it would be
> good to change the name to indicate it's not only the PID constants that
> are estimated.
I agree, I will rename it.
>
>> * @tz: thermal zone for which to estimate the constants
>> - * @sustainable_power: sustainable power for the thermal zone
>> * @trip_switch_on: trip point number for the switch on temperature
>> * @control_temp: target temperature for the power allocator governor
>> - * @force: whether to force the update of the constants
>> *
>> * This function is used to update the estimation of the PID
>> * controller constants in struct thermal_zone_parameters.
>
> How about replacing this with:
>
> """
> * This function is used to estimate the sustainable power and PID controller
> * constants in struct thermal_zone_parameters. These estimations will then be
> * available in sysfs.
> """
Yes, that change is required since the function name will also change.
>
>> - * 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
>> - * in thermal zone's parameters are overwritten.
>> + * Sustainable power is going to be estimated in case it is 0.
>> */
>> static void estimate_pid_constants(struct thermal_zone_device *tz,
>> - u32 sustainable_power, int trip_switch_on,
>> - int control_temp, bool force)
>> + int trip_switch_on, int control_temp)
>> {
>> - int ret;
>> - int switch_on_temp;
>> + u32 sustainable_power = tz->tzp->sustainable_power;
>> u32 temperature_threshold;
>> + int switch_on_temp;
>> + int ret;
>> s32 k_i;
>>
>> + if (!sustainable_power) {
>> + sustainable_power = estimate_sustainable_power(tz);
>> + /* Make the estimation available in sysfs */
>
> I would remove this comment from here. The reason is that this is not a
> special case. This will happen for all the tzp parameters set below.
> That's why I suggested adding this to the overall function comment above.
Yes, function comment will handle this better.
>
>> + tz->tzp->sustainable_power = sustainable_power;
>> + }
>> +
>> ret = tz->ops->get_trip_temp(tz, trip_switch_on, &switch_on_temp);
>> if (ret)
>> switch_on_temp = 0;
>> @@ -150,15 +150,15 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
>> if (!temperature_threshold)
>> return;
>>
>> - if (!tz->tzp->k_po || force)
>> + if (!tz->tzp->k_po)
>> tz->tzp->k_po = int_to_frac(sustainable_power) /
>> temperature_threshold;
>>
>> - if (!tz->tzp->k_pu || force)
>> + if (!tz->tzp->k_pu)
>> tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
>> temperature_threshold;
>>
>> - if (!tz->tzp->k_i || force) {
>> + if (!tz->tzp->k_i) {
>> k_i = tz->tzp->k_pu / 10;
>> tz->tzp->k_i = k_i > 0 ? k_i : 1;
>> }
>
> (Possibly judgement call)
>
> I agree we don't need the force argument to this function, but I would
> still keep an internal force variable (default false) to be set to true
> when we estimate and set the sustainable power.
Yes, make sense, I will set locally.
>
> The reason for this is that there is no guarantee that when
> sustainable_power is found to be 0 and estimated, we'll then find all of
> the PID constants 0 as well in order to set them to a sane default.
> Basically my worry is that we'll end up with a combination of PID
> constants and sustainable power (some estimated and some not) that is not
> quite sane.
>
> But I understand a potential usecase in which a user might want to set
> it's own PID constants while wanting an estimated sustainable_power.
> But for this do you think it might be worth just having a pr_info
> message saying that "Sustainable power is 0; will estimate sustainable
> power and PID constants."? For this the user would only have to know
> that they need to set the sustainable_power to 0 first and then
> populate its own PID constants if they want to.
Yes, the print message is good in this case.
>
>> @@ -198,14 +198,11 @@ 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);
>> - }
>> + if (!tz->tzp->sustainable_power)
>> + estimate_pid_constants(tz, params->trip_switch_on,
>> + control_temp);
>> +
>> + sustainable_power = tz->tzp->sustainable_power;
>>
>
> (Nit)
>
> This is only used once below in:
> power_range = sustainable_power + frac_to_int(power_range);
>
> I think we can use tz->tzp->sustainable_power directly there and
> completely remove sustainable_power.
I had the feeling that it won't fit in 80line there but it does.
I will change it.
Thank you for the review.
Lukasz
>
> Thank you,
> Ionela.
>
>> err = control_temp - tz->temperature;
>> err = int_to_frac(err);
>> @@ -603,20 +600,19 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
>>
>> get_governor_trips(tz, params);
>>
>> + tz->governor_data = params;
>> +
>> if (tz->trips > 0) {
>> ret = tz->ops->get_trip_temp(tz,
>> params->trip_max_desired_temperature,
>> &control_temp);
>> if (!ret)
>> - estimate_pid_constants(tz, tz->tzp->sustainable_power,
>> - params->trip_switch_on,
>> - control_temp, false);
>> + estimate_pid_constants(tz, params->trip_switch_on,
>> + control_temp);
>> }
>>
>> reset_pid_controller(params);
>>
>> - tz->governor_data = params;
>> -
>> return 0;
>>
>> free_params:
>> --
>> 2.17.1
>>
Powered by blists - more mailing lists