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

[Meng, Li (Jassmine)]

[AMD Official Use Only - General]

Hi Peter:

After our internal discussion, the following modifications will be made.
Do you think they are feasible?
        1. Add judgement for "highest_perf".  When it is less than 255, the preferred core feature is enabled. And it will set the priority.
        2. Delete "static u32 max_highset_perf/min_highest_perf", because amd p-state preferred core does not require special processing for hotplug.

+#define CPPC_MAX_PERF  U8_MAX
+
+static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
+{
+       int ret, prio;
+       u32 highest_perf;
+
+       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 < CPPC_MAX_PERF)
+               prio = (int)highest_perf;
+       else {
+               pr_debug("AMD CPPC preferred core is unsupported!\n");
+               cpudata->hw_prefcore = false;
+               return;
+       }
+
+       if (!amd_pstate_prefcore)
+               return;
+
+       /*
+        * 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);
+
+       schedule_work(&sched_prefcore_work);
+}

> -----Original Message-----
> From: srinivas pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Sent: Tuesday, October 17, 2023 2:51 AM
> To: Wysocki, Rafael J <rafael.j.wysocki@xxxxxxxxx>; Peter Zijlstra
> <peterz@xxxxxxxxxxxxx>; Meng, Li (Jassmine) <Li.Meng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; linux-pm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; Shuah
> Khan <skhan@xxxxxxxxxxxxxxxxxxx>; linux-kselftest@xxxxxxxxxxxxxxx;
> Fontenot, Nathan <Nathan.Fontenot@xxxxxxx>; Sharma, Deepak
> <Deepak.Sharma@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Limonciello, Mario
> <Mario.Limonciello@xxxxxxx>; Huang, Shimmer
> <Shimmer.Huang@xxxxxxx>; Yuan, Perry <Perry.Yuan@xxxxxxx>; Du,
> Xiaojian <Xiaojian.Du@xxxxxxx>; Viresh Kumar <viresh.kumar@xxxxxxxxxx>;
> Borislav Petkov <bp@xxxxxxxxx>; Oleksandr Natalenko
> <oleksandr@xxxxxxxxxxxxxx>; Karny, Wyes <Wyes.Karny@xxxxxxx>
> Subject: Re: [RESEND PATCH V9 3/7] cpufreq: amd-pstate: Enable amd-
> pstate preferred core supporting.
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On Mon, 2023-10-16 at 19:27 +0200, Wysocki, Rafael J wrote:
> > On 10/16/2023 12:58 PM, Peter Zijlstra wrote:
> > > On Mon, Oct 16, 2023 at 06:20:53AM +0000, Meng, Li (Jassmine)
> > > wrote:
> > > > > > +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;
> > > > >
> > > > [Meng, Li (Jassmine)]
> > > > We use ITMT architecture to utilize preferred core features.
> > > > Therefore, we need to try to be consistent with Intel's
> > > > implementation as much as possible.  For details, please refer to
> > > > the intel_pstate_set_itmt_prio function in file intel_pstate.c.
> > > > (Line
> > > > 355)
> > > >
> > > > I think using the data type of u32 is consistent with the data
> > > > structures of cppc_perf_ctrls and amd_cpudata etc.
> > > Rafael, should we fix intel_pstate too?
> >
> > Srinivas should be more familiar with this code than I am, so adding
> > him.
> >
> If we make
>         static u32 max_highest_perf = 0, min_highest_perf = U32_MAX; to
>         static int max_highest_perf = INT_MIN, min_highest_perf = INT_MAX;
>
> Then in intel_pstate we will compare signed vs unsigned comparison as
> cppc_perf.highest_perf is u32.
>
>
> In reality this will be fine to change to "int" as we will never reach
> u32 max as performance on any Intel platform.
>
> >
> > > The point is, that sched_asym_prefer(), the final consumer of these
> > > values uses int and thus an explicitly signed compare.
> > >
> > > Using u32 and U32_MAX anywhere near the setting the priority makes
> > > absolutely no sense.
> > >
> > > If you were to have the high bit set, things do not behave as
> > > expected.
> >
> > Right, but in practice these values are always between 0 and 255
> > inclusive AFAICS.
> >
> > It would have been better to use u8 I suppose.
> Should be fine as over clocked parts will set to max 0xff.
>
> >
> >
> > > Also, same question as to the amd folks; what serializes those
> > > static variables?
> >
> > That's a good one.
>
> This function which is checking static variables is called from cpufreq
> ->init callback. Which in turn is called from a function which is
> passed as startup() function pointer to
> cpuhp_setup_state_nocalls_cpuslocked().
>
> I see that startup() callbacks are called under a mutex cpuhp_state_mutex
> for each present CPUs. So if some tear down happen, that is also protected
> by the same mutex. The assumption is here is that cpuhp_invoke_callback()
> in hotplug state machine is not called in parallel on two CPUs by the hotplug
> state machine. But I see activity on parallel bringup, so this is questionable
> now.
>
> Thanks,
> Srinivas
>
> >
> >