linux-kernel - Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw

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: <Zs6w3RktAb6fJrJ+@BLRRASHENOY1.amd.com>
Date: Wed, 28 Aug 2024 10:38:45 +0530
From: "Gautham R. Shenoy" <gautham.shenoy@....com>
To: Mario Limonciello <mario.limonciello@....com>
Cc: Mario Limonciello <superm1@...nel.org>, Borislav Petkov <bp@...en8.de>,
	Perry Yuan <perry.yuan@....com>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	"open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <linux-kernel@...r.kernel.org>,
	"open list:ACPI" <linux-acpi@...r.kernel.org>,
	"open list:CPU FREQUENCY SCALING FRAMEWORK" <linux-pm@...r.kernel.org>,
	changwoo@...lia.com, David Vernet <void@...ifault.com>,
	Tejun Heo <tj@...nel.org>, righi.andrea@...il.com
Subject: Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of
 cpudata->hw_prefcore

On Tue, Aug 27, 2024 at 02:16:51PM -0500, Mario Limonciello wrote:
> On 8/27/2024 12:03, Gautham R. Shenoy wrote:
> > On Mon, Aug 26, 2024 at 04:13:58PM -0500, Mario Limonciello wrote:
> > > From: Mario Limonciello <mario.limonciello@....com>
> > > 
> > > As the global variable is cleared when preferred cores is not present
> > > the local variable use isn't needed in many functions.
> > > Drop it where possible.  No intended functional changes.
> > > 
> > > Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> > > ---
> > >   drivers/cpufreq/amd-pstate.c | 7 +------
> > >   1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > > index ed05d7a0add10..257e28e549bd1 100644
> > > --- a/drivers/cpufreq/amd-pstate.c
> > > +++ b/drivers/cpufreq/amd-pstate.c
> > > @@ -1112,12 +1112,7 @@ static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,
> > >   static ssize_t show_amd_pstate_hw_prefcore(struct cpufreq_policy *policy,
> > >   					   char *buf)
> > >   {
> > > -	bool hw_prefcore;
> > > -	struct amd_cpudata *cpudata = policy->driver_data;
> > > -
> > > -	hw_prefcore = READ_ONCE(cpudata->hw_prefcore);
> > > -
> > > -	return sysfs_emit(buf, "%s\n", str_enabled_disabled(hw_prefcore));
> > > +	return sysfs_emit(buf, "%s\n", str_enabled_disabled(amd_pstate_prefcore));
> > 
> > 
> > If the user boots with "amd_prefcore=disable" on a system that
> > supports preferred-core, pror to this patchset, cpudata->hw_prefcore
> > would be true and thus, amd_pstate_hw_prefcore would show "enabled".
> > 
> 
> Yes; you're right about the previous behavior.
> 
> > With this patchset, it would show "disabled". Or am I missing something?
> 
> This appears to be another case we don't have documentation for the sysfs
> file `amd_pstate_hw_prefcore`.

:(

> 
> I had thought this was a malfunction in the behavior that it reflected the
> current status, not the hardware /capability/.
> 
> Which one makes more sense for userspace?  In my mind the most likely
> consumer of this information would be something a sched_ext based userspace
> scheduler.  They would need to know whether the scheduler was using
> preferred cores; not whether the hardware supported it.

The commandline parameter currently impacts only the fair sched-class
tasks since the preference information gets used only during
load-balancing.

IMO, the same should continue with sched-ext, i.e. if the user has
explicitly disabled prefcore support via commandline, the no sched-ext
scheduler should use the preference information to make task placement
decisions. However, I would like to see what the sched-ext folks have
to say. Adding some of them to the Cc list.

> 
> Whichever direction we agree to go; I'll add documentation for v2.

Yes please.

--
Thanks and Regards
gautham.