diff mbox series

[v2,13/16] cpufreq/amd-pstate: Check if CPPC request has changed before writing to the MSR or shared memory

Message ID 20241208063031.3113-14-mario.limonciello@amd.com (mailing list archive)
State Superseded, archived
Headers show
Series amd-pstate fixes and improvements for 6.14 | expand

Commit Message

Mario Limonciello Dec. 8, 2024, 6:30 a.m. UTC 
Move the common MSR field formatting code to msr_update_perf() from
its callers.

Ensure that the MSR write is necessary before flushing a write out.
Also drop the comparison from the passive flow tracing.

Reviewed-and-tested-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate-trace.h |  7 +----
 drivers/cpufreq/amd-pstate.c       | 47 +++++++++++-------------------
 2 files changed, 18 insertions(+), 36 deletions(-)

Comments

Gautham R. Shenoy Dec. 9, 2024, 8:56 a.m. UTC | #1 
Hello Mario,


On Sun, Dec 08, 2024 at 12:30:28AM -0600, Mario Limonciello wrote:
> Move the common MSR field formatting code to msr_update_perf() from
> its callers.
> 
> Ensure that the MSR write is necessary before flushing a write out.
> Also drop the comparison from the passive flow tracing.
> 
> Reviewed-and-tested-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---

[..snip..]

> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index dd11ba6c00cc3..2178931fbf87b 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -224,15 +224,26 @@ static s16 shmem_get_epp(struct amd_cpudata *cpudata)
>  static int msr_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
>  			   u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
>  {
> -	u64 value;
> +	u64 value, prev;
> +
> +	value = prev = READ_ONCE(cpudata->cppc_req_cached);
> +
> +	value &= ~(AMD_CPPC_MAX_PERF_MASK | AMD_CPPC_MIN_PERF_MASK |
> +		   AMD_CPPC_DES_PERF_MASK | AMD_CPPC_EPP_PERF_MASK);
> +	value |= FIELD_PREP(AMD_CPPC_MAX_PERF_MASK, max_perf);
> +	value |= FIELD_PREP(AMD_CPPC_DES_PERF_MASK, des_perf);
> +	value |= FIELD_PREP(AMD_CPPC_MIN_PERF_MASK, min_perf);
> +	value |= FIELD_PREP(AMD_CPPC_EPP_PERF_MASK, epp);
> +
> +	if (value == prev)
> +		return 0;
>  
> -	value = READ_ONCE(cpudata->cppc_req_cached);
>  	if (fast_switch) {
> -		wrmsrl(MSR_AMD_CPPC_REQ, READ_ONCE(cpudata->cppc_req_cached));
> +		wrmsrl(MSR_AMD_CPPC_REQ, value);
>  		return 0;
>  	} else {
> -		int ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
> -					READ_ONCE(cpudata->cppc_req_cached));
> +		int ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> +
>  		if (ret)
>  			return ret;

Ok, so you are recomputing the value in this patch. Does it also make
sense to move trace_amd_pstate_perf() call to this place?


--
Thanks and Regards
gautham.

>  	}
> @@ -528,9 +539,7 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>  {
>  	unsigned long max_freq;
>  	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
> -	u64 prev = READ_ONCE(cpudata->cppc_req_cached);
>  	u32 nominal_perf = READ_ONCE(cpudata->nominal_perf);
> -	u64 value = prev;
>  
>  	des_perf = clamp_t(unsigned long, des_perf, min_perf, max_perf);
>  
> @@ -546,27 +555,14 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>  	if (!cpudata->boost_supported)
>  		max_perf = min_t(unsigned long, nominal_perf, max_perf);
>  
> -	value &= ~(AMD_CPPC_MAX_PERF_MASK | AMD_CPPC_MIN_PERF_MASK |
> -		   AMD_CPPC_DES_PERF_MASK);
> -	value |= FIELD_PREP(AMD_CPPC_MAX_PERF_MASK, max_perf);
> -	value |= FIELD_PREP(AMD_CPPC_DES_PERF_MASK, des_perf);
> -	value |= FIELD_PREP(AMD_CPPC_MIN_PERF_MASK, min_perf);
> -
>  	if (trace_amd_pstate_perf_enabled() && amd_pstate_sample(cpudata)) {
>  		trace_amd_pstate_perf(min_perf, des_perf, max_perf, cpudata->freq,
>  			cpudata->cur.mperf, cpudata->cur.aperf, cpudata->cur.tsc,
> -				cpudata->cpu, (value != prev), fast_switch);
> +				cpudata->cpu, fast_switch);
>  	}
>  
> -	if (value == prev)
> -		goto cpufreq_policy_put;
> -
> -	WRITE_ONCE(cpudata->cppc_req_cached, value);
> -
>  	amd_pstate_update_perf(cpudata, min_perf, des_perf, max_perf, 0, fast_switch);
>  
> -cpufreq_policy_put:
> -
>  	cpufreq_cpu_put(policy);
>  }
>  
> @@ -1562,19 +1558,10 @@ static void amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
>  static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>  {
>  	struct amd_cpudata *cpudata = policy->driver_data;
> -	u64 value;
>  	u32 epp;
>  
>  	amd_pstate_update_min_max_limit(policy);
>  
> -	value = READ_ONCE(cpudata->cppc_req_cached);
> -
> -	value &= ~(AMD_CPPC_MAX_PERF_MASK | AMD_CPPC_MIN_PERF_MASK |
> -		   AMD_CPPC_DES_PERF_MASK | AMD_CPPC_EPP_PERF_MASK);
> -	value |= FIELD_PREP(AMD_CPPC_MAX_PERF_MASK, cpudata->max_limit_perf);
> -	value |= FIELD_PREP(AMD_CPPC_DES_PERF_MASK, 0);
> -	value |= FIELD_PREP(AMD_CPPC_MIN_PERF_MASK, cpudata->min_limit_perf);
> -
>  	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>  		epp = 0;
>  	else
> -- 
> 2.43.0
>
Mario Limonciello Dec. 9, 2024, 4:41 p.m. UTC | #2 
On 12/9/2024 02:56, Gautham R. Shenoy wrote:
> Hello Mario,
> 
> 
> On Sun, Dec 08, 2024 at 12:30:28AM -0600, Mario Limonciello wrote:
>> Move the common MSR field formatting code to msr_update_perf() from
>> its callers.
>>
>> Ensure that the MSR write is necessary before flushing a write out.
>> Also drop the comparison from the passive flow tracing.
>>
>> Reviewed-and-tested-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
> 
> [..snip..]
> 
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index dd11ba6c00cc3..2178931fbf87b 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -224,15 +224,26 @@ static s16 shmem_get_epp(struct amd_cpudata *cpudata)
>>   static int msr_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
>>   			   u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
>>   {
>> -	u64 value;
>> +	u64 value, prev;
>> +
>> +	value = prev = READ_ONCE(cpudata->cppc_req_cached);
>> +
>> +	value &= ~(AMD_CPPC_MAX_PERF_MASK | AMD_CPPC_MIN_PERF_MASK |
>> +		   AMD_CPPC_DES_PERF_MASK | AMD_CPPC_EPP_PERF_MASK);
>> +	value |= FIELD_PREP(AMD_CPPC_MAX_PERF_MASK, max_perf);
>> +	value |= FIELD_PREP(AMD_CPPC_DES_PERF_MASK, des_perf);
>> +	value |= FIELD_PREP(AMD_CPPC_MIN_PERF_MASK, min_perf);
>> +	value |= FIELD_PREP(AMD_CPPC_EPP_PERF_MASK, epp);
>> +
>> +	if (value == prev)
>> +		return 0;
>>   
>> -	value = READ_ONCE(cpudata->cppc_req_cached);
>>   	if (fast_switch) {
>> -		wrmsrl(MSR_AMD_CPPC_REQ, READ_ONCE(cpudata->cppc_req_cached));
>> +		wrmsrl(MSR_AMD_CPPC_REQ, value);
>>   		return 0;
>>   	} else {
>> -		int ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
>> -					READ_ONCE(cpudata->cppc_req_cached));
>> +		int ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
>> +
>>   		if (ret)
>>   			return ret;
> 
> Ok, so you are recomputing the value in this patch. Does it also make
> sense to move trace_amd_pstate_perf() call to this place?

You mean so that essentially it only logs events that REALLY write to 
the MSR/shared memory region?

I guess with an ftrace function call graph + that event you can 
effectively determine that is what happened.

An alternative is to move it right above the (value == prev) check and 
include "prev" in the trace event.

I think that's my preference at least - you capture whether it changed 
and what the intent was.

> 
> 
> --
> Thanks and Regards
> gautham.
> 
>>   	}
>> @@ -528,9 +539,7 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>>   {
>>   	unsigned long max_freq;
>>   	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
>> -	u64 prev = READ_ONCE(cpudata->cppc_req_cached);
>>   	u32 nominal_perf = READ_ONCE(cpudata->nominal_perf);
>> -	u64 value = prev;
>>   
>>   	des_perf = clamp_t(unsigned long, des_perf, min_perf, max_perf);
>>   
>> @@ -546,27 +555,14 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>>   	if (!cpudata->boost_supported)
>>   		max_perf = min_t(unsigned long, nominal_perf, max_perf);
>>   
>> -	value &= ~(AMD_CPPC_MAX_PERF_MASK | AMD_CPPC_MIN_PERF_MASK |
>> -		   AMD_CPPC_DES_PERF_MASK);
>> -	value |= FIELD_PREP(AMD_CPPC_MAX_PERF_MASK, max_perf);
>> -	value |= FIELD_PREP(AMD_CPPC_DES_PERF_MASK, des_perf);
>> -	value |= FIELD_PREP(AMD_CPPC_MIN_PERF_MASK, min_perf);
>> -
>>   	if (trace_amd_pstate_perf_enabled() && amd_pstate_sample(cpudata)) {
>>   		trace_amd_pstate_perf(min_perf, des_perf, max_perf, cpudata->freq,
>>   			cpudata->cur.mperf, cpudata->cur.aperf, cpudata->cur.tsc,
>> -				cpudata->cpu, (value != prev), fast_switch);
>> +				cpudata->cpu, fast_switch);
>>   	}
>>   
>> -	if (value == prev)
>> -		goto cpufreq_policy_put;
>> -
>> -	WRITE_ONCE(cpudata->cppc_req_cached, value);
>> -
>>   	amd_pstate_update_perf(cpudata, min_perf, des_perf, max_perf, 0, fast_switch);
>>   
>> -cpufreq_policy_put:
>> -
>>   	cpufreq_cpu_put(policy);
>>   }
>>   
>> @@ -1562,19 +1558,10 @@ static void amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
>>   static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>>   {
>>   	struct amd_cpudata *cpudata = policy->driver_data;
>> -	u64 value;
>>   	u32 epp;
>>   
>>   	amd_pstate_update_min_max_limit(policy);
>>   
>> -	value = READ_ONCE(cpudata->cppc_req_cached);
>> -
>> -	value &= ~(AMD_CPPC_MAX_PERF_MASK | AMD_CPPC_MIN_PERF_MASK |
>> -		   AMD_CPPC_DES_PERF_MASK | AMD_CPPC_EPP_PERF_MASK);
>> -	value |= FIELD_PREP(AMD_CPPC_MAX_PERF_MASK, cpudata->max_limit_perf);
>> -	value |= FIELD_PREP(AMD_CPPC_DES_PERF_MASK, 0);
>> -	value |= FIELD_PREP(AMD_CPPC_MIN_PERF_MASK, cpudata->min_limit_perf);
>> -
>>   	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>>   		epp = 0;
>>   	else
>> -- 
>> 2.43.0
>>
diff mbox series

Patch

diff --git a/drivers/cpufreq/amd-pstate-trace.h b/drivers/cpufreq/amd-pstate-trace.h
index e2221a4b6901c..8d692415d9050 100644
--- a/drivers/cpufreq/amd-pstate-trace.h
+++ b/drivers/cpufreq/amd-pstate-trace.h
@@ -32,7 +32,6 @@  TRACE_EVENT(amd_pstate_perf,
 		 u64 aperf,
 		 u64 tsc,
 		 unsigned int cpu_id,
-		 bool changed,
 		 bool fast_switch
 		 ),
 
@@ -44,7 +43,6 @@  TRACE_EVENT(amd_pstate_perf,
 		aperf,
 		tsc,
 		cpu_id,
-		changed,
 		fast_switch
 		),
 
@@ -57,7 +55,6 @@  TRACE_EVENT(amd_pstate_perf,
 		__field(unsigned long long, aperf)
 		__field(unsigned long long, tsc)
 		__field(unsigned int, cpu_id)
-		__field(bool, changed)
 		__field(bool, fast_switch)
 		),
 
@@ -70,11 +67,10 @@  TRACE_EVENT(amd_pstate_perf,
 		__entry->aperf = aperf;
 		__entry->tsc = tsc;
 		__entry->cpu_id = cpu_id;
-		__entry->changed = changed;
 		__entry->fast_switch = fast_switch;
 		),
 
-	TP_printk("amd_min_perf=%lu amd_des_perf=%lu amd_max_perf=%lu freq=%llu mperf=%llu aperf=%llu tsc=%llu cpu_id=%u changed=%s fast_switch=%s",
+	TP_printk("amd_min_perf=%lu amd_des_perf=%lu amd_max_perf=%lu freq=%llu mperf=%llu aperf=%llu tsc=%llu cpu_id=%u fast_switch=%s",
 		  (unsigned long)__entry->min_perf,
 		  (unsigned long)__entry->target_perf,
 		  (unsigned long)__entry->capacity,
@@ -83,7 +79,6 @@  TRACE_EVENT(amd_pstate_perf,
 		  (unsigned long long)__entry->aperf,
 		  (unsigned long long)__entry->tsc,
 		  (unsigned int)__entry->cpu_id,
-		  (__entry->changed) ? "true" : "false",
 		  (__entry->fast_switch) ? "true" : "false"
 		 )
 );
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index dd11ba6c00cc3..2178931fbf87b 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -224,15 +224,26 @@  static s16 shmem_get_epp(struct amd_cpudata *cpudata)
 static int msr_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
 			   u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
 {
-	u64 value;
+	u64 value, prev;
+
+	value = prev = READ_ONCE(cpudata->cppc_req_cached);
+
+	value &= ~(AMD_CPPC_MAX_PERF_MASK | AMD_CPPC_MIN_PERF_MASK |
+		   AMD_CPPC_DES_PERF_MASK | AMD_CPPC_EPP_PERF_MASK);
+	value |= FIELD_PREP(AMD_CPPC_MAX_PERF_MASK, max_perf);
+	value |= FIELD_PREP(AMD_CPPC_DES_PERF_MASK, des_perf);
+	value |= FIELD_PREP(AMD_CPPC_MIN_PERF_MASK, min_perf);
+	value |= FIELD_PREP(AMD_CPPC_EPP_PERF_MASK, epp);
+
+	if (value == prev)
+		return 0;
 
-	value = READ_ONCE(cpudata->cppc_req_cached);
 	if (fast_switch) {
-		wrmsrl(MSR_AMD_CPPC_REQ, READ_ONCE(cpudata->cppc_req_cached));
+		wrmsrl(MSR_AMD_CPPC_REQ, value);
 		return 0;
 	} else {
-		int ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
-					READ_ONCE(cpudata->cppc_req_cached));
+		int ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
+
 		if (ret)
 			return ret;
 	}
@@ -528,9 +539,7 @@  static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
 {
 	unsigned long max_freq;
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
-	u64 prev = READ_ONCE(cpudata->cppc_req_cached);
 	u32 nominal_perf = READ_ONCE(cpudata->nominal_perf);
-	u64 value = prev;
 
 	des_perf = clamp_t(unsigned long, des_perf, min_perf, max_perf);
 
@@ -546,27 +555,14 @@  static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
 	if (!cpudata->boost_supported)
 		max_perf = min_t(unsigned long, nominal_perf, max_perf);
 
-	value &= ~(AMD_CPPC_MAX_PERF_MASK | AMD_CPPC_MIN_PERF_MASK |
-		   AMD_CPPC_DES_PERF_MASK);
-	value |= FIELD_PREP(AMD_CPPC_MAX_PERF_MASK, max_perf);
-	value |= FIELD_PREP(AMD_CPPC_DES_PERF_MASK, des_perf);
-	value |= FIELD_PREP(AMD_CPPC_MIN_PERF_MASK, min_perf);
-
 	if (trace_amd_pstate_perf_enabled() && amd_pstate_sample(cpudata)) {
 		trace_amd_pstate_perf(min_perf, des_perf, max_perf, cpudata->freq,
 			cpudata->cur.mperf, cpudata->cur.aperf, cpudata->cur.tsc,
-				cpudata->cpu, (value != prev), fast_switch);
+				cpudata->cpu, fast_switch);
 	}
 
-	if (value == prev)
-		goto cpufreq_policy_put;
-
-	WRITE_ONCE(cpudata->cppc_req_cached, value);
-
 	amd_pstate_update_perf(cpudata, min_perf, des_perf, max_perf, 0, fast_switch);
 
-cpufreq_policy_put:
-
 	cpufreq_cpu_put(policy);
 }
 
@@ -1562,19 +1558,10 @@  static void amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
 static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	u64 value;
 	u32 epp;
 
 	amd_pstate_update_min_max_limit(policy);
 
-	value = READ_ONCE(cpudata->cppc_req_cached);
-
-	value &= ~(AMD_CPPC_MAX_PERF_MASK | AMD_CPPC_MIN_PERF_MASK |
-		   AMD_CPPC_DES_PERF_MASK | AMD_CPPC_EPP_PERF_MASK);
-	value |= FIELD_PREP(AMD_CPPC_MAX_PERF_MASK, cpudata->max_limit_perf);
-	value |= FIELD_PREP(AMD_CPPC_DES_PERF_MASK, 0);
-	value |= FIELD_PREP(AMD_CPPC_MIN_PERF_MASK, cpudata->min_limit_perf);
-
 	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
 		epp = 0;
 	else