linux-kernel - Re: [PATCH v2 4/4] powercap/drivers/dtpm: Add CPU energy model based support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6c9db999-0846-cd42-a061-c638197b5661@linaro.org>
Date:   Tue, 17 Nov 2020 18:44:42 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Lukasz Luba <lukasz.luba@....com>
Cc:     rjw@...ysocki.net, ilina@...eaurora.org, ulf.hansson@...aro.org,
        linux-pm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, rkumbako@...eaurora.org,
        rui.zhang@...el.com
Subject: Re: [PATCH v2 4/4] powercap/drivers/dtpm: Add CPU energy model based
 support

On 17/11/2020 14:15, Lukasz Luba wrote:
> Hi Daniel,
> 
> Only one small comment regarding the setup of 'power_limit'.
> 
> On 11/16/20 3:26 PM, Daniel Lezcano wrote:
>> With the powercap dtpm controller, we are able to plug devices with
>> power limitation features in the tree.
>>
>> The following patch introduces the CPU power limitation based on the
>> energy model and the performance states.
>>
>> The power limitation is done at the performance domain level. If some
>> CPUs are unplugged, the corresponding power will be subtracted from
>> the performance domain total power.
>>
>> It is up to the platform to initialize the dtpm tree and add the CPU.
>>

[ ... ]

>> +
>> +    dtpm = per_cpu(dtpm_per_cpu, cpu);
>> +    if (dtpm)
>> +        return power_add(dtpm, pd);
> 
> The dtpm->power_limit is not incremented in this path, when a new
> CPU joins the cluster.
> Is it correct?

Yes, you are right, there is something missing here. It does not change
the behavior of the power capping, but the value will be inconsistent in
the tree.

> Or maybe we need something like:
> ------------------------------>8---------------------
>         if (dtpm) {
>                 ret = power_add(dtpm, pd);
>                 if (!ret)
>                         dtpm->power_limit = dtpm->power_max;
>                 return ret;
>         }
> ------------------------8<---------------
> 
> The power_max should be updated after successful power_add().
> It would disturb user set value in power_limit, though (described
> below).
> 
> 
>> +
>> +    dtpm = dtpm_alloc();
>> +    if (!dtpm)
>> +        return -EINVAL;
>> +
>> +    dtpm_cpu = kzalloc(sizeof(dtpm_cpu), GFP_KERNEL);
>> +    if (!dtpm_cpu) {
>> +        kfree(dtpm);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    dtpm->private = dtpm_cpu;
>> +    dtpm_cpu->cpu = cpu;
>> +
>> +    for_each_cpu(cpu, policy->related_cpus)
>> +        per_cpu(dtpm_per_cpu, cpu) = dtpm;
>> +
>> +    ret = power_add(dtpm, pd);
>> +    if (ret)
>> +        goto out_kfree_dtpm_cpu;
>> +
>> +    dtpm->power_limit = dtpm->power_max;
> 
> Here, the power_limit will be set only once with power_max
> for a single CPU. I am not sure, but maybe we can simple say:
> 
> dtpm->power_limit = dtpm->power_max * cpumask_weight(policy->related_cpus)
> 
> an avoid touching it later (?)
> 
> Because this function can be called in runtime, when the power_limit
> was already set by userspace, the hotpluging in/out/in... CPU shouldn't
> change this limit.

Hmm, I have to think about it because the power_limit is always less or
equal to power_max.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog