| Message ID | 20240611155012.2286044-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: arm_pmuv3: Avoid assigning fixed cycle counter with threshold | expand |
On 11/06/2024 16:50, Rob Herring (Arm) wrote: > If the user has requested a counting threshold for the CPU cycles event, > then the fixed cycle counter can't be assigned as it lacks threshold > support. Currently, the thresholds will work or not randomly depending > on which counter the event is assigned. > > While using thresholds for CPU cycles doesn't make much sense, it can be > useful for testing purposes. > > Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold") > Signed-off-by: Rob Herring (Arm) <robh@kernel.org> > --- > drivers/perf/arm_pmuv3.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c > index 23fa6c5da82c..2612be29ee23 100644 > --- a/drivers/perf/arm_pmuv3.c > +++ b/drivers/perf/arm_pmuv3.c > @@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc, > struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > struct hw_perf_event *hwc = &event->hw; > unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT; > + bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH); I was going to say doesn't it need to be (ARMV8_PMU_EVTYPE_TH | ARMV8_PMU_EVTYPE_TC) for it to give the same results as the hardware. But then I saw we only enable it if TH != 0, even if TC is set. And now I'm wondering if I inadvertently disabled a useful combination of options. The Arm ARM says it's only completely disabled when both TC and TH are 0. > > /* Always prefer to place a cycle counter into the cycle counter. */ > - if (evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) { > + if ((evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) && !has_threshold) { > if (!test_and_set_bit(ARMV8_IDX_CYCLE_COUNTER, cpuc->used_mask)) > return ARMV8_IDX_CYCLE_COUNTER; > else if (armv8pmu_event_is_64bit(event) &&
On Tue, Jun 11, 2024 at 09:50:12AM -0600, Rob Herring (Arm) wrote: > If the user has requested a counting threshold for the CPU cycles event, > then the fixed cycle counter can't be assigned as it lacks threshold > support. Currently, the thresholds will work or not randomly depending > on which counter the event is assigned. > > While using thresholds for CPU cycles doesn't make much sense, it can be > useful for testing purposes. > > Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold") > Signed-off-by: Rob Herring (Arm) <robh@kernel.org> > --- > drivers/perf/arm_pmuv3.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c > index 23fa6c5da82c..2612be29ee23 100644 > --- a/drivers/perf/arm_pmuv3.c > +++ b/drivers/perf/arm_pmuv3.c > @@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc, > struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > struct hw_perf_event *hwc = &event->hw; > unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT; > + bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH); Just a nit, but I don't think you need the '!!' here. Will
On 11/06/2024 17:13, James Clark wrote: > > > On 11/06/2024 16:50, Rob Herring (Arm) wrote: >> If the user has requested a counting threshold for the CPU cycles event, >> then the fixed cycle counter can't be assigned as it lacks threshold >> support. Currently, the thresholds will work or not randomly depending >> on which counter the event is assigned. >> >> While using thresholds for CPU cycles doesn't make much sense, it can be >> useful for testing purposes. >> >> Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold") >> Signed-off-by: Rob Herring (Arm) <robh@kernel.org> >> --- >> drivers/perf/arm_pmuv3.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c >> index 23fa6c5da82c..2612be29ee23 100644 >> --- a/drivers/perf/arm_pmuv3.c >> +++ b/drivers/perf/arm_pmuv3.c >> @@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc, >> struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); >> struct hw_perf_event *hwc = &event->hw; >> unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT; >> + bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH); > > I was going to say doesn't it need to be (ARMV8_PMU_EVTYPE_TH | > ARMV8_PMU_EVTYPE_TC) for it to give the same results as the hardware. > But then I saw we only enable it if TH != 0, even if TC is set. And now > I'm wondering if I inadvertently disabled a useful combination of options. > > The Arm ARM says it's only completely disabled when both TC and TH are 0. > If it's easy it might be worth adding a helper function for has_threshold() that's used in both places. That way if or when this issue gets fixed up it doesn't break here. >> >> /* Always prefer to place a cycle counter into the cycle counter. */ >> - if (evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) { >> + if ((evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) && !has_threshold) { >> if (!test_and_set_bit(ARMV8_IDX_CYCLE_COUNTER, cpuc->used_mask)) >> return ARMV8_IDX_CYCLE_COUNTER; >> else if (armv8pmu_event_is_64bit(event) &&
On Tue, Jun 18, 2024 at 10:30 AM James Clark <james.clark@arm.com> wrote: > > > > On 11/06/2024 17:13, James Clark wrote: > > > > > > On 11/06/2024 16:50, Rob Herring (Arm) wrote: > >> If the user has requested a counting threshold for the CPU cycles event, > >> then the fixed cycle counter can't be assigned as it lacks threshold > >> support. Currently, the thresholds will work or not randomly depending > >> on which counter the event is assigned. > >> > >> While using thresholds for CPU cycles doesn't make much sense, it can be > >> useful for testing purposes. > >> > >> Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold") > >> Signed-off-by: Rob Herring (Arm) <robh@kernel.org> > >> --- > >> drivers/perf/arm_pmuv3.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c > >> index 23fa6c5da82c..2612be29ee23 100644 > >> --- a/drivers/perf/arm_pmuv3.c > >> +++ b/drivers/perf/arm_pmuv3.c > >> @@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc, > >> struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > >> struct hw_perf_event *hwc = &event->hw; > >> unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT; > >> + bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH); > > > > I was going to say doesn't it need to be (ARMV8_PMU_EVTYPE_TH | > > ARMV8_PMU_EVTYPE_TC) for it to give the same results as the hardware. > > But then I saw we only enable it if TH != 0, even if TC is set. And now > > I'm wondering if I inadvertently disabled a useful combination of options. > > > > The Arm ARM says it's only completely disabled when both TC and TH are 0. > > > > If it's easy it might be worth adding a helper function for > has_threshold() that's used in both places. That way if or when this > issue gets fixed up it doesn't break here. The other place being in armv8pmu_set_event_filter()? A helper doesn't help there because that looks at the attr value, not config_base. Rob
On Tue, Jun 18, 2024 at 9:41 AM Will Deacon <will@kernel.org> wrote: > > On Tue, Jun 11, 2024 at 09:50:12AM -0600, Rob Herring (Arm) wrote: > > If the user has requested a counting threshold for the CPU cycles event, > > then the fixed cycle counter can't be assigned as it lacks threshold > > support. Currently, the thresholds will work or not randomly depending > > on which counter the event is assigned. > > > > While using thresholds for CPU cycles doesn't make much sense, it can be > > useful for testing purposes. > > > > Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold") > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org> > > --- > > drivers/perf/arm_pmuv3.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c > > index 23fa6c5da82c..2612be29ee23 100644 > > --- a/drivers/perf/arm_pmuv3.c > > +++ b/drivers/perf/arm_pmuv3.c > > @@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc, > > struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > > struct hw_perf_event *hwc = &event->hw; > > unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT; > > + bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH); > > Just a nit, but I don't think you need the '!!' here. Right, I guess since bool is a first class type in C9X we don't have to worry about truncation. Old habits... Rob
diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c index 23fa6c5da82c..2612be29ee23 100644 --- a/drivers/perf/arm_pmuv3.c +++ b/drivers/perf/arm_pmuv3.c @@ -939,9 +939,10 @@ static int armv8pmu_get_event_idx(struct pmu_hw_events *cpuc, struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); struct hw_perf_event *hwc = &event->hw; unsigned long evtype = hwc->config_base & ARMV8_PMU_EVTYPE_EVENT; + bool has_threshold = !!(hwc->config_base & ARMV8_PMU_EVTYPE_TH); /* Always prefer to place a cycle counter into the cycle counter. */ - if (evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) { + if ((evtype == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) && !has_threshold) { if (!test_and_set_bit(ARMV8_IDX_CYCLE_COUNTER, cpuc->used_mask)) return ARMV8_IDX_CYCLE_COUNTER; else if (armv8pmu_event_is_64bit(event) &&
If the user has requested a counting threshold for the CPU cycles event, then the fixed cycle counter can't be assigned as it lacks threshold support. Currently, the thresholds will work or not randomly depending on which counter the event is assigned. While using thresholds for CPU cycles doesn't make much sense, it can be useful for testing purposes. Fixes: 816c26754447 ("arm64: perf: Add support for event counting threshold") Signed-off-by: Rob Herring (Arm) <robh@kernel.org> --- drivers/perf/arm_pmuv3.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)