Messages in this thread | | | Date | Fri, 13 Oct 2023 18:01:28 +0200 | From | Peter Zijlstra <> | Subject | Re: [RESEND PATCH V9 3/7] cpufreq: amd-pstate: Enable amd-pstate preferred core supporting. |
| |
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;
> + > + 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?
> + 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?
> + /* > + * 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.
> +}
| |