[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAJZ5v0ikE668dXQRP7JTxU44t7TeLHQGNxR5T0AeiiFpPHQDOA@mail.gmail.com>
Date: Tue, 27 Aug 2024 13:57:58 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, x86 Maintainers <x86@...nel.org>,
LKML <linux-kernel@...r.kernel.org>, Linux PM <linux-pm@...r.kernel.org>,
Thomas Gleixner <tglx@...utronix.de>, Peter Zijlstra <peterz@...radead.org>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
"Rafael J. Wysocki" <rafael@...nel.org>, Dietmar Eggemann <dietmar.eggemann@....com>,
Ricardo Neri <ricardo.neri@...el.com>, Tim Chen <tim.c.chen@...el.com>
Subject: Re: [PATCH v2 2/3] x86/sched: Add basic support for CPU capacity scaling
On Tue, Aug 27, 2024 at 12:02 AM Ricardo Neri
<ricardo.neri-calderon@...ux.intel.com> wrote:
>
> On Mon, Aug 12, 2024 at 02:42:26PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> [...]
>
> > +bool arch_enable_hybrid_capacity_scale(void)
> > +{
> > + int cpu;
> > +
> > + if (static_branch_unlikely(&arch_hybrid_cap_scale_key)) {
> > + WARN_ONCE(1, "Hybrid CPU capacity scaling already enabled");
> > + return true;
> > + }
>
> Maybe an empty line here for readability?
Sure, if this helps.
> > + arch_cpu_scale = alloc_percpu(struct arch_hybrid_cpu_scale);
> > + if (!arch_cpu_scale)
> > + return false;
> > +
> > + for_each_possible_cpu(cpu) {
> > + per_cpu_ptr(arch_cpu_scale, cpu)->capacity = SCHED_CAPACITY_SCALE;
> > + per_cpu_ptr(arch_cpu_scale, cpu)->freq_ratio = arch_max_freq_ratio;
> > + }
> > +
> > + static_branch_enable(&arch_hybrid_cap_scale_key);
> > +
> > + pr_info("Hybrid CPU capacity scaling enabled\n");
> > +
> > + return true;
> > +}
> > +
> > +/**
> > + * arch_set_cpu_capacity - Set scale-invariance parameters for a CPU
> > + * @cpu: Target CPU.
> > + * @cap: Capacity of @cpu, relative to @base_cap, at its maximum frequency.
> > + * @base_cap: System-wide maximum CPU capacity.
>
> It is confusing to e that @base_cap is the maximum capacity of the system.
> Maybe @max_cap?
But the max_cap and max_freq are sort of confusing again.
I guess I can call it max_cap and also rename max_freq to cap_freq.
> > + * @max_freq: Frequency of @cpu corresponding to @cap.
> > + * @base_freq: Frequency of @cpu at which MPERF counts.
> > + *
> > + * The units in which @cap and @base_cap are expressed do not matter, so long
> > + * as they are consistent, because the former is effectively divided by the
> > + * latter. Analogously for @max_freq and @base_freq.
> > + *
> > + * After calling this function for all CPUs, call arch_rebuild_sched_domains()
> > + * to let the scheduler know that capacity-aware scheduling can be used going
> > + * forward.
> > + */
> > +void arch_set_cpu_capacity(int cpu, unsigned long cap, unsigned long base_cap,
> > + unsigned long max_freq, unsigned long base_freq)
> > +{
> > + if (static_branch_likely(&arch_hybrid_cap_scale_key)) {
> > + WRITE_ONCE(per_cpu_ptr(arch_cpu_scale, cpu)->capacity,
> > + div_u64(cap << SCHED_CAPACITY_SHIFT, base_cap));
> > + WRITE_ONCE(per_cpu_ptr(arch_cpu_scale, cpu)->freq_ratio,
> > + div_u64(max_freq << SCHED_CAPACITY_SHIFT, base_freq));
> > + } else {
> > + WARN_ONCE(1, "Hybrid CPU capacity scaling not enabled");
> > + }
> > +}
> > +
> > +unsigned long arch_scale_cpu_capacity(int cpu)
> > +{
> > + if (static_branch_unlikely(&arch_hybrid_cap_scale_key))
> > + return READ_ONCE(per_cpu_ptr(arch_cpu_scale, cpu)->capacity);
> > +
> > + return SCHED_CAPACITY_SCALE;
> > +}
> > +EXPORT_SYMBOL_GPL(arch_scale_cpu_capacity);
> > +
> > static void scale_freq_tick(u64 acnt, u64 mcnt)
> > {
> > u64 freq_scale;
> > + u64 freq_ratio;
>
> Why can't freq_ratio be declared on the same line as freq_scale?
It can.
> >
> > if (!arch_scale_freq_invariant())
> > return;
> > @@ -359,7 +439,12 @@ static void scale_freq_tick(u64 acnt, u6
> > if (check_shl_overflow(acnt, 2*SCHED_CAPACITY_SHIFT, &acnt))
> > goto error;
> >
> > - if (check_mul_overflow(mcnt, arch_max_freq_ratio, &mcnt) || !mcnt)
> > + if (static_branch_unlikely(&arch_hybrid_cap_scale_key))
> > + freq_ratio = READ_ONCE(this_cpu_ptr(arch_cpu_scale)->freq_ratio);
> > + else
> > + freq_ratio = arch_max_freq_ratio;
>
> It seems that arch_max_freq_ratio will never be used on hybrid processors
> and computing arch_turbo_freq_ratio will be a waste of cycles.
Well, what if the memory allocation is
arch_enable_hybrid_capacity_scale() fails?
> Unfortunately, intel_set_max_freq_ratio() is called before the
> arch_hybrid_cap_scale_key static key is set.
>
> Maybe some rework is in order?
I'd rather not do it. This is all initialization and done once.
However, a driver mode change can mess up with it which I have
overlooked. I'll fix this (and make the above changes) and send a new
version of the series.
Powered by blists - more mailing lists