Re: [PATCH 1/5] powercap/drivers/dtpm: Encapsulate even more the code

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 01/03/2021 22:21, Daniel Lezcano wrote:
> In order to increase the self-encapsulation of the dtpm generic code,
> the following changes are adding a power update ops to the dtpm
> ops. That allows the generic code to call directly the dtpm backend
> function to update the power values.
> 
> The power update function does compute the power characteristics when
> the function is invoked. In the case of the CPUs, the power
> consumption depends on the number of online CPUs. The online CPUs mask
> is not up to date at CPUHP_AP_ONLINE_DYN state in the tear down
> callback. That is the reason why the online / offline are at separate
> state. As there is already an existing state for DTPM, this one is
> only moved to the DEAD state, so there is no addition of new state
> with these changes.
> 
> That simplifies the code for the next changes and results in a more
> self-encapsulated code.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Is there any comment on this series ?

> ---
>  drivers/powercap/dtpm.c     |  54 ++++++++--------
>  drivers/powercap/dtpm_cpu.c | 124 +++++++++++++-----------------------
>  include/linux/cpuhotplug.h  |   2 +-
>  include/linux/dtpm.h        |   3 +-
>  4 files changed, 76 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> index c2185ec5f887..1085dccf9c58 100644
> --- a/drivers/powercap/dtpm.c
> +++ b/drivers/powercap/dtpm.c
> @@ -116,8 +116,6 @@ static void __dtpm_sub_power(struct dtpm *dtpm)
>  		parent->power_limit -= dtpm->power_limit;
>  		parent = parent->parent;
>  	}
> -
> -	__dtpm_rebalance_weight(root);
>  }
>  
>  static void __dtpm_add_power(struct dtpm *dtpm)
> @@ -130,45 +128,45 @@ static void __dtpm_add_power(struct dtpm *dtpm)
>  		parent->power_limit += dtpm->power_limit;
>  		parent = parent->parent;
>  	}
> +}
> +
> +static int __dtpm_update_power(struct dtpm *dtpm)
> +{
> +	int ret;
> +
> +	__dtpm_sub_power(dtpm);
>  
> -	__dtpm_rebalance_weight(root);
> +	ret = dtpm->ops->upt_power_uw(dtpm);
> +	if (ret)
> +		pr_err("Failed to update power for '%s': %d\n",
> +		       dtpm->zone.name, ret);
> +
> +	if (!test_bit(DTPM_POWER_LIMIT_FLAG, &dtpm->flags))
> +		dtpm->power_limit = dtpm->power_max;
> +
> +	__dtpm_add_power(dtpm);
> +
> +	if (root)
> +		__dtpm_rebalance_weight(root);
> +
> +	return ret;
>  }
>  
>  /**
>   * dtpm_update_power - Update the power on the dtpm
>   * @dtpm: a pointer to a dtpm structure to update
> - * @power_min: a u64 representing the new power_min value
> - * @power_max: a u64 representing the new power_max value
>   *
>   * Function to update the power values of the dtpm node specified in
>   * parameter. These new values will be propagated to the tree.
>   *
>   * Return: zero on success, -EINVAL if the values are inconsistent
>   */
> -int dtpm_update_power(struct dtpm *dtpm, u64 power_min, u64 power_max)
> +int dtpm_update_power(struct dtpm *dtpm)
>  {
> -	int ret = 0;
> +	int ret;
>  
>  	mutex_lock(&dtpm_lock);
> -
> -	if (power_min == dtpm->power_min && power_max == dtpm->power_max)
> -		goto unlock;
> -
> -	if (power_max < power_min) {
> -		ret = -EINVAL;
> -		goto unlock;
> -	}
> -
> -	__dtpm_sub_power(dtpm);
> -
> -	dtpm->power_min = power_min;
> -	dtpm->power_max = power_max;
> -	if (!test_bit(DTPM_POWER_LIMIT_FLAG, &dtpm->flags))
> -		dtpm->power_limit = power_max;
> -
> -	__dtpm_add_power(dtpm);
> -
> -unlock:
> +	ret = __dtpm_update_power(dtpm);
>  	mutex_unlock(&dtpm_lock);
>  
>  	return ret;
> @@ -436,6 +434,7 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
>  
>  	if (dtpm->ops && !(dtpm->ops->set_power_uw &&
>  			   dtpm->ops->get_power_uw &&
> +			   dtpm->ops->upt_power_uw &&
>  			   dtpm->ops->release))
>  		return -EINVAL;
>  
> @@ -455,7 +454,8 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
>  		root = dtpm;
>  	}
>  
> -	__dtpm_add_power(dtpm);
> +	if (dtpm->ops && !dtpm->ops->upt_power_uw(dtpm))
> +		__dtpm_add_power(dtpm);
>  
>  	pr_info("Registered dtpm node '%s' / %llu-%llu uW, \n",
>  		dtpm->zone.name, dtpm->power_min, dtpm->power_max);
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index 51c366938acd..aff79c649345 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -14,6 +14,8 @@
>   * The CPU hotplug is supported and the power numbers will be updated
>   * if a CPU is hot plugged / unplugged.
>   */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/cpumask.h>
>  #include <linux/cpufreq.h>
>  #include <linux/cpuhotplug.h>
> @@ -23,8 +25,6 @@
>  #include <linux/slab.h>
>  #include <linux/units.h>
>  
> -static struct dtpm *__parent;
> -
>  static DEFINE_PER_CPU(struct dtpm *, dtpm_per_cpu);
>  
>  struct dtpm_cpu {
> @@ -32,57 +32,16 @@ struct dtpm_cpu {
>  	int cpu;
>  };
>  
> -/*
> - * When a new CPU is inserted at hotplug or boot time, add the power
> - * contribution and update the dtpm tree.
> - */
> -static int power_add(struct dtpm *dtpm, struct em_perf_domain *em)
> -{
> -	u64 power_min, power_max;
> -
> -	power_min = em->table[0].power;
> -	power_min *= MICROWATT_PER_MILLIWATT;
> -	power_min += dtpm->power_min;
> -
> -	power_max = em->table[em->nr_perf_states - 1].power;
> -	power_max *= MICROWATT_PER_MILLIWATT;
> -	power_max += dtpm->power_max;
> -
> -	return dtpm_update_power(dtpm, power_min, power_max);
> -}
> -
> -/*
> - * When a CPU is unplugged, remove its power contribution from the
> - * dtpm tree.
> - */
> -static int power_sub(struct dtpm *dtpm, struct em_perf_domain *em)
> -{
> -	u64 power_min, power_max;
> -
> -	power_min = em->table[0].power;
> -	power_min *= MICROWATT_PER_MILLIWATT;
> -	power_min = dtpm->power_min - power_min;
> -
> -	power_max = em->table[em->nr_perf_states - 1].power;
> -	power_max *= MICROWATT_PER_MILLIWATT;
> -	power_max = dtpm->power_max - power_max;
> -
> -	return dtpm_update_power(dtpm, power_min, power_max);
> -}
> -
>  static u64 set_pd_power_limit(struct dtpm *dtpm, u64 power_limit)
>  {
>  	struct dtpm_cpu *dtpm_cpu = dtpm->private;
> -	struct em_perf_domain *pd;
> +	struct em_perf_domain *pd = em_cpu_get(dtpm_cpu->cpu);
>  	struct cpumask cpus;
>  	unsigned long freq;
>  	u64 power;
>  	int i, nr_cpus;
>  
> -	pd = em_cpu_get(dtpm_cpu->cpu);
> -
>  	cpumask_and(&cpus, cpu_online_mask, to_cpumask(pd->cpus));
> -
>  	nr_cpus = cpumask_weight(&cpus);
>  
>  	for (i = 0; i < pd->nr_perf_states; i++) {
> @@ -113,6 +72,7 @@ static u64 get_pd_power_uw(struct dtpm *dtpm)
>  
>  	pd = em_cpu_get(dtpm_cpu->cpu);
>  	freq = cpufreq_quick_get(dtpm_cpu->cpu);
> +
>  	cpumask_and(&cpus, cpu_online_mask, to_cpumask(pd->cpus));
>  	nr_cpus = cpumask_weight(&cpus);
>  
> @@ -128,6 +88,27 @@ static u64 get_pd_power_uw(struct dtpm *dtpm)
>  	return 0;
>  }
>  
> +static int upt_pd_power_uw(struct dtpm *dtpm)
> +{
> +	struct dtpm_cpu *dtpm_cpu = dtpm->private;
> +	struct em_perf_domain *em = em_cpu_get(dtpm_cpu->cpu);
> +	struct cpumask cpus;
> +	int nr_cpus;
> +
> +	cpumask_and(&cpus, cpu_online_mask, to_cpumask(em->cpus));
> +	nr_cpus = cpumask_weight(&cpus);
> +
> +	dtpm->power_min = em->table[0].power;
> +	dtpm->power_min *= MICROWATT_PER_MILLIWATT;
> +	dtpm->power_min *= nr_cpus;
> +
> +	dtpm->power_max = em->table[em->nr_perf_states - 1].power;
> +	dtpm->power_max *= MICROWATT_PER_MILLIWATT;
> +	dtpm->power_max *= nr_cpus;
> +
> +	return 0;
> +}
> +
>  static void pd_release(struct dtpm *dtpm)
>  {
>  	struct dtpm_cpu *dtpm_cpu = dtpm->private;
> @@ -141,37 +122,25 @@ static void pd_release(struct dtpm *dtpm)
>  static struct dtpm_ops dtpm_ops = {
>  	.set_power_uw = set_pd_power_limit,
>  	.get_power_uw = get_pd_power_uw,
> +	.upt_power_uw = upt_pd_power_uw,
>  	.release = pd_release,
>  };
>  
>  static int cpuhp_dtpm_cpu_offline(unsigned int cpu)
>  {
> -	struct cpufreq_policy *policy;
> +	struct cpumask cpus;
>  	struct em_perf_domain *pd;
>  	struct dtpm *dtpm;
>  
> -	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);
> -
> -	power_sub(dtpm, pd);
> -
> -	if (cpumask_weight(policy->cpus) != 1)
> -		return 0;
> -
> -	for_each_cpu(cpu, policy->related_cpus)
> -		per_cpu(dtpm_per_cpu, cpu) = NULL;
> +	cpumask_and(&cpus, cpu_online_mask, to_cpumask(pd->cpus));
>  
> -	dtpm_unregister(dtpm);
> +	dtpm = per_cpu(dtpm_per_cpu, cpu);
>  
> -	return 0;
> +	return dtpm_update_power(dtpm);
>  }
>  
>  static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> @@ -184,7 +153,6 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>  	int ret = -ENOMEM;
>  
>  	policy = cpufreq_cpu_get(cpu);
> -
>  	if (!policy)
>  		return 0;
>  
> @@ -194,7 +162,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>  
>  	dtpm = per_cpu(dtpm_per_cpu, cpu);
>  	if (dtpm)
> -		return power_add(dtpm, pd);
> +		return dtpm_update_power(dtpm);
>  
>  	dtpm = dtpm_alloc(&dtpm_ops);
>  	if (!dtpm)
> @@ -210,27 +178,20 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>  	for_each_cpu(cpu, policy->related_cpus)
>  		per_cpu(dtpm_per_cpu, cpu) = dtpm;
>  
> -	sprintf(name, "cpu%d", dtpm_cpu->cpu);
> +	sprintf(name, "cpu%d-cpufreq", dtpm_cpu->cpu);
>  
> -	ret = dtpm_register(name, dtpm, __parent);
> +	ret = dtpm_register(name, dtpm, NULL);
>  	if (ret)
>  		goto out_kfree_dtpm_cpu;
>  
> -	ret = power_add(dtpm, pd);
> -	if (ret)
> -		goto out_dtpm_unregister;
> -
>  	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_power_sub;
> +		goto out_dtpm_unregister;
>  
>  	return 0;
>  
> -out_power_sub:
> -	power_sub(dtpm, pd);
> -
>  out_dtpm_unregister:
>  	dtpm_unregister(dtpm);
>  	dtpm_cpu = NULL;
> @@ -248,10 +209,17 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>  
>  int dtpm_register_cpu(struct dtpm *parent)
>  {
> -	__parent = parent;
> +	int ret;
>  
> -	return cpuhp_setup_state(CPUHP_AP_DTPM_CPU_ONLINE,
> -				 "dtpm_cpu:online",
> -				 cpuhp_dtpm_cpu_online,
> -				 cpuhp_dtpm_cpu_offline);
> +	ret = cpuhp_setup_state(CPUHP_AP_DTPM_CPU_DEAD, "dtpm_cpu:offline",
> +				NULL, cpuhp_dtpm_cpu_offline);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "dtpm_cpu:online",
> +				cpuhp_dtpm_cpu_online, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
>  }
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index ee09a39627d6..fcb2967fb5ba 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -61,6 +61,7 @@ enum cpuhp_state {
>  	CPUHP_LUSTRE_CFS_DEAD,
>  	CPUHP_AP_ARM_CACHE_B15_RAC_DEAD,
>  	CPUHP_PADATA_DEAD,
> +	CPUHP_AP_DTPM_CPU_DEAD,
>  	CPUHP_WORKQUEUE_PREP,
>  	CPUHP_POWER_NUMA_PREPARE,
>  	CPUHP_HRTIMERS_PREPARE,
> @@ -193,7 +194,6 @@ enum cpuhp_state {
>  	CPUHP_AP_ONLINE_DYN_END		= CPUHP_AP_ONLINE_DYN + 30,
>  	CPUHP_AP_X86_HPET_ONLINE,
>  	CPUHP_AP_X86_KVM_CLK_ONLINE,
> -	CPUHP_AP_DTPM_CPU_ONLINE,
>  	CPUHP_AP_ACTIVE,
>  	CPUHP_ONLINE,
>  };
> diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
> index e80a332e3d8a..d29be6a0e513 100644
> --- a/include/linux/dtpm.h
> +++ b/include/linux/dtpm.h
> @@ -29,6 +29,7 @@ struct dtpm {
>  struct dtpm_ops {
>  	u64 (*set_power_uw)(struct dtpm *, u64);
>  	u64 (*get_power_uw)(struct dtpm *);
> +	int (*upt_power_uw)(struct dtpm *);
>  	void (*release)(struct dtpm *);
>  };
>  
> @@ -62,7 +63,7 @@ static inline struct dtpm *to_dtpm(struct powercap_zone *zone)
>  	return container_of(zone, struct dtpm, zone);
>  }
>  
> -int dtpm_update_power(struct dtpm *dtpm, u64 power_min, u64 power_max);
> +int dtpm_update_power(struct dtpm *dtpm);
>  
>  int dtpm_release_zone(struct powercap_zone *pcz);
>  
> 


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