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