| Message ID | 20231025093847.3740104-3-zengheng4@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make the cpuinfo_cur_freq interface read correctly | expand |
[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 >
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
> [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 >>
在 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 >
在 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 >>
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
在 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
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 > > >
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 --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); }
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(-)