Hi Leo
On Wed, 2 Apr 2025 at 13:31, Leo Yan leo.yan@arm.com wrote:
Hi Mike,
On Wed, Apr 02, 2025 at 09:45:20AM +0100, Mike Leach wrote:
[...]
static void etm_event_start(struct perf_event *event, int flags) { int cpu = smp_processor_id(); @@ -463,6 +484,14 @@ static void etm_event_start(struct perf_event *event, int flags) if (!csdev) goto fail;
Is it possible here that the first call to etm_event_start() also has the PERF_EF_RESUME flag set?
The first call has a flow below, using flag 0 but not PERF_EF_RESUME.
etm_event_add() `> etm_event_start(event, 0);
When I looked at the vague comments in the perf source - it seemed to imply that ->start() calls could overlap - so the associated event that resumes trace could occur at the same time as the initialising start from paused for the trace operations.
Good point. A subtle but important thing is the 'cs_etm' event must be an event group leader, otherwise, the tool reports error:
# perf record -m,64M -e cycles/aux-action=pause,period=10000000/ \ -e cycles/aux-action=resume,period=10/ \ -e cs_etm/aux-action=start-paused/u -- /mnt/sort Events with aux-action must have AUX area event group leader
If the 'cs_etm' event is the event group leader, it will be always enabled ahead other PMU events. So etm_event_start(event, 0) is invoked prior to PMU events enabling. As a result, this can avoid the race condition you mentioned.
I confirmed with ftrace log:
sort-901 [005] d..3. 1033.827186: etm_event_add <-event_sched_in sort-901 [005] d..3. 1033.827187: etm_event_start <-etm_event_add sort-901 [005] d..3. 1033.827283: armpmu_add <-event_sched_in sort-901 [005] d..3. 1033.827287: armpmu_start <-armpmu_add sort-901 [005] d..3. 1033.827288: armpmu_event_set_period <-armpmu_start sort-901 [005] d..3. 1033.827292: armpmu_add <-event_sched_in sort-901 [005] d..3. 1033.827293: armpmu_start <-armpmu_add sort-901 [005] d..3. 1033.827294: armpmu_event_set_period <-armpmu_start sort-901 [005] d..3. 1033.827298: armpmu_filter <-visit_groups_merge.constprop.0.isra.0 sort-901 [005] d..3. 1033.827298: armpmu_enable <-perf_pmu_enable sort-901 [005] d..3. 1033.827301: armpmu_enable <-perf_pmu_enable sort-901 [005] d.h1. 1033.827304: armpmu_dispatch_irq <-__handle_irq_event_percpu sort-901 [005] d.h1. 1033.827306: armpmu_event_update <-armv8pmu_handle_irq sort-901 [005] d.h1. 1033.827308: armpmu_event_set_period <-armv8pmu_handle_irq sort-901 [005] d.h.. 1033.827322: perf_event_aux_pause: event=ffff000207503e40 pause=0
If we are guaranteed this cannot happen then we are good to go!
Now I think we are safe, right? ;)
Thanks, Leo
I'd agree with that!
Mike