Re: [PATCH] cpufreq/amd-pstate: Enable ITMT support after initializing core rankings

[Mario Limonciello <mario.limonciello@amd.com>]

On 4/8/2025 10:00 PM, K Prateek Nayak wrote:
> When working on dynamic asym priority support, it was observed that
> "asym_prefer_cpu" on AMD systems supporting Preferred Core ranking
> was always the first CPU in the sched group after boot even though it
> was not the CPU with the highest asym priority.
> 
> "asym_prefer_cpu" is cached when the sched domain hierarchy is
> constructed. On AMD systems that support Preferred Core rankings, sched
> domains are rebuilt when ITMT support is enabled for the first time from
> amd_pstate*_cpu_init().
> 
> Since amd_pstate*_cpu_init() is called to initialize the cpudata for
> each CPU, the ITMT support is enabled after the first CPU initializes
> its asym priority but this is too early since other CPUs have not yet
> initialized their asym priorities and the sched domain is rebuilt when
> the ITMT support is toggled on for the first time.
> 
> Initialize the asym priorities first in amd_pstate*_cpu_init() and then
> enable ITMT support only after amd_pstate_register_driver() is finished
> to ensure all CPUs have correctly initialized their asym priorities
> before sched domain hierarchy is rebuilt and "asym_prefer_cpu" is
> cached.
> 
> Remove the delayed work mechanism now that ITMT support is only toggled
> from the driver init path which is outside the cpuhp critical section.
> 
> Fixes: f3a052391822 ("cpufreq: amd-pstate: Enable amd-pstate preferred core support")
> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 28 ++++++++--------------------
>   1 file changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index c54c031939c8..ee638589f5f9 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -794,19 +794,9 @@ static void amd_perf_ctl_reset(unsigned int cpu)
>   	wrmsrl_on_cpu(cpu, MSR_AMD_PERF_CTL, 0);
>   }
>   
> -/*
> - * Set amd-pstate preferred core enable can't be done directly from cpufreq callbacks
> - * due to locking, so queue the work for later.
> - */
> -static void amd_pstste_sched_prefcore_workfn(struct work_struct *work)
> -{
> -	sched_set_itmt_support();
> -}
> -static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);
> -
>   #define CPPC_MAX_PERF	U8_MAX
>   
> -static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
> +static void amd_pstate_init_asym_prio(struct amd_cpudata *cpudata)

I think the previous function name was fine.

1) It still does set cpudata->hw_prefcore afterall and
2) We still have an amd_detect_prefcore() that is used to determine 
whether amd_pstate_prefcore is set.

>   {
>   	/* user disabled or not detected */
>   	if (!amd_pstate_prefcore)
> @@ -814,14 +804,8 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
>   
>   	cpudata->hw_prefcore = true;
>   
> -	/*
> -	 * 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.
> -	 */
> +	/* The priorities must be initialized before ITMT support can be toggled on. */
>   	sched_set_itmt_core_prio((int)READ_ONCE(cpudata->prefcore_ranking), cpudata->cpu);
> -
> -	schedule_work(&sched_prefcore_work);
>   }
>   
>   static void amd_pstate_update_limits(unsigned int cpu)
> @@ -974,7 +958,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>   	if (ret)
>   		goto free_cpudata1;
>   
> -	amd_pstate_init_prefcore(cpudata);
> +	amd_pstate_init_asym_prio(cpudata);
>   
>   	ret = amd_pstate_init_freq(cpudata);
>   	if (ret)
> @@ -1450,7 +1434,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>   	if (ret)
>   		goto free_cpudata1;
>   
> -	amd_pstate_init_prefcore(cpudata);
> +	amd_pstate_init_asym_prio(cpudata);
>   
>   	ret = amd_pstate_init_freq(cpudata);
>   	if (ret)
> @@ -1780,6 +1764,10 @@ static int __init amd_pstate_init(void)
>   		}
>   	}
>   
> +	/* Enable ITMT support once all CPUs have initialized their asym priorities. */
> +	if (amd_pstate_prefcore)
> +		sched_set_itmt_support();
> +

Hmm, by moving it after the first registration that has the side effect 
that if you changed driver modes from active to passive (for example) 
ITMT priorities stay identical and aren't updated.
I guess that makes sense since the rankings /shouldn't/ change.

I feel this should be OK, thanks.

>   	return ret;
>   
>   global_attr_free:
> 
> base-commit: 56a49e19e1aea1374e9ba58cfd40260587bb7355