lkml.org 
[lkml]   [2018]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@infradead.org>
    > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
    > Signed-off-by: Quentin Perret <quentin.perret@arm.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

    \
     
     \ /
      Last update: 2018-06-07 17:34    [W:4.332 / U:0.248 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site