External email: Use caution opening links or attachmentsWith this change, the delta between the set and get freq has reduced
On 4/26/23 12:01 PM, Ionela Voinescu wrote:
Hi,
+ Sumit
On Tuesday 25 Apr 2023 at 18:32:55 (-0700), Yang Shi wrote:
Yes, there is a minimum delay required there of 25us due to the way that
On 4/24/23 4:44 AM, Ionela Voinescu wrote:
Hey,Yeah, but it seems like using larger delay could mitigate both issues.
On Thursday 20 Apr 2023 at 13:49:24 (-0700), Yang Shi wrote:
On 4/20/23 9:01 AM, Pierre Gondois wrote:Yes, my bad, I did not get the chance to read this full thread before
Thanks for the information. I think we do have the similar syndrome. But I'm+IonelaYou say that the cause of this is a congestion in the interconnect. INo, other CPUs were not shut down in my test. I just ran "yes" on all
don't
see a way to check that right now.
However your trace is on the CPU0, so maybe all the other cores were
shutdown
in your test. If this is the case, do you think a congestion could
happen with
only one CPU ?
cores except CPU 0, then ran the reading freq script. Since all other
cores are busy, so the script should be always running on CPU 0.
Since the counters, memory and other devices are on the interconnect, so
the congestion may be caused by plenty of factors IIUC.
Ionela pointed me to the following patch-set, which seems realated:
https://lore.kernel.org/all/20230418113459.12860-5-sumitg@xxxxxxxxxx/
not sure how their counters are implemented, we may not have similar root
cause.
talking with Pierre. In your case you have AMUs accessed via MMIO and in that
case they are accessed though FFH (IPIs and system registers). The root
cause is indeed different.
the counters accumulate, which is not too bad even with interrupts
disabled (to cater to cpufreq_quick_get()).
[..]
Yes, this is what I was suggesting above.It won't work, right? The problem is cppc_get_perf_ctrs() calls cpc_read()sCan you not disable IRQs in cppc_get_perf_ctrs() only if the registersYeah, you are correct. The irq disabling window has to cover all theYeah, we should be able to find a smaller irq disable section.I thought the issue was that irqs could happen in between cpc_read()
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.ccpc_read_ffh() would return -EPERM if irq is disabled.
index c51d3ccb4cca..105a7e2ffffa 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1315,6 +1315,7 @@ int cppc_get_perf_ctrs(int cpunum, struct
cppc_perf_fb_ctrs *perf_fb_ctrs)
struct cppc_pcc_data *pcc_ss_data = NULL;
u64 delivered, reference, ref_perf, ctr_wrap_time;
int ret = 0, regs_in_pcc = 0;
+ unsigned long flags;
if (!cpc_desc) {
pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
@@ -1350,10 +1351,14 @@ int cppc_get_perf_ctrs(int cpunum, struct
cppc_perf_fb_ctrs *perf_fb_ctrs)
}
}
+ local_irq_save(flags);
+
cpc_read(cpunum, delivered_reg, &delivered);
cpc_read(cpunum, reference_reg, &reference);
cpc_read(cpunum, ref_perf_reg, &ref_perf);
+ local_irq_restore(flags);
+
So, the irq disabling must happen for mmio only in cpc_read(), for
example:
functions,
the patch below would not cover it. If the frequency is more accurate
with this patch, I think I don't understand something.
cpc_read(). I didn't test with this patch. My test was done conceptually
with:
disable irq
cppc_get_perf_ctrs(t0)
udelay(2)
cppc_get_perf_ctrs(t1)
enable irq
But this will break cpc_read_ffh().
are CPC_IN_SYSTEM_MEMORY? Only spanning the reads of the delivered
register and the reference register, which should have minimal delay in
between?
As in:
if (CPC_IN_SYSTEM_MEMORY(delivered_reg) &&
CPC_IN_SYSTEM_MEMORY(reference_reg))
...
This will and should not affect FFH - the fix for that would have to be
different.
to read delivered and reference respectively, we just can tell whether they
are CPC_IN_SYSTEM_MEMORY in cpc_read() instead of in cppc_get_perf_ctrs().
So the resulting code should conceptually look like:
disable irq
read delivered
enable irq
disable irq
read reference
enable irq
But there still may be interrupts between the two reads. We actually want:
disable irq
read delivered
read reference
enable irq
I've hacked up the following code. It covers the FFH case as well, with a
modified solution that Sumit proposed on the other thread:
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 0f17b1c32718..7e828aed3693 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -110,6 +110,11 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
(cpc)->cpc_entry.reg.space_id == \
ACPI_ADR_SPACE_SYSTEM_IO)
+/* Check if a CPC register is in FFH */
+#define CPC_IN_FFH(cpc) ((cpc)->type == ACPI_TYPE_BUFFER && \
+ (cpc)->cpc_entry.reg.space_id == \
+ ACPI_ADR_SPACE_FIXED_HARDWARE)
+
/* Evaluates to True if reg is a NULL register descriptor */
#define IS_NULL_REG(reg) ((reg)->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY && \
(reg)->address == 0 && \
@@ -1292,6 +1297,24 @@ EXPORT_SYMBOL_GPL(cppc_perf_ctrs_in_pcc);
*
* Return: 0 for success with perf_fb_ctrs populated else -ERRNO.
*/
+
+struct cycle_counters {
+ int cpunum;
+ struct cpc_register_resource *delivered_reg;
+ struct cpc_register_resource *reference_reg;
+ u64 *delivered;
+ u64 *reference;
+};
+
+static int cppc_get_cycle_ctrs(void *cycle_ctrs) {
+ struct cycle_counters *ctrs = cycle_ctrs;
+
+ cpc_read(ctrs->cpunum, ctrs->delivered_reg, ctrs->delivered);
+ cpc_read(ctrs->cpunum, ctrs->reference_reg, ctrs->reference);
+
+ return 0;
+}
+
int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
{
struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
@@ -1300,7 +1323,9 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
struct cppc_pcc_data *pcc_ss_data = NULL;
u64 delivered, reference, ref_perf, ctr_wrap_time;
+ struct cycle_counters ctrs = {0};
int ret = 0, regs_in_pcc = 0;
+ unsigned long flags;
if (!cpc_desc) {
pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
@@ -1336,8 +1361,25 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
}
}
- cpc_read(cpunum, delivered_reg, &delivered);
- cpc_read(cpunum, reference_reg, &reference);
+ ctrs.cpunum = cpunum;
+ ctrs.delivered_reg = delivered_reg;
+ ctrs.reference_reg = reference_reg;
+ ctrs.delivered = &delivered;
+ ctrs.reference = &reference;
+
+ if (CPC_IN_FFH(delivered_reg) && CPC_IN_FFH(reference_reg)) {
+ ret = smp_call_on_cpu(cpunum, cppc_get_cycle_ctrs, &ctrs, false);
+ } else {
+ if (CPC_IN_SYSTEM_MEMORY(delivered_reg) &&
+ CPC_IN_SYSTEM_MEMORY(reference_reg)) {
+ local_irq_save(flags);
+ cppc_get_cycle_ctrs(&ctrs);krish
+ local_irq_restore(flags);
+ } else {
+ cppc_get_cycle_ctrs(&ctrs);
+ }
+ }
+
cpc_read(cpunum, ref_perf_reg, &ref_perf);
/*
I've only tested this on a model using FFH, with 10us delay, and it
worked well for me. Yang, Sumit, could you give it a try?
Thanks for the patch. I tested it with 10us delay, it works well for me.
There was 0 high error in my 3 hours test.
Even with a solution like the above (more refined, of course) there is an
additional improvement possible: we can implement arch_freq_get_on_cpu()
for arm64 systems that will use cached (on the tick) AMU cycle counter
samples and have this function called from show_cpuinfo_cur_freq()
before/instead of calling the .get() function. But this will only help
arm64 systems with AMUs accessible though system registers. We'll try to
submit patches on this soon. But as I mentioned to Sumit on the other
thread, the returned frequency value from this will be an average over 4ms
(with CONFIG_HZ=250) and could be up to 4ms old (more than that only if the
CPU was idle/isolated).
Thanks,
Ionela.