* [PATCH v3 0/3] perf: RISC-V: misc fixes and improvements
@ 2022-08-26 20:34 Sergey Matyukevich
2022-08-26 20:34 ` [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array Sergey Matyukevich
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Sergey Matyukevich @ 2022-08-26 20:34 UTC (permalink / raw)
To: linux-riscv, Atish Patra, Mark Rutland, Will Deacon
Cc: Anup Patel, Albert Ou, Palmer Dabbelt, Paul Walmsley,
Sergey Matyukevich
Hi all,
It looks like the previous revision of these patches has slipped through
the cracks. So I am sending an updated version rebased on top of for-next
branch in riscv kernel tree.
Regards,
Sergey
v2 -> v3:
- rebased on top of for-next branch in riscv kernel tree
- added RB tag by Atish Patra to the first patch
- added perf throttle patch
Sergey Matyukevich (3):
perf: RISC-V: fix access beyond allocated array
perf: RISC-V: exclude invalid pmu counters from SBI calls
perf: RISC-V: throttle perf events
drivers/perf/riscv_pmu_legacy.c | 4 ++--
drivers/perf/riscv_pmu_sbi.c | 33 +++++++++++++++++++++------------
include/linux/perf/riscv_pmu.h | 2 +-
3 files changed, 24 insertions(+), 15 deletions(-)
--
2.37.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array 2022-08-26 20:34 [PATCH v3 0/3] perf: RISC-V: misc fixes and improvements Sergey Matyukevich @ 2022-08-26 20:34 ` Sergey Matyukevich 2022-08-26 22:06 ` Conor.Dooley 2022-08-26 20:34 ` [PATCH v3 2/3] perf: RISC-V: exclude invalid pmu counters from SBI calls Sergey Matyukevich 2022-08-26 20:34 ` [PATCH v3 3/3] perf: RISC-V: throttle perf events Sergey Matyukevich 2 siblings, 1 reply; 7+ messages in thread From: Sergey Matyukevich @ 2022-08-26 20:34 UTC (permalink / raw) To: linux-riscv, Atish Patra, Mark Rutland, Will Deacon Cc: Anup Patel, Albert Ou, Palmer Dabbelt, Paul Walmsley, Sergey Matyukevich, Sergey Matyukevich, Atish Patra From: Sergey Matyukevich <sergey.matyukevich@syntacore.com> The root cause could be related to the interpretation of the number of counters reported by SBI firmware. For instance, if we assume that unused timer counter with index 1 is not reported, then the range is correct and larger array needs to be allocated. This is not the case though since SBI firmware is supposed to report the total number of firmware and hardware counters including special or unused ones like the timer counter. So just fix the range in for-loop. Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com> Reviewed-by: Atish Patra <atishp@rivosinc.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 6f6681bbfd36..8de4ca2fef21 100644 --- a/drivers/perf/riscv_pmu_sbi.c +++ b/drivers/perf/riscv_pmu_sbi.c @@ -473,7 +473,7 @@ static int pmu_sbi_get_ctrinfo(int nctr) if (!pmu_ctr_list) return -ENOMEM; - for (i = 0; i <= nctr; i++) { + for (i = 0; i < nctr; i++) { ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_GET_INFO, i, 0, 0, 0, 0, 0); if (ret.error) /* The logical counter ids are not expected to be contiguous */ -- 2.37.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array 2022-08-26 20:34 ` [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array Sergey Matyukevich @ 2022-08-26 22:06 ` Conor.Dooley 2022-08-28 21:31 ` Sergey Matyukevich 0 siblings, 1 reply; 7+ messages in thread From: Conor.Dooley @ 2022-08-26 22:06 UTC (permalink / raw) To: geomatsi, linux-riscv, atishp, mark.rutland, will Cc: anup, aou, palmer, paul.walmsley, sergey.matyukevich, atishp On 26/08/2022 21:34, Sergey Matyukevich wrote: > [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array ^^^ (see below) > > From: Sergey Matyukevich <sergey.matyukevich@syntacore.com> > > The root cause could be related to the interpretation of the number of > counters reported by SBI firmware. For instance, if we assume that unused > timer counter with index 1 is not reported, then the range is correct > and larger array needs to be allocated. I found this to be confusingly worded, had to read it a few times before I understood what you meant. Maybe it's just late on a Friday, I think the theorycrafting about why the code looks how it does got me lol > This is not the case though since SBI firmware is supposed to report the > total number of firmware and hardware counters including special or > unused ones like the timer counter. So just fix the range in for-loop. ^^^ I see "fix" mentioned twice here, what commmit does it fix? Thanks, Conor. > > Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com> > Reviewed-by: Atish Patra <atishp@rivosinc.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 6f6681bbfd36..8de4ca2fef21 100644 > --- a/drivers/perf/riscv_pmu_sbi.c > +++ b/drivers/perf/riscv_pmu_sbi.c > @@ -473,7 +473,7 @@ static int pmu_sbi_get_ctrinfo(int nctr) > if (!pmu_ctr_list) > return -ENOMEM; > > - for (i = 0; i <= nctr; i++) { > + for (i = 0; i < nctr; i++) { > ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_GET_INFO, i, 0, 0, 0, 0, 0); > if (ret.error) > /* The logical counter ids are not expected to be contiguous */ > -- > 2.37.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array 2022-08-26 22:06 ` Conor.Dooley @ 2022-08-28 21:31 ` Sergey Matyukevich 2022-08-28 22:45 ` Conor.Dooley 0 siblings, 1 reply; 7+ messages in thread From: Sergey Matyukevich @ 2022-08-28 21:31 UTC (permalink / raw) To: Conor.Dooley Cc: linux-riscv, atishp, mark.rutland, will, anup, aou, palmer, paul.walmsley, sergey.matyukevich, atishp > > [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array > ^^^ (see below) > > > > From: Sergey Matyukevich <sergey.matyukevich@syntacore.com> > > > > The root cause could be related to the interpretation of the number of > > counters reported by SBI firmware. For instance, if we assume that unused > > timer counter with index 1 is not reported, then the range is correct > > and larger array needs to be allocated. > > I found this to be confusingly worded, had to read it a few times before > I understood what you meant. Maybe it's just late on a Friday, I think the > theorycrafting about why the code looks how it does got me lol Well, it was not exactly theorycrafting, see accompanying opensbi changes: http://lists.infradead.org/pipermail/opensbi/2022-June/002926.html I tried to provide some context for this one-liner, but failed to do so. > > This is not the case though since SBI firmware is supposed to report the > > total number of firmware and hardware counters including special or > > unused ones like the timer counter. So just fix the range in for-loop. > ^^^ > I see "fix" mentioned twice here, what commmit does it fix? That is the initial commit adding the whole SBI PMU driver, so it is a bit confusing to add 'Fixes' in this case. I think the following commit message should be sufficient: SBI firmware should report total number of firmware and hardware counters including unused ones or special ones. In this case the kernel doesn't need to make any assumptions about gaps in reported counters, e.g. excluded timer counter. That was fixed in OpenSBI v1.1 by commit 3f66465fb6bf ("lib: pmu: allow to use the highest available counter"). This kernel patch has no effect if SBI firmware behaves correctly. However it eliminates access beyond the allocated pmu_ctr_list if the kernel is used with OpenSBI older than v1.1. Regards, Sergey _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array 2022-08-28 21:31 ` Sergey Matyukevich @ 2022-08-28 22:45 ` Conor.Dooley 0 siblings, 0 replies; 7+ messages in thread From: Conor.Dooley @ 2022-08-28 22:45 UTC (permalink / raw) To: geomatsi Cc: linux-riscv, atishp, mark.rutland, will, anup, aou, palmer, paul.walmsley, sergey.matyukevich, atishp On 28/08/2022 22:31, Sergey Matyukevich wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>> [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array >> ^^^ (see below) >>> >>> From: Sergey Matyukevich <sergey.matyukevich@syntacore.com> >>> >>> The root cause could be related to the interpretation of the number of >>> counters reported by SBI firmware. For instance, if we assume that unused >>> timer counter with index 1 is not reported, then the range is correct >>> and larger array needs to be allocated. >> >> I found this to be confusingly worded, had to read it a few times before >> I understood what you meant. Maybe it's just late on a Friday, I think the >> theorycrafting about why the code looks how it does got me lol > > Well, it was not exactly theorycrafting, see accompanying opensbi changes: > http://lists.infradead.org/pipermail/opensbi/2022-June/002926.html > I tried to provide some context for this one-liner, but failed to do so. It may not have been theorycrafting, but it read like it was. Your proposed commit message below I think does a much better job of explaining why this needs to be fixed. > >>> This is not the case though since SBI firmware is supposed to report the >>> total number of firmware and hardware counters including special or >>> unused ones like the timer counter. So just fix the range in for-loop. >> ^^^ >> I see "fix" mentioned twice here, what commmit does it fix? > > That is the initial commit adding the whole SBI PMU driver, so it is a > bit confusing to add 'Fixes' in this case. I think the following commit > message should be sufficient: Fixes tags are good so we know how far back a fix should be backported. The driver was added in 5.18 AFAICT, but that's EOL so this fix would only need to be backported to 5.19. The appropriate fixes line would be: Fixes: e9991434596f ("RISC-V: Add perf platform driver based on SBI PMU extension") > > SBI firmware should report total number of firmware and hardware counters > including unused ones or special ones. In this case the kernel doesn't need > to make any assumptions about gaps in reported counters, e.g. excluded timer > counter. That was fixed in OpenSBI v1.1 by commit 3f66465fb6bf ("lib: pmu: > allow to use the highest available counter"). This kernel patch has no effect > if SBI firmware behaves correctly. However it eliminates access beyond the > allocated pmu_ctr_list if the kernel is used with OpenSBI older than v1.1. As I said above, I think this commit message does a far better job of explaining. For v4, could you please use this as the commit message and add the fixes line that I pasted above? Thanks, Conor. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 2/3] perf: RISC-V: exclude invalid pmu counters from SBI calls 2022-08-26 20:34 [PATCH v3 0/3] perf: RISC-V: misc fixes and improvements Sergey Matyukevich 2022-08-26 20:34 ` [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array Sergey Matyukevich @ 2022-08-26 20:34 ` Sergey Matyukevich 2022-08-26 20:34 ` [PATCH v3 3/3] perf: RISC-V: throttle perf events Sergey Matyukevich 2 siblings, 0 replies; 7+ messages in thread From: Sergey Matyukevich @ 2022-08-26 20:34 UTC (permalink / raw) To: linux-riscv, Atish Patra, Mark Rutland, Will Deacon Cc: Anup Patel, Albert Ou, Palmer Dabbelt, Paul Walmsley, Sergey Matyukevich, Sergey Matyukevich From: Sergey Matyukevich <sergey.matyukevich@syntacore.com> SBI firmware may not provide information for some counters in response to SBI_EXT_PMU_COUNTER_GET_INFO call. Exclude such counters from the subsequent SBI requests. For this purpose use global mask to keep track of fully specified counters. Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com> --- drivers/perf/riscv_pmu_legacy.c | 4 ++-- drivers/perf/riscv_pmu_sbi.c | 27 ++++++++++++++++----------- include/linux/perf/riscv_pmu.h | 2 +- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/drivers/perf/riscv_pmu_legacy.c b/drivers/perf/riscv_pmu_legacy.c index 342778782359..7d7131c47bc0 100644 --- a/drivers/perf/riscv_pmu_legacy.c +++ b/drivers/perf/riscv_pmu_legacy.c @@ -14,7 +14,6 @@ #define RISCV_PMU_LEGACY_CYCLE 0 #define RISCV_PMU_LEGACY_INSTRET 1 -#define RISCV_PMU_LEGACY_NUM_CTR 2 static bool pmu_init_done; @@ -83,7 +82,8 @@ static void pmu_legacy_init(struct riscv_pmu *pmu) { pr_info("Legacy PMU implementation is available\n"); - pmu->num_counters = RISCV_PMU_LEGACY_NUM_CTR; + pmu->cmask = BIT(RISCV_PMU_LEGACY_CYCLE) | + BIT(RISCV_PMU_LEGACY_INSTRET); pmu->ctr_start = pmu_legacy_ctr_start; pmu->ctr_stop = NULL; pmu->event_map = pmu_legacy_event_map; diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c index 8de4ca2fef21..bc7db9739d5a 100644 --- a/drivers/perf/riscv_pmu_sbi.c +++ b/drivers/perf/riscv_pmu_sbi.c @@ -271,7 +271,6 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) struct sbiret ret; int idx; uint64_t cbase = 0; - uint64_t cmask = GENMASK_ULL(rvpmu->num_counters - 1, 0); unsigned long cflags = 0; if (event->attr.exclude_kernel) @@ -281,11 +280,12 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) /* retrieve the available counter index */ #if defined(CONFIG_32BIT) - ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH, cbase, cmask, - cflags, hwc->event_base, hwc->config, hwc->config >> 32); + ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH, cbase, + rvpmu->cmask, cflags, hwc->event_base, hwc->config, + hwc->config >> 32); #else - ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH, cbase, cmask, - cflags, hwc->event_base, hwc->config, 0); + ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH, cbase, + rvpmu->cmask, cflags, hwc->event_base, hwc->config, 0); #endif if (ret.error) { pr_debug("Not able to find a counter for event %lx config %llx\n", @@ -294,7 +294,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) } idx = ret.value; - if (idx >= rvpmu->num_counters || !pmu_ctr_list[idx].value) + if (!test_bit(idx, &rvpmu->cmask) || !pmu_ctr_list[idx].value) return -ENOENT; /* Additional sanity check for the counter id */ @@ -463,7 +463,7 @@ static int pmu_sbi_find_num_ctrs(void) return sbi_err_map_linux_errno(ret.error); } -static int pmu_sbi_get_ctrinfo(int nctr) +static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask) { struct sbiret ret; int i, num_hw_ctr = 0, num_fw_ctr = 0; @@ -478,6 +478,9 @@ static int pmu_sbi_get_ctrinfo(int nctr) if (ret.error) /* The logical counter ids are not expected to be contiguous */ continue; + + *mask |= BIT(i); + cinfo.value = ret.value; if (cinfo.type == SBI_PMU_CTR_TYPE_FW) num_fw_ctr++; @@ -498,7 +501,7 @@ static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu) * which may include counters that are not enabled yet. */ sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP, - 0, GENMASK_ULL(pmu->num_counters - 1, 0), 0, 0, 0, 0); + 0, pmu->cmask, 0, 0, 0, 0); } static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu) @@ -788,8 +791,9 @@ static void riscv_pmu_destroy(struct riscv_pmu *pmu) static int pmu_sbi_device_probe(struct platform_device *pdev) { struct riscv_pmu *pmu = NULL; - int num_counters; + unsigned long cmask = 0; int ret = -ENODEV; + int num_counters; pr_info("SBI PMU extension is available\n"); pmu = riscv_pmu_alloc(); @@ -803,7 +807,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev) } /* cache all the information about counters now */ - if (pmu_sbi_get_ctrinfo(num_counters)) + if (pmu_sbi_get_ctrinfo(num_counters, &cmask)) goto out_free; ret = pmu_sbi_setup_irqs(pmu, pdev); @@ -812,8 +816,9 @@ static int pmu_sbi_device_probe(struct platform_device *pdev) pmu->pmu.capabilities |= PERF_PMU_CAP_NO_INTERRUPT; pmu->pmu.capabilities |= PERF_PMU_CAP_NO_EXCLUDE; } + pmu->pmu.attr_groups = riscv_pmu_attr_groups; - pmu->num_counters = num_counters; + pmu->cmask = cmask; pmu->ctr_start = pmu_sbi_ctr_start; pmu->ctr_stop = pmu_sbi_ctr_stop; pmu->event_map = pmu_sbi_event_map; diff --git a/include/linux/perf/riscv_pmu.h b/include/linux/perf/riscv_pmu.h index bf66fe011fa8..e17e86ad6f3a 100644 --- a/include/linux/perf/riscv_pmu.h +++ b/include/linux/perf/riscv_pmu.h @@ -45,7 +45,7 @@ struct riscv_pmu { irqreturn_t (*handle_irq)(int irq_num, void *dev); - int num_counters; + unsigned long cmask; u64 (*ctr_read)(struct perf_event *event); int (*ctr_get_idx)(struct perf_event *event); int (*ctr_get_width)(int idx); -- 2.37.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 3/3] perf: RISC-V: throttle perf events 2022-08-26 20:34 [PATCH v3 0/3] perf: RISC-V: misc fixes and improvements Sergey Matyukevich 2022-08-26 20:34 ` [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array Sergey Matyukevich 2022-08-26 20:34 ` [PATCH v3 2/3] perf: RISC-V: exclude invalid pmu counters from SBI calls Sergey Matyukevich @ 2022-08-26 20:34 ` Sergey Matyukevich 2 siblings, 0 replies; 7+ messages in thread From: Sergey Matyukevich @ 2022-08-26 20:34 UTC (permalink / raw) To: linux-riscv, Atish Patra, Mark Rutland, Will Deacon Cc: Anup Patel, Albert Ou, Palmer Dabbelt, Paul Walmsley, Sergey Matyukevich, Sergey Matyukevich From: Sergey Matyukevich <sergey.matyukevich@syntacore.com> Call perf_sample_event_took() to report time spent in overflow interrupts. Perf core uses these measurements to throttle perf events properly. Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com> --- drivers/perf/riscv_pmu_sbi.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c index bc7db9739d5a..15e5a47be7d5 100644 --- a/drivers/perf/riscv_pmu_sbi.c +++ b/drivers/perf/riscv_pmu_sbi.c @@ -18,6 +18,7 @@ #include <linux/of_irq.h> #include <linux/of.h> #include <linux/cpu_pm.h> +#include <linux/sched/clock.h> #include <asm/sbi.h> #include <asm/hwcap.h> @@ -570,6 +571,7 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev) unsigned long overflow; unsigned long overflowed_ctrs = 0; struct cpu_hw_events *cpu_hw_evt = dev; + u64 start_clock = sched_clock(); if (WARN_ON_ONCE(!cpu_hw_evt)) return IRQ_NONE; @@ -638,7 +640,9 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev) perf_event_overflow(event, &data, regs); } } + pmu_sbi_start_overflow_mask(pmu, overflowed_ctrs); + perf_sample_event_took(sched_clock() - start_clock); return IRQ_HANDLED; } -- 2.37.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-08-28 22:45 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-26 20:34 [PATCH v3 0/3] perf: RISC-V: misc fixes and improvements Sergey Matyukevich 2022-08-26 20:34 ` [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array Sergey Matyukevich 2022-08-26 22:06 ` Conor.Dooley 2022-08-28 21:31 ` Sergey Matyukevich 2022-08-28 22:45 ` Conor.Dooley 2022-08-26 20:34 ` [PATCH v3 2/3] perf: RISC-V: exclude invalid pmu counters from SBI calls Sergey Matyukevich 2022-08-26 20:34 ` [PATCH v3 3/3] perf: RISC-V: throttle perf events Sergey Matyukevich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.