linux-kernel - Re: [RFT][PATCH v1 5/8] PM: EM: Introduce em_adjust_cpu

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJZ5v0jrnF+k81nCHEKvHa-SQp8J_iUkvW+jFo8ZHsj3AcG2vg@mail.gmail.com>
Date: Tue, 6 May 2025 21:46:15 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Dietmar Eggemann <dietmar.eggemann@....com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, "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>, 
	Morten Rasmussen <morten.rasmussen@....com>, Vincent Guittot <vincent.guittot@...aro.org>, 
	Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>, 
	Pierre Gondois <pierre.gondois@....com>, Christian Loehle <christian.loehle@....com>
Subject: Re: [RFT][PATCH v1 5/8] PM: EM: Introduce em_adjust_cpu_capacity()

On Thu, May 1, 2025 at 2:30 PM Dietmar Eggemann
<dietmar.eggemann@....com> wrote:
>
> On 30/04/2025 21:23, Rafael J. Wysocki wrote:
> > On Sun, Apr 27, 2025 at 4:07 PM Dietmar Eggemann
> > <dietmar.eggemann@....com> wrote:
> >>
> >> On 16/04/2025 20:06, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> [...]
>
> >>> +     if (!(pd->flags & EM_PERF_DOMAIN_ARTIFICIAL)) {
> >>
> >> This looks weird to me. How can an artificial EM ever have a non-ZERO
> >> em_data_callback here?
> >>
> >> There is already EM_PERF_DOMAIN_ARTIFICIAL specific handling in
> >> em_compute_costs(). Which probably works well for the
> >> em_create_perf_table() call-site.
> >
> > Yes, but that one doesn't pass a NULL cb pointer to it.
> >
> >> Will there be cases for Hybrid CPU EM's in which 'em_max_perf !=
> >> cpu_capacity':
> >
> > When the capacity is updated, the EM needs to be updated accordingly,
> > which is why the new function is being added.
> >
> >> em_adjust_new_capacity()
> >>
> >>   if (em_max_perf == cpu_capacity)
> >>     return
> >>
> >>   em_recalc_and_update()
> >>     em_compute_costs()
> >>
> >> so that em_compute_costs() might be called?
> >>
> >> Maybe:
> >>
> >> @@ -233,11 +237,17 @@ static int em_compute_costs(struct device *dev,
> >> struct em_perf_state *table,
> >>         unsigned long prev_cost = ULONG_MAX;
> >>         int i, ret;
> >>
> >> +       if (!cb && (flags & EM_PERF_DOMAIN_ARTIFICIAL))
> >> +               return 0;
> >>
> >> is somehow clearer in this case?
> >
> > This would work, but I prefer my version because it does one check
> > less and it does the check directly in em_recalc_and_update(), so it
> > is clear that this doesn't call em_compute_costs() for artificial PDs
> > at all.
>
> OK, but checking it inside em_compute_costs() would also avoid this 'cb
> = NULL' crash for an artificial EM in:
>
> int em_dev_compute_costs(struct device *dev, struct em_perf_state
>                          *table, int nr_states)
> {
>         return em_compute_costs(dev, table, NULL, nr_states, 0);
> }

This is unused currently, so no worries, but you have a point.  It
should return -EINVAL for artificial perf domains.

>
> BTW, there is this:
>
> #define em_is_artificial(em) ((em)->flags & EM_PERF_DOMAIN_ARTIFICIAL)
>
> (I guess s/em/pd ?) which lets you check this when you have the perf
> domain. So far it's used in dtpm, cpu- and devfreq cooling.

Thanks for letting me know about it, I'll use it in that check.

> Anyway, you can add my:
>
> Reviewed-by: Dietmar Eggemann <dietmar.eggemann@....com>
>
> for the entire set.

Thank you!