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