Message-ID: <20180607144409.GB3311@localhost.localdomain>
Date:   Thu, 7 Jun 2018 16:44:09 +0200
From:   Juri Lelli <juri.lelli@...hat.com>
To:     Quentin Perret <quentin.perret@....com>
Cc:     peterz@...radead.org, rjw@...ysocki.net,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, mingo@...hat.com,
        dietmar.eggemann@....com, morten.rasmussen@....com,
        chris.redpath@....com, patrick.bellasi@....com,
        valentin.schneider@....com, vincent.guittot@...aro.org,
        thara.gopinath@...aro.org, viresh.kumar@...aro.org,
        tkjos@...gle.com, joelaf@...gle.com, smuckle@...gle.com,
        adharmap@...cinc.com, skannan@...cinc.com, pkondeti@...eaurora.org,
        edubezval@...il.com, srinivas.pandruvada@...ux.intel.com,
        currojerez@...eup.net, javi.merino@...nel.org
Subject: Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management
 framework

On 21/05/18 15:24, Quentin Perret wrote:
> Several subsystems in the kernel (scheduler and/or thermal at the time
> of writing) can benefit from knowing about the energy consumed by CPUs.
> Yet, this information can come from different sources (DT or firmware for
> example), in different formats, hence making it hard to exploit without
> a standard API.
> 
> This patch attempts to solve this issue by introducing a centralized
> Energy Model (EM) framework which can be used to interface the data
> providers with the client subsystems. This framework standardizes the
> API to expose power costs, and to access them from multiple locations.
> 
> The current design assumes that all CPUs in a frequency domain share the
> same micro-architecture. As such, the EM data is structured in a
> per-frequency-domain fashion. Drivers aware of frequency domains
> (typically, but not limited to, CPUFreq drivers) are expected to register
> data in the EM framework using the em_register_freq_domain() API. To do
> so, the drivers must provide a callback function that will be called by
> the EM framework to populate the tables. As of today, only the active
> power of the CPUs is considered. For each frequency domain, the EM
> includes a list of <frequency, power, capacity> tuples for the capacity
> states of the domain alongside a cpumask covering the involved CPUs.
> 
> The EM framework also provides an API to re-scale the capacity values
> of the model asynchronously, after it has been created. This is required
> for architectures where the capacity scale factor of CPUs can change at
> run-time. This is the case for Arm/Arm64 for example where the
> arch_topology driver recomputes the capacity scale factors of the CPUs
> after the maximum frequency of all CPUs has been discovered. Although
> complex, the process of creating and re-scaling the EM has to be kept in
> two separate steps to fulfill the needs of the different users. The thermal
> subsystem doesn't use the capacity values and shouldn't have dependencies
> on subsystems providing them. On the other hand, the task scheduler needs
> the capacity values, and it will benefit from seeing them up-to-date when
> applicable.
> 
> Because of this need for asynchronous update, the capacity state table
> of each frequency domain is protected by RCU, hence guaranteeing a safe
> modification of the table and a fast access to readers in latency-sensitive
> code paths.
> 
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>
> Signed-off-by: Quentin Perret <quentin.perret@....com>
> ---

OK, I think I'll start with a few comments while I get more into
understanding the set. :)

> +static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
> +{
> +	unsigned long cmax = arch_scale_cpu_capacity(NULL, cpu);
> +	int max_cap_state = cs_table->nr_cap_states - 1;
                 ^
You don't need this on the stack, right?

> +	unsigned long fmax = cs_table->state[max_cap_state].frequency;
> +	int i;
> +
> +	for (i = 0; i < cs_table->nr_cap_states; i++)
> +		cs_table->state[i].capacity = cmax *
> +					cs_table->state[i].frequency / fmax;
> +}
> +
> +static struct em_freq_domain *em_create_fd(cpumask_t *span, int nr_states,
> +						struct em_data_callback *cb)
> +{
> +	unsigned long opp_eff, prev_opp_eff = ULONG_MAX;
> +	int i, ret, cpu = cpumask_first(span);
> +	struct em_freq_domain *fd;
> +	unsigned long power, freq;
> +
> +	if (!cb->active_power)
> +		return NULL;
> +
> +	fd = kzalloc(sizeof(*fd), GFP_KERNEL);
> +	if (!fd)
> +		return NULL;
> +
> +	fd->cs_table = alloc_cs_table(nr_states);

Mmm, don't you need to rcu_assign_pointer this first one as well?

> +	if (!fd->cs_table)
> +		goto free_fd;
> +
> +	/* Copy the span of the frequency domain */
> +	cpumask_copy(&fd->cpus, span);
> +
> +	/* Build the list of capacity states for this freq domain */
> +	for (i = 0, freq = 0; i < nr_states; i++, freq++) {
                     ^                              ^
The fact that this relies on active_power() to use ceil OPP for a given
freq might deserve a comment. Also, is this behaviour of active_power()
standardized?

> +		ret = cb->active_power(&power, &freq, cpu);
> +		if (ret)
> +			goto free_cs_table;
> +
> +		fd->cs_table->state[i].power = power;
> +		fd->cs_table->state[i].frequency = freq;
> +
> +		/*
> +		 * The hertz/watts efficiency ratio should decrease as the
> +		 * frequency grows on sane platforms. If not, warn the user
> +		 * that some high OPPs are more power efficient than some
> +		 * of the lower ones.
> +		 */
> +		opp_eff = freq / power;
> +		if (opp_eff >= prev_opp_eff)
> +			pr_warn("%*pbl: hz/watt efficiency: OPP %d >= OPP%d\n",
> +					cpumask_pr_args(span), i, i - 1);
> +		prev_opp_eff = opp_eff;
> +	}
> +	fd_update_cs_table(fd->cs_table, cpu);
> +
> +	return fd;
> +
> +free_cs_table:
> +	free_cs_table(fd->cs_table);
> +free_fd:
> +	kfree(fd);
> +
> +	return NULL;
> +}
> +
> +static void rcu_free_cs_table(struct rcu_head *rp)
> +{
> +	struct em_cs_table *table;
> +
> +	table = container_of(rp, struct em_cs_table, rcu);
> +	free_cs_table(table);
> +}
> +
> +/**
> + * em_rescale_cpu_capacity() - Re-scale capacity values of the Energy Model
> + *
> + * This re-scales the capacity values for all capacity states of all frequency
> + * domains of the Energy Model. This should be used when the capacity values
> + * of the CPUs are updated at run-time, after the EM was registered.
> + */
> +void em_rescale_cpu_capacity(void)

So, is this thought to be called eventually also after thermal capping
events and such?

> +{
> +	struct em_cs_table *old_table, *new_table;
> +	struct em_freq_domain *fd;
> +	unsigned long flags;
> +	int nr_states, cpu;
> +
> +	read_lock_irqsave(&em_data_lock, flags);

Don't you need write_lock_ here, since you are going to exchange the
em tables?

> +	for_each_cpu(cpu, cpu_possible_mask) {
> +		fd = per_cpu(em_data, cpu);
> +		if (!fd || cpu != cpumask_first(&fd->cpus))
> +			continue;
> +
> +		/* Copy the existing table. */
> +		old_table = rcu_dereference(fd->cs_table);
> +		nr_states = old_table->nr_cap_states;
> +		new_table = alloc_cs_table(nr_states);
> +		if (!new_table) {
> +			read_unlock_irqrestore(&em_data_lock, flags);
> +			return;
> +		}
> +		memcpy(new_table->state, old_table->state,
> +					nr_states * sizeof(*new_table->state));
> +
> +		/* Re-scale the capacity values on the copy. */
> +		fd_update_cs_table(new_table, cpumask_first(&fd->cpus));
> +
> +		/* Replace the table with the rescaled version. */
> +		rcu_assign_pointer(fd->cs_table, new_table);
> +		call_rcu(&old_table->rcu, rcu_free_cs_table);
> +	}
> +	read_unlock_irqrestore(&em_data_lock, flags);
> +	pr_debug("Re-scaled CPU capacities\n");
> +}
> +EXPORT_SYMBOL_GPL(em_rescale_cpu_capacity);
> +
> +/**
> + * em_cpu_get() - Return the frequency domain for a CPU
> + * @cpu : CPU to find the frequency domain for
> + *
> + * Return: the frequency domain to which 'cpu' belongs, or NULL if it doesn't
> + * exist.
> + */
> +struct em_freq_domain *em_cpu_get(int cpu)
> +{
> +	struct em_freq_domain *fd;
> +	unsigned long flags;
> +
> +	read_lock_irqsave(&em_data_lock, flags);
> +	fd = per_cpu(em_data, cpu);
> +	read_unlock_irqrestore(&em_data_lock, flags);
> +
> +	return fd;
> +}
> +EXPORT_SYMBOL_GPL(em_cpu_get);

Mmm, this gets complicated pretty fast eh? :)

I had to go back and forth between patches to start understanding the
different data structures and how they are use, and I'm not sure yet
I've got the full picture. I guess some nice diagram (cover letter or
documentation patch) would help a lot.

Locking of such data structures is pretty involved as well, adding
comments/docs shouldn't harm. :)

Best,

- Juri

Powered by blists - more mailing lists