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

[Daniel Lezcano]

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