lkml.org 
[lkml]   [2020]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 4/4] powercap/drivers/dtpm: Add CPU energy model based support
From
Date
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.

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

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.

Regards,
Lukasz

\
 
 \ /
  Last update: 2020-11-26 11:07    [W:0.041 / U:0.420 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site