linux-kernel - Re: [RESEND PATCH V9 3/7] cpufreq: amd-pstate: Enable amd-pstate preferred core supporting.

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: <20231013160128.GB36211@noisy.programming.kicks-ass.net>
Date:   Fri, 13 Oct 2023 18:01:28 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Meng Li <li.meng@....com>
Cc:     "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Huang Rui <ray.huang@....com>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        linux-acpi@...r.kernel.org, Shuah Khan <skhan@...uxfoundation.org>,
        linux-kselftest@...r.kernel.org,
        Nathan Fontenot <nathan.fontenot@....com>,
        Deepak Sharma <deepak.sharma@....com>,
        Alex Deucher <alexander.deucher@....com>,
        Mario Limonciello <mario.limonciello@....com>,
        Shimmer Huang <shimmer.huang@....com>,
        Perry Yuan <Perry.Yuan@....com>,
        Xiaojian Du <Xiaojian.Du@....com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Borislav Petkov <bp@...en8.de>,
        Oleksandr Natalenko <oleksandr@...alenko.name>,
        Wyes Karny <wyes.karny@....com>
Subject: Re: [RESEND PATCH V9 3/7] cpufreq: amd-pstate: Enable amd-pstate
 preferred core supporting.

On Fri, Oct 13, 2023 at 11:31:14AM +0800, Meng Li wrote:

> +#define AMD_PSTATE_PREFCORE_THRESHOLD	166
> +#define AMD_PSTATE_MAX_CPPC_PERF	255

> +static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
> +{
> +	int ret, prio;
> +	u32 highest_perf;
> +	static u32 max_highest_perf = 0, min_highest_perf = U32_MAX;

What serializes these things?

Also, *why* are you using u32 here, what's wrong with something like:

	int max_hp = INT_MIN, min_hp = INT_MAX;

> +
> +	ret = amd_pstate_get_highest_perf(cpudata->cpu, &highest_perf);
> +	if (ret)
> +		return;
> +
> +	cpudata->hw_prefcore = true;
> +	/* check if CPPC preferred core feature is enabled*/
> +	if (highest_perf == AMD_PSTATE_MAX_CPPC_PERF) {

Which effectively means <255 (also, seems to suggest MAX_CPPC_PERF might
not be the best name, hmm?)

Should you not write '>= 255' then? Just in case something 'funny'
happens?

> +		pr_debug("AMD CPPC preferred core is unsupported!\n");
> +		cpudata->hw_prefcore = false;
> +		return;
> +	}
> +
> +	if (!amd_pstate_prefcore)
> +		return;
> +
> +	/* The maximum value of highest perf is 255 */
> +	prio = (int)(highest_perf & 0xff);

If for some weird reason you get 0x1ff or whatever above (dodgy BIOS
never happens, right) then this makes sense how?

Perhaps stop sending patches at break-nek speed and think for a little
while on how to write this and not be confused?


> +	/*
> +	 * The priorities can be set regardless of whether or not
> +	 * sched_set_itmt_support(true) has been called and it is valid to
> +	 * update them at any time after it has been called.
> +	 */
> +	sched_set_itmt_core_prio(prio, cpudata->cpu);
> +
> +	if (max_highest_perf <= min_highest_perf) {
> +		if (highest_perf > max_highest_perf)
> +			max_highest_perf = highest_perf;
> +
> +		if (highest_perf < min_highest_perf)
> +			min_highest_perf = highest_perf;
> +
> +		if (max_highest_perf > min_highest_perf) {
> +			/*
> +			 * This code can be run during CPU online under the
> +			 * CPU hotplug locks, so sched_set_itmt_support()
> +			 * cannot be called from here.  Queue up a work item
> +			 * to invoke it.
> +			 */
> +			schedule_work(&sched_prefcore_work);
> +		}
> +	}

Not a word about what serializes these variables.

> +}