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

[Gautham R. Shenoy]

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@xxxxxxx>
> > > 
> > > 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@xxxxxxx>
> > > ---
> > >   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.