| Message ID | 20240729125858.630653-1-dmitry.shifrin@syntacore.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 941a8e9b7a86763ac52d5bf6ccc9986d37fde628 |
| Headers | show |
| Series | perf: riscv: Fix selecting counters in legacy mode | expand |
> It is required to check event type before checking event config. > Events with the different types can have the same config. > This check is missed for legacy mode code > > For such perf usage: > sysctl -w kernel.perf_user_access=2 > perf stat -e cycles,L1-dcache-loads -- > driver will try to force both events to CYCLE counter. > > This commit implements event type check before forcing > events on the special counters. > > Signed-off-by: Shifrin Dmitry <dmitry.shifrin@syntacore.com> > --- > drivers/perf/riscv_pmu_sbi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > index 36d128ff166f..bf14ab282e11 100644 > --- a/drivers/perf/riscv_pmu_sbi.c > +++ b/drivers/perf/riscv_pmu_sbi.c > @@ -416,7 +416,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) > * but not in the user access mode as we want to use the other counters > * that support sampling/filtering. > */ > - if (hwc->flags & PERF_EVENT_FLAG_LEGACY) { > + if ((hwc->flags & PERF_EVENT_FLAG_LEGACY) && (event->attr.type == PERF_TYPE_HARDWARE)) { > if (event->attr.config == PERF_COUNT_HW_CPU_CYCLES) { > cflags |= SBI_PMU_CFG_FLAG_SKIP_MATCH; > cmask = 1; > Nice catch. Reviewed-by: Atish Patra <atishp@rivosinc.com>
Hi Dmitry, On 29/07/2024 20:05, Atish Patra wrote: >> It is required to check event type before checking event config. >> Events with the different types can have the same config. >> This check is missed for legacy mode code >> >> For such perf usage: >> sysctl -w kernel.perf_user_access=2 >> perf stat -e cycles,L1-dcache-loads -- >> driver will try to force both events to CYCLE counter. >> >> This commit implements event type check before forcing >> events on the special counters. >> >> Signed-off-by: Shifrin Dmitry <dmitry.shifrin@syntacore.com> >> --- >> drivers/perf/riscv_pmu_sbi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c >> index 36d128ff166f..bf14ab282e11 100644 >> --- a/drivers/perf/riscv_pmu_sbi.c >> +++ b/drivers/perf/riscv_pmu_sbi.c >> @@ -416,7 +416,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) >> * but not in the user access mode as we want to use the other counters >> * that support sampling/filtering. >> */ >> - if (hwc->flags & PERF_EVENT_FLAG_LEGACY) { >> + if ((hwc->flags & PERF_EVENT_FLAG_LEGACY) && (event->attr.type == PERF_TYPE_HARDWARE)) { >> if (event->attr.config == PERF_COUNT_HW_CPU_CYCLES) { >> cflags |= SBI_PMU_CFG_FLAG_SKIP_MATCH; >> cmask = 1; >> > Nice catch. > Reviewed-by: Atish Patra <atishp@rivosinc.com> Can you add a Fixes tag? No need to send another version, just reply with the corresponding tag and b4 will grab it. Thanks! Alex > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Alexandre, On Mon, 2024-07-29 at 20:17 +0200, Alexandre Ghiti wrote: > Hi Dmitry, > > On 29/07/2024 20:05, Atish Patra wrote: > > > It is required to check event type before checking event config. > > > Events with the different types can have the same config. > > > This check is missed for legacy mode code > > > > > > For such perf usage: > > > sysctl -w kernel.perf_user_access=2 > > > perf stat -e cycles,L1-dcache-loads -- > > > driver will try to force both events to CYCLE counter. > > > > > > This commit implements event type check before forcing > > > events on the special counters. > > > > > > Signed-off-by: Shifrin Dmitry <dmitry.shifrin@syntacore.com> > > > --- > > > drivers/perf/riscv_pmu_sbi.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > > index 36d128ff166f..bf14ab282e11 100644 > > > --- a/drivers/perf/riscv_pmu_sbi.c > > > +++ b/drivers/perf/riscv_pmu_sbi.c > > > @@ -416,7 +416,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) > > > * but not in the user access mode as we want to use the other counters > > > * that support sampling/filtering. > > > */ > > > - if (hwc->flags & PERF_EVENT_FLAG_LEGACY) { > > > + if ((hwc->flags & PERF_EVENT_FLAG_LEGACY) && (event->attr.type == PERF_TYPE_HARDWARE)) { > > > if (event->attr.config == PERF_COUNT_HW_CPU_CYCLES) { > > > cflags |= SBI_PMU_CFG_FLAG_SKIP_MATCH; > > > cmask = 1; > > > > > Nice catch. > > Reviewed-by: Atish Patra <atishp@rivosinc.com> > > > Can you add a Fixes tag? No need to send another version, just reply > with the corresponding tag and b4 will grab it. > Fixes: cc4c07c89aad ("drivers: perf: Implement perf event mmap support in the SBI backend") Thanks, Dmitry > Thanks! > > Alex > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Dmitry, On 2024-07-29 7:58 AM, Shifrin Dmitry wrote: > It is required to check event type before checking event config. > Events with the different types can have the same config. > This check is missed for legacy mode code > > For such perf usage: > sysctl -w kernel.perf_user_access=2 > perf stat -e cycles,L1-dcache-loads -- > driver will try to force both events to CYCLE counter. > > This commit implements event type check before forcing > events on the special counters. It looks like pmu_sbi_event_mapped() and pmu_sbi_event_mapped() have a similar bug where they do not check event->attr.type. Regards, Samuel > > Signed-off-by: Shifrin Dmitry <dmitry.shifrin@syntacore.com> > --- > drivers/perf/riscv_pmu_sbi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > index 36d128ff166f..bf14ab282e11 100644 > --- a/drivers/perf/riscv_pmu_sbi.c > +++ b/drivers/perf/riscv_pmu_sbi.c > @@ -416,7 +416,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) > * but not in the user access mode as we want to use the other counters > * that support sampling/filtering. > */ > - if (hwc->flags & PERF_EVENT_FLAG_LEGACY) { > + if ((hwc->flags & PERF_EVENT_FLAG_LEGACY) && (event->attr.type == PERF_TYPE_HARDWARE)) { > if (event->attr.config == PERF_COUNT_HW_CPU_CYCLES) { > cflags |= SBI_PMU_CFG_FLAG_SKIP_MATCH; > cmask = 1;
Hi Samuel, On Wed, 2024-07-31 at 02:37 -0500, Samuel Holland wrote: > Hi Dmitry, > > On 2024-07-29 7:58 AM, Shifrin Dmitry wrote: > > It is required to check event type before checking event config. > > Events with the different types can have the same config. > > This check is missed for legacy mode code > > > > For such perf usage: > > sysctl -w kernel.perf_user_access=2 > > perf stat -e cycles,L1-dcache-loads -- > > driver will try to force both events to CYCLE counter. > > > > This commit implements event type check before forcing > > events on the special counters. > > It looks like pmu_sbi_event_mapped() and pmu_sbi_event_mapped() have a similar > bug where they do not check event->attr.type. I am not sure that pmu_sbi_event_unmapped() and pmu_sbi_event_mapped() should be changed in the same way. These functions in legacy mode just set/clear PERF_EVENT_FLAG_USER_READ_CNT for legacy events without any mapping/unmapping. However in pmu_sbi_starting_cpu() CYCLE, TIME and INSTRET counters are always mapped to user space in legacy mode. So it looks like in legacy mode we can just use : if (event->hw.flags & PERF_EVENT_FLAG_LEGACY) : return; Thanks, Dmitry > > Regards, > Samuel > > > > > Signed-off-by: Shifrin Dmitry <dmitry.shifrin@syntacore.com> > > --- > > drivers/perf/riscv_pmu_sbi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > index 36d128ff166f..bf14ab282e11 100644 > > --- a/drivers/perf/riscv_pmu_sbi.c > > +++ b/drivers/perf/riscv_pmu_sbi.c > > @@ -416,7 +416,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) > > * but not in the user access mode as we want to use the other counters > > * that support sampling/filtering. > > */ > > - if (hwc->flags & PERF_EVENT_FLAG_LEGACY) { > > + if ((hwc->flags & PERF_EVENT_FLAG_LEGACY) && (event->attr.type == PERF_TYPE_HARDWARE)) { > > if (event->attr.config == PERF_COUNT_HW_CPU_CYCLES) { > > cflags |= SBI_PMU_CFG_FLAG_SKIP_MATCH; > > cmask = 1; >
Hello: This patch was applied to riscv/linux.git (fixes) by Palmer Dabbelt <palmer@rivosinc.com>: On Mon, 29 Jul 2024 15:58:58 +0300 you wrote: > It is required to check event type before checking event config. > Events with the different types can have the same config. > This check is missed for legacy mode code > > For such perf usage: > sysctl -w kernel.perf_user_access=2 > perf stat -e cycles,L1-dcache-loads -- > driver will try to force both events to CYCLE counter. > > [...] Here is the summary with links: - perf: riscv: Fix selecting counters in legacy mode https://git.kernel.org/riscv/c/941a8e9b7a86 You are awesome, thank you!
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c index 36d128ff166f..bf14ab282e11 100644 --- a/drivers/perf/riscv_pmu_sbi.c +++ b/drivers/perf/riscv_pmu_sbi.c @@ -416,7 +416,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) * but not in the user access mode as we want to use the other counters * that support sampling/filtering. */ - if (hwc->flags & PERF_EVENT_FLAG_LEGACY) { + if ((hwc->flags & PERF_EVENT_FLAG_LEGACY) && (event->attr.type == PERF_TYPE_HARDWARE)) { if (event->attr.config == PERF_COUNT_HW_CPU_CYCLES) { cflags |= SBI_PMU_CFG_FLAG_SKIP_MATCH; cmask = 1;
It is required to check event type before checking event config. Events with the different types can have the same config. This check is missed for legacy mode code For such perf usage: sysctl -w kernel.perf_user_access=2 perf stat -e cycles,L1-dcache-loads -- driver will try to force both events to CYCLE counter. This commit implements event type check before forcing events on the special counters. Signed-off-by: Shifrin Dmitry <dmitry.shifrin@syntacore.com> --- drivers/perf/riscv_pmu_sbi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)