diff mbox series

[2/3] cpufreq: CPPC: Keep the target core awake when reading its cpufreq rate

Message ID 20231025093847.3740104-3-zengheng4@huawei.com
State New
Headers
Series Make the cpuinfo_cur_freq interface read correctly |

Commit Message

Zeng Heng Oct. 25, 2023, 9:38 a.m. UTC 
  As ARM AMU's document says, all counters are subject to any changes
in clock frequency, including clock stopping caused by the WFI and WFE
instructions.

Therefore, using smp_call_on_cpu() to trigger target CPU to
read self's AMU counters, which ensures the counters are working
properly while cstate feature is enabled.

Reported-by: Sumit Gupta <sumitg@nvidia.com>
Link: https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
 drivers/cpufreq/cppc_cpufreq.c | 39 ++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 9 deletions(-)

Comments

Mark Rutland Oct. 25, 2023, 10:54 a.m. UTC | #1 
[adding Ionela]

On Wed, Oct 25, 2023 at 05:38:46PM +0800, Zeng Heng wrote:
> As ARM AMU's document says, all counters are subject to any changes
> in clock frequency, including clock stopping caused by the WFI and WFE
> instructions.
> 
> Therefore, using smp_call_on_cpu() to trigger target CPU to
> read self's AMU counters, which ensures the counters are working
> properly while cstate feature is enabled.

IIUC there's a pretty deliberate split with all the actual reading of the AMU
living in arch/arm64/kernel/topolgy.c, and the driver code being (relatively)
generic.

We already have code in arch/arm64/kernel/topolgy.c to read counters on a
specific CPU; why can't e reuse that (and avoid exporting cpu_has_amu_feat())?

Mark.

> 
> Reported-by: Sumit Gupta <sumitg@nvidia.com>
> Link: https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 39 ++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index fe08ca419b3d..321a9dc9484d 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -90,6 +90,12 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>  				 struct cppc_perf_fb_ctrs *fb_ctrs_t0,
>  				 struct cppc_perf_fb_ctrs *fb_ctrs_t1);
>  
> +struct fb_ctr_pair {
> +	u32 cpu;
> +	struct cppc_perf_fb_ctrs fb_ctrs_t0;
> +	struct cppc_perf_fb_ctrs fb_ctrs_t1;
> +};
> +
>  /**
>   * cppc_scale_freq_workfn - CPPC arch_freq_scale updater for frequency invariance
>   * @work: The work item.
> @@ -840,9 +846,24 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>  	return (reference_perf * delta_delivered) / delta_reference;
>  }
>  
> +static int cppc_get_perf_ctrs_pair(void *val)
> +{
> +	struct fb_ctr_pair *fb_ctrs = val;
> +	int cpu = fb_ctrs->cpu;
> +	int ret;
> +
> +	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t0);
> +	if (ret)
> +		return ret;
> +
> +	udelay(2); /* 2usec delay between sampling */
> +
> +	return cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t1);
> +}
> +
>  static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>  {
> -	struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
> +	struct fb_ctr_pair fb_ctrs = { .cpu = cpu, };
>  	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
>  	struct cppc_cpudata *cpu_data = policy->driver_data;
>  	u64 delivered_perf;
> @@ -850,18 +871,18 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>  
>  	cpufreq_cpu_put(policy);
>  
> -	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
> -	if (ret)
> -		return 0;
> -
> -	udelay(2); /* 2usec delay between sampling */
> +	if (cpu_has_amu_feat(cpu))
> +		ret = smp_call_on_cpu(cpu, cppc_get_perf_ctrs_pair,
> +				      &fb_ctrs, false);
> +	else
> +		ret = cppc_get_perf_ctrs_pair(&fb_ctrs);
>  
> -	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>  	if (ret)
>  		return 0;
>  
> -	delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
> -					       &fb_ctrs_t1);
> +	delivered_perf = cppc_perf_from_fbctrs(cpu_data,
> +					      &fb_ctrs.fb_ctrs_t0,
> +					      &fb_ctrs.fb_ctrs_t1);
>  
>  	return cppc_cpufreq_perf_to_khz(cpu_data, delivered_perf);
>  }
> -- 
> 2.25.1
>
Sudeep Holla Oct. 25, 2023, 11:13 a.m. UTC | #2 
On Wed, Oct 25, 2023 at 05:38:46PM +0800, Zeng Heng wrote:
> As ARM AMU's document says, all counters are subject to any changes
> in clock frequency, including clock stopping caused by the WFI and WFE
> instructions.
> 
> Therefore, using smp_call_on_cpu() to trigger target CPU to
> read self's AMU counters, which ensures the counters are working
> properly while cstate feature is enabled.
> 
> Reported-by: Sumit Gupta <sumitg@nvidia.com>
> Link: https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 39 ++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index fe08ca419b3d..321a9dc9484d 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c

[...]

> @@ -850,18 +871,18 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>  
>  	cpufreq_cpu_put(policy);
>  
> -	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
> -	if (ret)
> -		return 0;
> -
> -	udelay(2); /* 2usec delay between sampling */
> +	if (cpu_has_amu_feat(cpu))

Have you compiled this on x86 ? Even if you have somehow managed to,
this is not the right place to check the presence of AMU feature on
the CPU.

If AMU registers are used in CPPC, they must be using FFH GAS, in which
case the interpretation of FFH is architecture dependent code.

--
Regards,
Sudeep
Sumit Gupta Oct. 25, 2023, 2:57 p.m. UTC | #3 
> [adding Ionela]
> 
> On Wed, Oct 25, 2023 at 05:38:46PM +0800, Zeng Heng wrote:
>> As ARM AMU's document says, all counters are subject to any changes
>> in clock frequency, including clock stopping caused by the WFI and WFE
>> instructions.
>>
>> Therefore, using smp_call_on_cpu() to trigger target CPU to
>> read self's AMU counters, which ensures the counters are working
>> properly while cstate feature is enabled.
> 
> IIUC there's a pretty deliberate split with all the actual reading of the AMU
> living in arch/arm64/kernel/topolgy.c, and the driver code being (relatively)
> generic.
> 
> We already have code in arch/arm64/kernel/topolgy.c to read counters on a
> specific CPU; why can't e reuse that (and avoid exporting cpu_has_amu_feat())?


This patch seems mostly based on my previous patch [1] and discussed 
here [2] already. Beata [CCed] shared an alternate approach [3] 
leveraging existing code from 'topology.c' to get the average freq for 
last tick period.


Beata,

Could you share v2 of [3] with the request to merge. We can try to solve 
the issue with CPU IDLE case later on top?

Additionally, also please include the fix in [4] if it looks fine.

Best Regards,
Sumit Gupta

[1] https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
[2] 
https://lore.kernel.org/lkml/cde1d8a9-3a21-e82b-7895-40603a14d898@nvidia.com/T/#m2174305de4706006e0bd9c103a0e5ff61cea7a12
[3] 
https://lore.kernel.org/lkml/20230606155754.245998-1-beata.michalska@arm.com/
[4] 
https://lore.kernel.org/lkml/6a5710f6-bfbb-5dfd-11cd-0cd02220cee7@nvidia.com/


>>
>> Reported-by: Sumit Gupta <sumitg@nvidia.com>
>> Link: https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
>> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 39 ++++++++++++++++++++++++++--------
>>   1 file changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index fe08ca419b3d..321a9dc9484d 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -90,6 +90,12 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>                                 struct cppc_perf_fb_ctrs *fb_ctrs_t0,
>>                                 struct cppc_perf_fb_ctrs *fb_ctrs_t1);
>>
>> +struct fb_ctr_pair {
>> +     u32 cpu;
>> +     struct cppc_perf_fb_ctrs fb_ctrs_t0;
>> +     struct cppc_perf_fb_ctrs fb_ctrs_t1;
>> +};
>> +
>>   /**
>>    * cppc_scale_freq_workfn - CPPC arch_freq_scale updater for frequency invariance
>>    * @work: The work item.
>> @@ -840,9 +846,24 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>        return (reference_perf * delta_delivered) / delta_reference;
>>   }
>>
>> +static int cppc_get_perf_ctrs_pair(void *val)
>> +{
>> +     struct fb_ctr_pair *fb_ctrs = val;
>> +     int cpu = fb_ctrs->cpu;
>> +     int ret;
>> +
>> +     ret = cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t0);
>> +     if (ret)
>> +             return ret;
>> +
>> +     udelay(2); /* 2usec delay between sampling */
>> +
>> +     return cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t1);
>> +}
>> +
>>   static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>   {
>> -     struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>> +     struct fb_ctr_pair fb_ctrs = { .cpu = cpu, };
>>        struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
>>        struct cppc_cpudata *cpu_data = policy->driver_data;
>>        u64 delivered_perf;
>> @@ -850,18 +871,18 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>
>>        cpufreq_cpu_put(policy);
>>
>> -     ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>> -     if (ret)
>> -             return 0;
>> -
>> -     udelay(2); /* 2usec delay between sampling */
>> +     if (cpu_has_amu_feat(cpu))
>> +             ret = smp_call_on_cpu(cpu, cppc_get_perf_ctrs_pair,
>> +                                   &fb_ctrs, false);
>> +     else
>> +             ret = cppc_get_perf_ctrs_pair(&fb_ctrs);
>>
>> -     ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>>        if (ret)
>>                return 0;
>>
>> -     delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>> -                                            &fb_ctrs_t1);
>> +     delivered_perf = cppc_perf_from_fbctrs(cpu_data,
>> +                                           &fb_ctrs.fb_ctrs_t0,
>> +                                           &fb_ctrs.fb_ctrs_t1);
>>
>>        return cppc_cpufreq_perf_to_khz(cpu_data, delivered_perf);
>>   }
>> --
>> 2.25.1
>>
Zeng Heng Oct. 26, 2023, 2:24 a.m. UTC | #4 
在 2023/10/25 19:13, Sudeep Holla 写道:
> On Wed, Oct 25, 2023 at 05:38:46PM +0800, Zeng Heng wrote:
>> As ARM AMU's document says, all counters are subject to any changes
>> in clock frequency, including clock stopping caused by the WFI and WFE
>> instructions.
>>
>> Therefore, using smp_call_on_cpu() to trigger target CPU to
>> read self's AMU counters, which ensures the counters are working
>> properly while cstate feature is enabled.
>>
>> Reported-by: Sumit Gupta <sumitg@nvidia.com>
>> Link: https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
>> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 39 ++++++++++++++++++++++++++--------
>>   1 file changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index fe08ca419b3d..321a9dc9484d 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
> [...]
>
>> @@ -850,18 +871,18 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>   
>>   	cpufreq_cpu_put(policy);
>>   
>> -	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>> -	if (ret)
>> -		return 0;
>> -
>> -	udelay(2); /* 2usec delay between sampling */
>> +	if (cpu_has_amu_feat(cpu))
> Have you compiled this on x86 ? Even if you have somehow managed to,
> this is not the right place to check the presence of AMU feature on
> the CPU.
> If AMU registers are used in CPPC, they must be using FFH GAS, in which
> case the interpretation of FFH is architecture dependent code.


According to drivers/cpufreq/Makefile, cppc_cpufreq.c is only compiled with

ARM architecture.

But here, I would change cpu_has_amu_feat() with cpc_ffh_supported(), which

belongs to FFH APIs.

Thanks for the suggestion.


Thanks again,

Zeng Heng



> --
> Regards,
> Sudeep
>
Zeng Heng Oct. 26, 2023, 3:21 a.m. UTC | #5 
在 2023/10/25 18:54, Mark Rutland 写道:
> [adding Ionela]
>
> On Wed, Oct 25, 2023 at 05:38:46PM +0800, Zeng Heng wrote:
>> As ARM AMU's document says, all counters are subject to any changes
>> in clock frequency, including clock stopping caused by the WFI and WFE
>> instructions.
>>
>> Therefore, using smp_call_on_cpu() to trigger target CPU to
>> read self's AMU counters, which ensures the counters are working
>> properly while cstate feature is enabled.
> IIUC there's a pretty deliberate split with all the actual reading of the AMU
> living in arch/arm64/kernel/topolgy.c, and the driver code being (relatively)
> generic.
>
> We already have code in arch/arm64/kernel/topolgy.c to read counters on a
> specific CPU; why can't e reuse that (and avoid exporting cpu_has_amu_feat())?
>
> Mark.

In this scenario, both topology.c and cppc_acpi.c do not provide an API 
to keep the AMU online

during the whole sampling period. Just using cpc_read_ffh at the start 
and end of the sampling

period is not enough.

However, I can propose cpc_ffh_supported() function to replace the 
cpu_has_amu_feat() as v2

if you think this patch set is still valuable.


Thanks,

Zeng Heng

>> Reported-by: Sumit Gupta <sumitg@nvidia.com>
>> Link: https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
>> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 39 ++++++++++++++++++++++++++--------
>>   1 file changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index fe08ca419b3d..321a9dc9484d 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -90,6 +90,12 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>   				 struct cppc_perf_fb_ctrs *fb_ctrs_t0,
>>   				 struct cppc_perf_fb_ctrs *fb_ctrs_t1);
>>   
>> +struct fb_ctr_pair {
>> +	u32 cpu;
>> +	struct cppc_perf_fb_ctrs fb_ctrs_t0;
>> +	struct cppc_perf_fb_ctrs fb_ctrs_t1;
>> +};
>> +
>>   /**
>>    * cppc_scale_freq_workfn - CPPC arch_freq_scale updater for frequency invariance
>>    * @work: The work item.
>> @@ -840,9 +846,24 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>   	return (reference_perf * delta_delivered) / delta_reference;
>>   }
>>   
>> +static int cppc_get_perf_ctrs_pair(void *val)
>> +{
>> +	struct fb_ctr_pair *fb_ctrs = val;
>> +	int cpu = fb_ctrs->cpu;
>> +	int ret;
>> +
>> +	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	udelay(2); /* 2usec delay between sampling */
>> +
>> +	return cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t1);
>> +}
>> +
>>   static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>   {
>> -	struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>> +	struct fb_ctr_pair fb_ctrs = { .cpu = cpu, };
>>   	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
>>   	struct cppc_cpudata *cpu_data = policy->driver_data;
>>   	u64 delivered_perf;
>> @@ -850,18 +871,18 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>   
>>   	cpufreq_cpu_put(policy);
>>   
>> -	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>> -	if (ret)
>> -		return 0;
>> -
>> -	udelay(2); /* 2usec delay between sampling */
>> +	if (cpu_has_amu_feat(cpu))
>> +		ret = smp_call_on_cpu(cpu, cppc_get_perf_ctrs_pair,
>> +				      &fb_ctrs, false);
>> +	else
>> +		ret = cppc_get_perf_ctrs_pair(&fb_ctrs);
>>   
>> -	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>>   	if (ret)
>>   		return 0;
>>   
>> -	delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>> -					       &fb_ctrs_t1);
>> +	delivered_perf = cppc_perf_from_fbctrs(cpu_data,
>> +					      &fb_ctrs.fb_ctrs_t0,
>> +					      &fb_ctrs.fb_ctrs_t1);
>>   
>>   	return cppc_cpufreq_perf_to_khz(cpu_data, delivered_perf);
>>   }
>> -- 
>> 2.25.1
>>
Sudeep Holla Oct. 26, 2023, 8:53 a.m. UTC | #6 
On Thu, Oct 26, 2023 at 10:24:54AM +0800, Zeng Heng wrote:
> 
> 在 2023/10/25 19:13, Sudeep Holla 写道:
> > On Wed, Oct 25, 2023 at 05:38:46PM +0800, Zeng Heng wrote:
> > > As ARM AMU's document says, all counters are subject to any changes
> > > in clock frequency, including clock stopping caused by the WFI and WFE
> > > instructions.
> > > 
> > > Therefore, using smp_call_on_cpu() to trigger target CPU to
> > > read self's AMU counters, which ensures the counters are working
> > > properly while cstate feature is enabled.
> > > 
> > > Reported-by: Sumit Gupta <sumitg@nvidia.com>
> > > Link: https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
> > > Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> > > ---
> > >   drivers/cpufreq/cppc_cpufreq.c | 39 ++++++++++++++++++++++++++--------
> > >   1 file changed, 30 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > > index fe08ca419b3d..321a9dc9484d 100644
> > > --- a/drivers/cpufreq/cppc_cpufreq.c
> > > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > [...]
> > 
> > > @@ -850,18 +871,18 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
> > >   	cpufreq_cpu_put(policy);
> > > -	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
> > > -	if (ret)
> > > -		return 0;
> > > -
> > > -	udelay(2); /* 2usec delay between sampling */
> > > +	if (cpu_has_amu_feat(cpu))
> > Have you compiled this on x86 ? Even if you have somehow managed to,
> > this is not the right place to check the presence of AMU feature on
> > the CPU.
> > If AMU registers are used in CPPC, they must be using FFH GAS, in which
> > case the interpretation of FFH is architecture dependent code.
>
> According to drivers/cpufreq/Makefile, cppc_cpufreq.c is only compiled with
> ARM architecture.
>

Well that's true but this change doesn't belong to cppc_cpufreq.c, it must
be part of drivers/acpi/cppc_acpi.c IMO and sorry I assumed that without
explicitly mentioning that here.

> But here, I would change cpu_has_amu_feat() with cpc_ffh_supported(), which
> belongs to FFH APIs.
>

It is not like that. cppc_acpi.c will know the GAS is FFH based so no need to
check anything there. I see counters_read_on_cpu() called from cpc_ffh_read()
already takes care of reading the AMUs on the right CPU. What exactly is
the issue you are seeing ? I don't if this change is needed at all.

--
Regards,
Sudeep
Zeng Heng Oct. 26, 2023, 9:05 a.m. UTC | #7 
在 2023/10/26 16:53, Sudeep Holla 写道:
> On Thu, Oct 26, 2023 at 10:24:54AM +0800, Zeng Heng wrote:
>> 在 2023/10/25 19:13, Sudeep Holla 写道:
>>> On Wed, Oct 25, 2023 at 05:38:46PM +0800, Zeng Heng wrote:
>>>> As ARM AMU's document says, all counters are subject to any changes
>>>> in clock frequency, including clock stopping caused by the WFI and WFE
>>>> instructions.
>>>>
>>>> Therefore, using smp_call_on_cpu() to trigger target CPU to
>>>> read self's AMU counters, which ensures the counters are working
>>>> properly while cstate feature is enabled.
>>>>
>>>> Reported-by: Sumit Gupta <sumitg@nvidia.com>
>>>> Link: https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
>>>> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
>>>> ---
>>>>    drivers/cpufreq/cppc_cpufreq.c | 39 ++++++++++++++++++++++++++--------
>>>>    1 file changed, 30 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>>> index fe08ca419b3d..321a9dc9484d 100644
>>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>> [...]
>>>
>>>> @@ -850,18 +871,18 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>>>    	cpufreq_cpu_put(policy);
>>>> -	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>>>> -	if (ret)
>>>> -		return 0;
>>>> -
>>>> -	udelay(2); /* 2usec delay between sampling */
>>>> +	if (cpu_has_amu_feat(cpu))
>>> Have you compiled this on x86 ? Even if you have somehow managed to,
>>> this is not the right place to check the presence of AMU feature on
>>> the CPU.
>>> If AMU registers are used in CPPC, they must be using FFH GAS, in which
>>> case the interpretation of FFH is architecture dependent code.
>> According to drivers/cpufreq/Makefile, cppc_cpufreq.c is only compiled with
>> ARM architecture.
>>
> Well that's true but this change doesn't belong to cppc_cpufreq.c, it must
> be part of drivers/acpi/cppc_acpi.c IMO and sorry I assumed that without
> explicitly mentioning that here.
>
>> But here, I would change cpu_has_amu_feat() with cpc_ffh_supported(), which
>> belongs to FFH APIs.
>>
> It is not like that. cppc_acpi.c will know the GAS is FFH based so no need to
> check anything there. I see counters_read_on_cpu() called from cpc_ffh_read()
> already takes care of reading the AMUs on the right CPU. What exactly is
> the issue you are seeing ? I don't if this change is needed at all.
>
> --
> Regards,
> Sudeep


In this scenario, both topology.c and cppc_acpi.c do not provide an API 
to keep the AMU online

during the whole sampling period. Just using cpc_read_ffh() at the start 
and end of the sampling

period is not enough.


Zeng Heng
Beata Michalska Oct. 30, 2023, 1:19 p.m. UTC | #8 
On Wed, Oct 25, 2023 at 08:27:23PM +0530, Sumit Gupta wrote:
> 
> 
> 
> > [adding Ionela]
> > 
> > On Wed, Oct 25, 2023 at 05:38:46PM +0800, Zeng Heng wrote:
> > > As ARM AMU's document says, all counters are subject to any changes
> > > in clock frequency, including clock stopping caused by the WFI and WFE
> > > instructions.
> > > 
> > > Therefore, using smp_call_on_cpu() to trigger target CPU to
> > > read self's AMU counters, which ensures the counters are working
> > > properly while cstate feature is enabled.
> > 
> > IIUC there's a pretty deliberate split with all the actual reading of the AMU
> > living in arch/arm64/kernel/topolgy.c, and the driver code being (relatively)
> > generic.
> > 
> > We already have code in arch/arm64/kernel/topolgy.c to read counters on a
> > specific CPU; why can't e reuse that (and avoid exporting cpu_has_amu_feat())?
> 
> 
> This patch seems mostly based on my previous patch [1] and discussed here
> [2] already. Beata [CCed] shared an alternate approach [3] leveraging
> existing code from 'topology.c' to get the average freq for last tick
> period.
> 
> 
> Beata,
> 
> Could you share v2 of [3] with the request to merge. We can try to solve the
> issue with CPU IDLE case later on top?
>
Will do (same for the below request if feasible)

---
BR
B.
> Additionally, also please include the fix in [4] if it looks fine.
> 
> Best Regards,
> Sumit Gupta
> 
> [1] https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
> [2] https://lore.kernel.org/lkml/cde1d8a9-3a21-e82b-7895-40603a14d898@nvidia.com/T/#m2174305de4706006e0bd9c103a0e5ff61cea7a12
> [3]
> https://lore.kernel.org/lkml/20230606155754.245998-1-beata.michalska@arm.com/
> [4]
> https://lore.kernel.org/lkml/6a5710f6-bfbb-5dfd-11cd-0cd02220cee7@nvidia.com/
> 
> 
> > > 
> > > Reported-by: Sumit Gupta <sumitg@nvidia.com>
> > > Link: https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/
> > > Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> > > ---
> > >   drivers/cpufreq/cppc_cpufreq.c | 39 ++++++++++++++++++++++++++--------
> > >   1 file changed, 30 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > > index fe08ca419b3d..321a9dc9484d 100644
> > > --- a/drivers/cpufreq/cppc_cpufreq.c
> > > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > > @@ -90,6 +90,12 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
> > >                                 struct cppc_perf_fb_ctrs *fb_ctrs_t0,
> > >                                 struct cppc_perf_fb_ctrs *fb_ctrs_t1);
> > > 
> > > +struct fb_ctr_pair {
> > > +     u32 cpu;
> > > +     struct cppc_perf_fb_ctrs fb_ctrs_t0;
> > > +     struct cppc_perf_fb_ctrs fb_ctrs_t1;
> > > +};
> > > +
> > >   /**
> > >    * cppc_scale_freq_workfn - CPPC arch_freq_scale updater for frequency invariance
> > >    * @work: The work item.
> > > @@ -840,9 +846,24 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
> > >        return (reference_perf * delta_delivered) / delta_reference;
> > >   }
> > > 
> > > +static int cppc_get_perf_ctrs_pair(void *val)
> > > +{
> > > +     struct fb_ctr_pair *fb_ctrs = val;
> > > +     int cpu = fb_ctrs->cpu;
> > > +     int ret;
> > > +
> > > +     ret = cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t0);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     udelay(2); /* 2usec delay between sampling */
> > > +
> > > +     return cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t1);
> > > +}
> > > +
> > >   static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
> > >   {
> > > -     struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
> > > +     struct fb_ctr_pair fb_ctrs = { .cpu = cpu, };
> > >        struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> > >        struct cppc_cpudata *cpu_data = policy->driver_data;
> > >        u64 delivered_perf;
> > > @@ -850,18 +871,18 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
> > > 
> > >        cpufreq_cpu_put(policy);
> > > 
> > > -     ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
> > > -     if (ret)
> > > -             return 0;
> > > -
> > > -     udelay(2); /* 2usec delay between sampling */
> > > +     if (cpu_has_amu_feat(cpu))
> > > +             ret = smp_call_on_cpu(cpu, cppc_get_perf_ctrs_pair,
> > > +                                   &fb_ctrs, false);
> > > +     else
> > > +             ret = cppc_get_perf_ctrs_pair(&fb_ctrs);
> > > 
> > > -     ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
> > >        if (ret)
> > >                return 0;
> > > 
> > > -     delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
> > > -                                            &fb_ctrs_t1);
> > > +     delivered_perf = cppc_perf_from_fbctrs(cpu_data,
> > > +                                           &fb_ctrs.fb_ctrs_t0,
> > > +                                           &fb_ctrs.fb_ctrs_t1);
> > > 
> > >        return cppc_cpufreq_perf_to_khz(cpu_data, delivered_perf);
> > >   }
> > > --
> > > 2.25.1
> > >
kernel test robot Oct. 31, 2023, 11:52 p.m. UTC | #9 
Hi Zeng,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/acpi-bus arm64/for-next/core linus/master v6.6 next-20231031]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Zeng-Heng/arm64-cpufeature-Export-cpu_has_amu_feat/20231025-173559
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20231025093847.3740104-3-zengheng4%40huawei.com
patch subject: [PATCH 2/3] cpufreq: CPPC: Keep the target core awake when reading its cpufreq rate
config: arm64-randconfig-003-20231101 (https://download.01.org/0day-ci/archive/20231101/202311010726.MjF49sPn-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231101/202311010726.MjF49sPn-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311010726.MjF49sPn-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/cpufreq/cppc_cpufreq.c: In function 'cppc_get_perf_ctrs_pair':
>> drivers/cpufreq/cppc_cpufreq.c:852:26: error: invalid use of undefined type 'struct fb_ctr_pair'
     852 |         int cpu = fb_ctrs->cpu;
         |                          ^~
   drivers/cpufreq/cppc_cpufreq.c:855:47: error: invalid use of undefined type 'struct fb_ctr_pair'
     855 |         ret = cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t0);
         |                                               ^~
   drivers/cpufreq/cppc_cpufreq.c:861:48: error: invalid use of undefined type 'struct fb_ctr_pair'
     861 |         return cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t1);
         |                                                ^~
   drivers/cpufreq/cppc_cpufreq.c: In function 'cppc_cpufreq_get_rate':
>> drivers/cpufreq/cppc_cpufreq.c:866:16: error: variable 'fb_ctrs' has initializer but incomplete type
     866 |         struct fb_ctr_pair fb_ctrs = { .cpu = cpu, };
         |                ^~~~~~~~~~~
>> drivers/cpufreq/cppc_cpufreq.c:866:41: error: 'struct fb_ctr_pair' has no member named 'cpu'
     866 |         struct fb_ctr_pair fb_ctrs = { .cpu = cpu, };
         |                                         ^~~
>> drivers/cpufreq/cppc_cpufreq.c:866:47: warning: excess elements in struct initializer
     866 |         struct fb_ctr_pair fb_ctrs = { .cpu = cpu, };
         |                                               ^~~
   drivers/cpufreq/cppc_cpufreq.c:866:47: note: (near initialization for 'fb_ctrs')
>> drivers/cpufreq/cppc_cpufreq.c:866:28: error: storage size of 'fb_ctrs' isn't known
     866 |         struct fb_ctr_pair fb_ctrs = { .cpu = cpu, };
         |                            ^~~~~~~
>> drivers/cpufreq/cppc_cpufreq.c:866:28: warning: unused variable 'fb_ctrs' [-Wunused-variable]


vim +852 drivers/cpufreq/cppc_cpufreq.c

   848	
   849	static int cppc_get_perf_ctrs_pair(void *val)
   850	{
   851		struct fb_ctr_pair *fb_ctrs = val;
 > 852		int cpu = fb_ctrs->cpu;
   853		int ret;
   854	
   855		ret = cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t0);
   856		if (ret)
   857			return ret;
   858	
   859		udelay(2); /* 2usec delay between sampling */
   860	
   861		return cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t1);
   862	}
   863	
   864	static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
   865	{
 > 866		struct fb_ctr_pair fb_ctrs = { .cpu = cpu, };
   867		struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
   868		struct cppc_cpudata *cpu_data = policy->driver_data;
   869		u64 delivered_perf;
   870		int ret;
   871	
   872		cpufreq_cpu_put(policy);
   873	
   874		if (cpu_has_amu_feat(cpu))
   875			ret = smp_call_on_cpu(cpu, cppc_get_perf_ctrs_pair,
   876					      &fb_ctrs, false);
   877		else
   878			ret = cppc_get_perf_ctrs_pair(&fb_ctrs);
   879	
   880		if (ret)
   881			return 0;
   882	
   883		delivered_perf = cppc_perf_from_fbctrs(cpu_data,
   884						      &fb_ctrs.fb_ctrs_t0,
   885						      &fb_ctrs.fb_ctrs_t1);
   886	
   887		return cppc_cpufreq_perf_to_khz(cpu_data, delivered_perf);
   888	}
   889
diff mbox series

Patch

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index fe08ca419b3d..321a9dc9484d 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -90,6 +90,12 @@  static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
 				 struct cppc_perf_fb_ctrs *fb_ctrs_t0,
 				 struct cppc_perf_fb_ctrs *fb_ctrs_t1);
 
+struct fb_ctr_pair {
+	u32 cpu;
+	struct cppc_perf_fb_ctrs fb_ctrs_t0;
+	struct cppc_perf_fb_ctrs fb_ctrs_t1;
+};
+
 /**
  * cppc_scale_freq_workfn - CPPC arch_freq_scale updater for frequency invariance
  * @work: The work item.
@@ -840,9 +846,24 @@  static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
 	return (reference_perf * delta_delivered) / delta_reference;
 }
 
+static int cppc_get_perf_ctrs_pair(void *val)
+{
+	struct fb_ctr_pair *fb_ctrs = val;
+	int cpu = fb_ctrs->cpu;
+	int ret;
+
+	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t0);
+	if (ret)
+		return ret;
+
+	udelay(2); /* 2usec delay between sampling */
+
+	return cppc_get_perf_ctrs(cpu, &fb_ctrs->fb_ctrs_t1);
+}
+
 static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 {
-	struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
+	struct fb_ctr_pair fb_ctrs = { .cpu = cpu, };
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
 	struct cppc_cpudata *cpu_data = policy->driver_data;
 	u64 delivered_perf;
@@ -850,18 +871,18 @@  static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 
 	cpufreq_cpu_put(policy);
 
-	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
-	if (ret)
-		return 0;
-
-	udelay(2); /* 2usec delay between sampling */
+	if (cpu_has_amu_feat(cpu))
+		ret = smp_call_on_cpu(cpu, cppc_get_perf_ctrs_pair,
+				      &fb_ctrs, false);
+	else
+		ret = cppc_get_perf_ctrs_pair(&fb_ctrs);
 
-	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
 	if (ret)
 		return 0;
 
-	delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
-					       &fb_ctrs_t1);
+	delivered_perf = cppc_perf_from_fbctrs(cpu_data,
+					      &fb_ctrs.fb_ctrs_t0,
+					      &fb_ctrs.fb_ctrs_t1);
 
 	return cppc_cpufreq_perf_to_khz(cpu_data, delivered_perf);
 }