[AMD Official Use Only - General] Hi Peter:
> -----Original Message----- > From: Peter Zijlstra <[email protected]> > Sent: Saturday, October 14, 2023 12:01 AM > To: Meng, Li (Jassmine) <[email protected]> > Cc: Rafael J . Wysocki <[email protected]>; 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 Fri, Oct 13, 2023 at 11:31:14AM +0800, Meng Li wrote: > > > +#define AMD_PSTATE_PREFCORE_THRESHOLD 166 > > +#define AMD_PSTATE_MAX_CPPC_PERF 255 > > > +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. > > + > > + 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 == AMD_PSTATE_MAX_CPPC_PERF) { > > Which effectively means <255 (also, seems to suggest MAX_CPPC_PERF > might not be the best name, hmm?) > > Should you not write '>= 255' then? Just in case something 'funny' > happens? > [Meng, Li (Jassmine)] OK, I will modify these. > > + pr_debug("AMD CPPC preferred core is unsupported!\n"); > > + cpudata->hw_prefcore = false; > > + return; > > + } > > + > > + if (!amd_pstate_prefcore) > > + return; > > + > > + /* The maximum value of highest perf is 255 */ > > + prio = (int)(highest_perf & 0xff); > > If for some weird reason you get 0x1ff or whatever above (dodgy BIOS never > happens, right) then this makes sense how? > > Perhaps stop sending patches at break-nek speed and think for a little while > on how to write this and not be confused? > [Meng, Li (Jassmine)] If I use '>= 255' to check, the issue mentioned will not exist. Because it will be returned when highest_perff>0xff. > > > + /* > > + * 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); > > + > > + if (max_highest_perf <= min_highest_perf) { > > + if (highest_perf > max_highest_perf) > > + max_highest_perf = highest_perf; > > + > > + if (highest_perf < min_highest_perf) > > + min_highest_perf = highest_perf; > > + > > + if (max_highest_perf > min_highest_perf) { > > + /* > > + * This code can be run during CPU online under the > > + * CPU hotplug locks, so sched_set_itmt_support() > > + * cannot be called from here. Queue up a work item > > + * to invoke it. > > + */ > > + schedule_work(&sched_prefcore_work); > > + } > > + } > > Not a word about what serializes these variables. > > > +}
