Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

From: Juri Lelli
Date: Thu Jun 07 2018 - 10:44:22 EST


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@xxxxxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> Signed-off-by: Quentin Perret <quentin.perret@xxxxxxx>
> ---

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