Messages in this thread Patch in this message | | | Date | Thu, 30 Nov 2023 09:28:23 +0530 | Subject | Re: [V14 3/8] drivers: perf: arm_pmuv3: Enable branch stack sampling framework | From | Anshuman Khandual <> |
| |
On 11/14/23 22:40, James Clark wrote: > > On 14/11/2023 05:13, Anshuman Khandual wrote: > [...] >> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c >> index d712a19e47ac..76f1376ae594 100644 >> --- a/drivers/perf/arm_pmu.c >> +++ b/drivers/perf/arm_pmu.c >> @@ -317,6 +317,15 @@ armpmu_del(struct perf_event *event, int flags) >> struct hw_perf_event *hwc = &event->hw; >> int idx = hwc->idx; >> >> + if (has_branch_stack(event)) { >> + WARN_ON_ONCE(!hw_events->brbe_users); >> + hw_events->brbe_users--; >> + if (!hw_events->brbe_users) { >> + hw_events->brbe_context = NULL; >> + hw_events->brbe_sample_type = 0; >> + } >> + } >> + >> armpmu_stop(event, PERF_EF_UPDATE); >> hw_events->events[idx] = NULL; >> armpmu->clear_event_idx(hw_events, event); >> @@ -333,6 +342,22 @@ armpmu_add(struct perf_event *event, int flags) >> struct hw_perf_event *hwc = &event->hw; >> int idx; >> >> + if (has_branch_stack(event)) { >> + /* >> + * Reset branch records buffer if a new task event gets >> + * scheduled on a PMU which might have existing records. >> + * Otherwise older branch records present in the buffer >> + * might leak into the new task event. >> + */ >> + if (event->ctx->task && hw_events->brbe_context != event->ctx) { >> + hw_events->brbe_context = event->ctx; >> + if (armpmu->branch_reset) >> + armpmu->branch_reset(); > What about a per-thread event following a per-cpu event? Doesn't that > also need to branch_reset()? If hw_events->brbe_context was already > previously assigned, once the per-thread event is switched in it skips > this reset following a per-cpu event on the same core.
Right, guess it is real a possibility. How about folding in something like ..
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c index 76f1376ae594..15bb80823ae6 100644 --- a/drivers/perf/arm_pmu.c +++ b/drivers/perf/arm_pmu.c @@ -343,6 +343,22 @@ armpmu_add(struct perf_event *event, int flags) int idx; if (has_branch_stack(event)) { + /* + * Reset branch records buffer if a new CPU bound event + * gets scheduled on a PMU. Otherwise existing branch + * records present in the buffer might just leak into + * such events. + * + * Also reset current 'hw_events->brbe_context' because + * any previous task bound event now would have lost an + * opportunity for continuous branch records. + */ + if (!event->ctx->task) { + hw_events->brbe_context = NULL; + if (armpmu->branch_reset) + armpmu->branch_reset(); + } + /* * Reset branch records buffer if a new task event gets * scheduled on a PMU which might have existing records.
| |