| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0jZf=FaLAPtZB1O_g+P=rH7Lu5kGPdn2K6pk3vXLLJbLg@mail.gmail.com>
Date: Tue, 19 Nov 2024 15:38:06 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Dietmar Eggemann <dietmar.eggemann@....com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>, Lukasz Luba <lukasz.luba@....com>,
Peter Zijlstra <peterz@...radead.org>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>, Len Brown <len.brown@...el.com>,
Morten Rasmussen <morten.rasmussen@....com>, Vincent Guittot <vincent.guittot@...aro.org>,
Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
Subject: Re: [RFC][PATCH v0.1 6/6] cpufreq: intel_pstate: Add basic EAS
support on hybrid platforms
First off, thanks for all the feedback!
On Tue, Nov 12, 2024 at 9:21 AM Dietmar Eggemann
<dietmar.eggemann@....com> wrote:
>
> On 08/11/2024 17:46, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > Modify intel_pstate to register stub EM perf domains for CPUs on
> > hybrid platforms via em_dev_register_perf_domain() and to use
> > em_dev_expand_perf_domain() introduced previously for adding new
> > CPUs to existing EM perf domains when those CPUs become online for
> > the first time after driver initialization.
> >
> > This change is targeting platforms (for example, Lunar Lake) where
> > "small" CPUs (E-cores) are always more energy-efficient than the "big"
> > or "performance" CPUs (P-cores) when run at the same HWP performance
> > level, so it is sufficient to tell the EAS that E-cores are always
> > preferred (so long as there is enough spare capacity on one of them
> > to run the given task).
>
> By treating all big CPUs (ignoring the different itmt prio values
> between them) we would have a system in which PD's are not in sync with
> the asym_cap_list* or the CPU capacities of individual CPUs and sched
> groups within the sched domain. Not sure if we want to go this way?
I guess you want the biggest tasks to be scheduled at the most-capable CPUs.
That's fair, and it may even improve single-threaded performance in
some cases I suppose, but then the cost for the "favored cores" PD
would be the same as for the "other P-cores" PD because there is no
difference between them other than the top-most turbo bin, so we'd
compare PDs with the same cost and that wouldn't be super-useful.
> * used by misfit handling - 22d5607400c6 ("sched/fair: Check if a task
> has a fitting CPU when updating misfit")
>
> > Accordingly, the perf domains are registered per CPU type (that is,
> > all P-cores belong to one perf domain and all E-cores belong to another
> > perf domain) and they are registered only if asymmetric CPU capacity is
> > enabled. Each perf domain has a one-element states table and that
> > element only contains the relative cost value (the other fields in
> > it are not initialized, so they are all equal to zero), and the cost
> > value for the E-core perf domain is lower.
>
> [...]
>
> > +static int hybrid_pcore_cost(struct device *dev, unsigned long freq,
> > + unsigned long *cost)
> > +{
> > + /*
> > + * The number used here needs to be higher than the analogous
> > + * one in hybrid_ecore_cost() below. The units and the actual
> > + * values don't matter.
> > + */
> > + *cost = 2;
> > + return 0;
>
> So you're not tying this to HFI energy scores?
Not at this time.
> > +}
> > +
> > +static int hybrid_ecore_cost(struct device *dev, unsigned long freq,
> > + unsigned long *cost)
> > +{
> > + *cost = 1;
> > + return 0;
> > +}
> > +
> > +static struct hybrid_em_perf_domain perf_domains[HYBRID_NR_TYPES] = {
> > + [HYBRID_PCORE] = { .cb.get_cost = hybrid_pcore_cost, },
> > + [HYBRID_ECORE] = { .cb.get_cost = hybrid_ecore_cost, }
> > +};
> > +
> > +static bool hybrid_register_perf_domain(struct hybrid_em_perf_domain *pd)
> > +{
> > + /*
> > + * Registering EM perf domains without asymmetric CPU capacity
> > + * support enabled is wasteful, so don't do that.
> > + */
> > + if (!hybrid_max_perf_cpu)
> > + return false;
> > +
> > + pd->dev = get_cpu_device(cpumask_first(&pd->cpumask));
> > + if (!pd->dev)
> > + return false;
> > +
> > + if (em_dev_register_perf_domain(pd->dev, 1, &pd->cb, &pd->cpumask, false)) {
> > + pd->dev = NULL;
> > + return false;
> > + }
> > +
> > + return true;
> > +}
>
> What are the issues in case you would use the existing ways (non-stub)
> to setup the EM?
>
> static int intel_pstate_get_cpu_cost()
>
> static void intel_pstate_register_em(struct cpufreq_policy *policy)
>
> struct em_data_callback em_cb = EM_ADV_DATA_CB(NULL,
> intel_pstate_get_cpu_cost)
>
> em_dev_register_perf_domain(get_cpu_device(policy->cpu), 1,
> &em_cb, policy->related_cpus, 1);
> ^^^^^^^^^^^^^^^^^^^^*
I'm not sure what you are asking about here, but I'll try to answer.
No, I don't want to register a PD per policy with one CPU in it
because that would mean useless comparing PDs with the same cost and
CPU capacity.
> static void intel_pstate_set_register_em_fct(void)
>
> default_driver->register_em = intel_pstate_register_em
No, I don't want to register PDs through cpufreq because it is too
early (and see above).
> static int __init intel_pstate_init(void)
>
> ...
> intel_pstate_set_register_em_fct()
> ...
>
> I guess one issue is the per-CPU policy as an argument to
> em_dev_register_perf_domain() (*) ?
Yes.
> > +static void hybrid_register_all_perf_domains(void)
> > +{
> > + enum hybrid_cpu_type type;
> > +
> > + for (type = HYBRID_PCORE; type < HYBRID_NR_TYPES; type++)
> > + hybrid_register_perf_domain(&perf_domains[type]);
> > +}
> > +
> > +static void hybrid_add_to_perf_domain(int cpu, enum hybrid_cpu_type type)
> > +{
> > + struct hybrid_em_perf_domain *pd = &perf_domains[type];
> > +
> > + guard(mutex)(&hybrid_capacity_lock);
> > +
> > + if (cpumask_test_cpu(cpu, &pd->cpumask))
> > + return;
> > +
> > + cpumask_set_cpu(cpu, &pd->cpumask);
> > + if (pd->dev)
> > + em_dev_expand_perf_domain(pd->dev, cpu);
> > + else if (hybrid_register_perf_domain(pd))
> > + em_rebuild_perf_domains();
>
> I assume that the 'if' and the 'else if' condition here are only taken
> when the CPU is brought online after boot?
Yes.
Powered by blists - more mailing lists