Re: [PATCH v7 03/14] PM: Introduce an Energy Model management framework

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Sep 12, 2018 at 10:12:58AM +0100, Quentin Perret wrote:
> +/**
> + * em_register_perf_domain() - Register the Energy Model of a performance domain
> + * @span	: Mask of CPUs in the performance domain
> + * @nr_states	: Number of capacity states to register
> + * @cb		: Callback functions providing the data of the Energy Model
> + *
> + * Create Energy Model tables for a performance domain using the callbacks
> + * defined in cb.
> + *
> + * If multiple clients register the same performance domain, all but the first
> + * registration will be ignored.
> + *
> + * Return 0 on success
> + */
> +int em_register_perf_domain(cpumask_t *span, unsigned int nr_states,
> +						struct em_data_callback *cb)
> +{
> +	unsigned long cap, prev_cap = 0;
> +	struct em_perf_domain *pd;
> +	int cpu, ret = 0;
> +
> +	if (!span || !nr_states || !cb)
> +		return -EINVAL;
> +
> +	/*
> +	 * Use a mutex to serialize the registration of performance domains and
> +	 * let the driver-defined callback functions sleep.
> +	 */
> +	mutex_lock(&em_pd_mutex);
> +
> +	for_each_cpu(cpu, span) {
> +		/* Make sure we don't register again an existing domain. */
> +		if (READ_ONCE(per_cpu(em_data, cpu))) {
> +			ret = -EEXIST;
> +			goto unlock;
> +		}
> +
> +		/*
> +		 * All CPUs of a domain must have the same micro-architecture
> +		 * since they all share the same table.
> +		 */
> +		cap = arch_scale_cpu_capacity(NULL, cpu);
> +		if (prev_cap && prev_cap != cap) {
> +			pr_err("CPUs of %*pbl must have the same capacity\n",
> +							cpumask_pr_args(span));
> +			ret = -EINVAL;
> +			goto unlock;
> +		}
> +		prev_cap = cap;
> +	}
> +
> +	/* Create the performance domain and add it to the Energy Model. */
> +	pd = em_create_pd(span, nr_states, cb);
> +	if (!pd) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	for_each_cpu(cpu, span)
> +		WRITE_ONCE(per_cpu(em_data, cpu), pd);

It's not immediately obvious to me why this doesn't need to be
smp_store_release(). The moment you publish that pointer, it can be
read, right?

Even if you never again change the pointer value, you want to ensure the
content of pd is stable before pd itself is observable, right?

> +
> +	pr_debug("Created perf domain %*pbl\n", cpumask_pr_args(span));
> +unlock:
> +	mutex_unlock(&em_pd_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(em_register_perf_domain);
> -- 
> 2.18.0
>