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.
