[AMD Official Use Only - General] Hi Peter: > -----Original Message----- > From: Peter Zijlstra <peterz@xxxxxxxxxxxxx> > Sent: Saturday, October 14, 2023 12:01 AM > To: Meng, Li (Jassmine) <Li.Meng@xxxxxxx> > Cc: Rafael J . Wysocki <rafael.j.wysocki@xxxxxxxxx>; 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 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. > > > +}