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] [thread-next>] [day] [month] [year] [list]

Message-ID: <c3eda6eb-3e01-4d9c-bcbc-348e5f5552cc@arm.com>
Date: Thu, 1 May 2025 14:30:07 +0200
From: Dietmar Eggemann <dietmar.eggemann@....com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
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>,
 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 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);
}

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.

Anyway, you can add my:

Reviewed-by: Dietmar Eggemann <dietmar.eggemann@....com>

for the entire set.