Re: [PATCH V5 01/12] perf/core: Add aux_pause, aux_resume, aux_start_paused
From: Adrian Hunter <adrian.hunter@intel.com>
Date: 2024-02-09 08:14:54
Also in:
linux-perf-users, lkml
On 9/02/24 02:13, Andi Kleen wrote:
quoted
+static void __perf_event_aux_pause(struct perf_event *event, bool pause) +{ + if (pause) { + if (!READ_ONCE(event->aux_paused)) { + WRITE_ONCE(event->aux_paused, 1); + event->pmu->stop(event, PERF_EF_PAUSE); + } + } else { + if (READ_ONCE(event->aux_paused)) { + WRITE_ONCE(event->aux_paused, 0); + event->pmu->start(event, PERF_EF_RESUME); + }This doesn't look atomic. Either the READ/WRITE once are not needed, or you need an actually atomic construct.
Yes READ_ONCE / WRITE_ONCE is not really needed here.
quoted
+ + rb = ring_buffer_get(event); + if (!rb) + return; + + local_irq_save(flags); + /* Guard against NMI, NMI loses here */ + if (READ_ONCE(rb->aux_in_pause_resume)) + goto out_restore; + WRITE_ONCE(rb->aux_in_pause_resume, 1);quoted
+ barrier(); + __perf_event_aux_pause(event, pause); + barrier(); + WRITE_ONCE(rb->aux_in_pause_resume, 0);Dito.
The writes to rb->aux_in_pause_resume must be done only once. It might be possible to get away without WRITE_ONCE(), but really the compiler should be informed not to make assumptions. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel