Messages in this thread | | | Subject | Re: [PATCH v3 4/4] powercap/drivers/dtpm: Add CPU energy model based support | From | Lukasz Luba <> | Date | Thu, 26 Nov 2020 10:06:52 +0000 |
| |
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
| |