From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: ulf.hansson@linaro.org, linux-pm@vger.kernel.org, corbet@lwn.net,
rjw@rjwysocki.net, linux-kernel@vger.kernel.org,
ilina@codeaurora.org, rkumbako@codeaurora.org,
rui.zhang@intel.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 4/4] powercap/drivers/dtpm: Add CPU energy model based support
Date: Thu, 26 Nov 2020 16:06:54 +0100 [thread overview]
Message-ID: <68b0f4e5-bdff-a4a7-f59a-e2a4d0a138de@linaro.org> (raw)
In-Reply-To: <50db7265-3870-b977-6e41-b0a0ac3cdb94@arm.com>
Hi Lukasz,
On 26/11/2020 11:06, Lukasz Luba wrote:
> Hi Daniel,
>
> On 11/23/20 9:42 PM, Daniel Lezcano wrote:
>> With the powercap dtpm controller, we are able to plug devices with
>> power limitation features in the tree.
>>
>
> [snip]
>
>> +
>> +static void pd_release(struct dtpm *dtpm)
>> +{
>> + struct dtpm_cpu *dtpm_cpu = dtpm->private;
>> +
>
> Maybe it's worth to add:
> ------------------->8----------------
> if (freq_qos_request_active(&dtpm_cpu->qos_req))
> freq_qos_remove_request(&dtpm_cpu->qos_req);
> -------------------8<---------------
>
> If we are trying to unregister dtpm in error path due to freq_qos
> registration failure, a warning would be emitted from freq_qos.
Ah yes, good point.
>> + freq_qos_remove_request(&dtpm_cpu->qos_req);
>> + kfree(dtpm_cpu);
>> +}
>
> [snip]
>
>> +
>> +static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>> +{
>> + struct dtpm *dtpm;
>> + struct dtpm_cpu *dtpm_cpu;
>> + struct cpufreq_policy *policy;
>> + struct em_perf_domain *pd;
>> + char name[CPUFREQ_NAME_LEN];
>> + int ret;
>> +
>> + policy = cpufreq_cpu_get(cpu);
>> +
>> + if (!policy)
>> + return 0;
>> +
>> + pd = em_cpu_get(cpu);
>> + if (!pd)
>> + return -EINVAL;
>> +
>> + dtpm = per_cpu(dtpm_per_cpu, cpu);
>> + if (dtpm)
>> + return power_add(dtpm, pd);
>> +
>> + dtpm = dtpm_alloc(&dtpm_ops);
>> + 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;
>> +
>> + sprintf(name, "cpu%d", dtpm_cpu->cpu);
>> +
>> + ret = dtpm_register(name, dtpm, __parent);
>> + if (ret)
>> + goto out_kfree_dtpm_cpu;
>> +
>> + ret = power_add(dtpm, pd);
>> + if (ret)
>> + goto out_power_sub;
>
> Shouldn't we call dtpm_unregister() instead?
> The dtpm_unregister() would remove the zone, which IIUC we
> are currently missing.
>
>> +
>> + ret = freq_qos_add_request(&policy->constraints,
>> + &dtpm_cpu->qos_req, FREQ_QOS_MAX,
>> + pd->table[pd->nr_perf_states - 1].frequency);
>> + if (ret)
>> + goto out_dtpm_unregister;
>
> Could this trigger different steps, starting from out_power_sub_v2
> below?
>
>> +
>> + return 0;
>> +
>> +out_dtpm_unregister:
>> + dtpm_unregister(dtpm);
>> + dtpm_cpu = NULL; /* Already freed by the release ops */
>> +out_power_sub:
>> + power_sub(dtpm, pd);
>
> I would change the order of these two above into something like:
Ok, I'll revisit the rollback routine.
> out_power_sub_v2:
> power_sub(dtpm, pd);
> out_dtpm_unregister_v2:
> dtpm_unregister(dtpm);
> dtpm_cpu = NULL;
>
>> +out_kfree_dtpm_cpu:
>> + for_each_cpu(cpu, policy->related_cpus)
>> + per_cpu(dtpm_per_cpu, cpu) = NULL;
>> + kfree(dtpm_cpu);
>> +
>> + return ret;
>> +}
>
> IIUC power_sub() would decrement the power and set it to 0 for that
> dtmp, then the dtpm_unregister() would also try to decrement the power,
> but by the value of 0. So it should be safe.
Right.
Thanks for the review
--
<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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
prev parent reply other threads:[~2020-11-26 15:08 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-23 21:42 [PATCH V2 0/4] powercap/dtpm: Add the DTPM framework Daniel Lezcano
2020-11-23 21:42 ` [PATCH v3 1/4] units: Add Watt units Daniel Lezcano
2020-11-23 21:42 ` [PATCH v3 2/4] Documentation/powercap/dtpm: Add documentation for dtpm Daniel Lezcano
2020-11-23 21:42 ` [PATCH v3 3/4] powercap/drivers/dtpm: Add API for dynamic thermal power management Daniel Lezcano
2020-11-23 21:42 ` [PATCH v3 4/4] powercap/drivers/dtpm: Add CPU energy model based support Daniel Lezcano
2020-11-26 10:06 ` Lukasz Luba
2020-11-26 15:06 ` Daniel Lezcano [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=68b0f4e5-bdff-a4a7-f59a-e2a4d0a138de@linaro.org \
--to=daniel.lezcano@linaro.org \
--cc=corbet@lwn.net \
--cc=ilina@codeaurora.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lukasz.luba@arm.com \
--cc=rjw@rjwysocki.net \
--cc=rkumbako@codeaurora.org \
--cc=rui.zhang@intel.com \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).