[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 <[email protected]>
> Sent: Tuesday, October 17, 2023 2:51 AM
> To: Wysocki, Rafael J <[email protected]>; Peter Zijlstra
> <[email protected]>; Meng, Li (Jassmine) <[email protected]>
> Cc: Huang, Ray <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; [email protected]; Shuah
> Khan <[email protected]>; [email protected];
> Fontenot, Nathan <[email protected]>; Sharma, Deepak
> <[email protected]>; Deucher, Alexander
> <[email protected]>; Limonciello, Mario
> <[email protected]>; Huang, Shimmer
> <[email protected]>; Yuan, Perry <[email protected]>; Du,
> Xiaojian <[email protected]>; Viresh Kumar <[email protected]>;
> Borislav Petkov <[email protected]>; Oleksandr Natalenko
> <[email protected]>; Karny, Wyes <[email protected]>
> 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
>
> >
> >