linux-kernel - Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model

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: <6b980e2a-c15c-0718-14b8-e8aa7510c832@linaro.org>
Date:   Mon, 6 Apr 2020 16:58:52 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Lukasz Luba <lukasz.luba@....com>, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        dri-devel@...ts.freedesktop.org, linux-omap@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
        linux-imx@....com
Cc:     Morten.Rasmussen@....com, Dietmar.Eggemann@....com,
        javi.merino@....com, cw00.choi@...sung.com,
        b.zolnierkie@...sung.com, rjw@...ysocki.net, sudeep.holla@....com,
        viresh.kumar@...aro.org, nm@...com, sboyd@...nel.org,
        rui.zhang@...el.com, amit.kucheria@...durent.com, mingo@...hat.com,
        peterz@...radead.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, rostedt@...dmis.org,
        qperret@...gle.com, bsegall@...gle.com, mgorman@...e.de,
        shawnguo@...nel.org, s.hauer@...gutronix.de, festevam@...il.com,
        kernel@...gutronix.de, khilman@...nel.org, agross@...nel.org,
        bjorn.andersson@...aro.org, robh@...nel.org,
        matthias.bgg@...il.com, steven.price@....com,
        tomeu.vizoso@...labora.com, alyssa.rosenzweig@...labora.com,
        airlied@...ux.ie, daniel@...ll.ch, liviu.dudau@....com,
        lorenzo.pieralisi@....com, patrick.bellasi@...bug.net,
        orjan.eide@....com, rdunlap@...radead.org, mka@...omium.org
Subject: Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model


Hi Lukasz,


On 06/04/2020 15:29, Lukasz Luba wrote:
> Hi Daniel,
> 
> Thank you for the review.
> 
> On 4/3/20 5:05 PM, Daniel Lezcano wrote:
>>
>> Hi Lukasz,
>>
>>
>> On 18/03/2020 12:45, Lukasz Luba wrote:
>>> Add support of other devices into the Energy Model framework not only
>>> the
>>> CPUs. Change the interface to be more unified which can handle other
>>> devices as well.
>>
>> thanks for taking care of that. Overall I like the changes in this patch
>> but it hard to review in details because the patch is too big :/
>>
>> Could you split this patch into smaller ones?
>>
>> eg. (at your convenience)
>>
>>   - One patch renaming s/cap/perf/
>>
>>   - One patch adding a new function:
>>
>>      em_dev_register_perf_domain(struct device *dev,
>>                 unsigned int nr_states,
>>                 struct em_data_callback *cb);
>>
>>     (+ EXPORT_SYMBOL_GPL)
>>
>>      And em_register_perf_domain() using it.
>>
>>   - One converting the em_register_perf_domain() user to
>>     em_dev_register_perf_domain
>>
>>   - One adding the different new 'em' functions
>>
>>   - And finally one removing em_register_perf_domain().
> 
> I agree and will do the split. I could also break the dependencies
> for future easier merge.
> 
>>
>>
>>> Acked-by: Quentin Perret <qperret@...gle.com>
>>> Signed-off-by: Lukasz Luba <lukasz.luba@....com>
>>> ---
>>
>> [ ... ]
>>
>>>   2. Core APIs
>>> @@ -70,14 +72,16 @@ CONFIG_ENERGY_MODEL must be enabled to use the EM
>>> framework.
>>>   Drivers are expected to register performance domains into the EM
>>> framework by
>>>   calling the following API::
>>>   -  int em_register_perf_domain(cpumask_t *span, unsigned int
>>> nr_states,
>>> -                  struct em_data_callback *cb);
>>> +  int em_register_perf_domain(struct device *dev, unsigned int
>>> nr_states,
>>> +        struct em_data_callback *cb, cpumask_t *cpus);
>>
>> Isn't possible to get rid of this cpumask by using
>> cpufreq_cpu_get() which returns the cpufreq's policy and from their get
>> the related cpus ?
> 
> We had similar thoughts with Quentin and I've checked this.

Yeah, I suspected you already think about that :)

> Unfortunately, if the policy is a 'new policy' [1] it gets
> allocated and passed into cpufreq driver ->init(policy) [2].
> Then that policy is set into per_cpu pointer for each related_cpu [3]:
> 
> for_each_cpu(j, policy->related_cpus)
>     per_cpu(cpufreq_cpu_data, j) = policy;
> 
>  
> Thus, any calls of functions (i.e. cpufreq_cpu_get()) which try to
> take this ptr before [3] won't work.
> 
> We are trying to register EM from cpufreq_driver->init(policy) and the
> per_cpu policy is likely to be not populated at that phase.

What is the problem of registering at the end of the cpufreq_online ?


> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1328
> 
> [2]
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1350
> 
> [3]
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1374
> 
> 
> 


-- 
<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