AUX ring buffer applies almost the same barriers as perf ring buffer, but there has an exception for ordering between writing the AUX trace data and updating user_page::aux_head.

This patch adds comment for how to use the barriers on AUX ring buffer, and gives comment to ask the drivers to flush the trace data into AUX ring buffer prior to updating user_page::aux_head.

Signed-off-by: Leo Yan leo.yan@linaro.org Acked-by: Peter Zijlstra (Intel) peterz@infradead.org --- kernel/events/ring_buffer.c | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c index 52868716ec35..5cf6579be05e 100644 --- a/kernel/events/ring_buffer.c +++ b/kernel/events/ring_buffer.c @@ -509,6 +509,15 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size) perf_event_aux_event(handle->event, aux_head, size, handle->aux_flags);

+ /* + * See perf_output_put_handle(), AUX ring buffer applies the same + * barrier pairing as the perf ring buffer; except for B, since + * AUX ring buffer is written by hardware trace, we cannot simply + * use the generic memory barrier (like smp_wmb()) prior to update + * user_page::aux_head, the hardware trace driver takes the + * responsibility to ensure the trace data has been flushed into + * the AUX buffer before calling perf_aux_output_end(). + */ WRITE_ONCE(rb->user_page->aux_head, rb->aux_head); if (rb_need_aux_wakeup(rb)) wakeup = true;

AUX ring buffer is required to separate the data store and aux_head store, since the function CS_LOCK() has contained memory barrier mb(), mb() is a more conservative barrier than smp_wmb() on Arm32/Arm64, thus it's needless to add any explicit barrier anymore.

Add comment to make clear for the barrier usage for ETF.

Signed-off-by: Leo Yan leo.yan@linaro.org --- drivers/hwtracing/coresight/coresight-tmc-etf.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index 45b85edfc690..9a42ee689921 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -553,6 +553,12 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, if (buf->snapshot) handle->head += to_read;

+ /* + * AUX ring buffer requires to use memory barrier to separate the trace + * data store and aux_head store, because CS_LOCK() contains mb() which + * gives more heavy barrier than smp_wmb(), it's not necessary to + * explicitly invoke any barrier. + */ CS_LOCK(drvdata->base); out: spin_unlock_irqrestore(&drvdata->spinlock, flags);

Use WRITE_ONCE() for updating aux_tail, so can avoid unexpected memory behaviour.

Signed-off-by: Leo Yan leo.yan@linaro.org Acked-by: Adrian Hunter adrian.hunter@intel.com Acked-by: Peter Zijlstra (Intel) peterz@infradead.org --- tools/perf/util/auxtrace.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h index cc1c1b9cec9c..79227b8864cd 100644 --- a/tools/perf/util/auxtrace.h +++ b/tools/perf/util/auxtrace.h @@ -480,7 +480,7 @@ static inline void auxtrace_mmap__write_tail(struct auxtrace_mmap *mm, u64 tail) /* Ensure all reads are done before we write the tail out */ smp_mb(); #if BITS_PER_LONG == 64 || !defined(HAVE_SYNC_COMPARE_AND_SWAP_SUPPORT) - pc->aux_tail = tail; + WRITE_ONCE(pc->aux_tail, tail); #else do { old_tail = __sync_val_compare_and_swap(&pc->aux_tail, 0, 0);

Since the function auxtrace_mmap__read_snapshot_head() is exactly same with auxtrace_mmap__read_head(), whether the session is in snapshot mode or not, it's unified to use function auxtrace_mmap__read_head() for reading AUX buffer head.

And the function auxtrace_mmap__read_snapshot_head() is unused so this patch removes it.

Signed-off-by: Leo Yan leo.yan@linaro.org --- tools/perf/util/auxtrace.c | 5 ++--- tools/perf/util/auxtrace.h | 10 ---------- 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c index cb19669d2a5b..7958e17229ea 100644 --- a/tools/perf/util/auxtrace.c +++ b/tools/perf/util/auxtrace.c @@ -1686,13 +1686,12 @@ static int __auxtrace_mmap__read(struct mmap *map, union perf_event ev; void *data1, *data2;

+ head = auxtrace_mmap__read_head(mm); + if (snapshot) { - head = auxtrace_mmap__read_snapshot_head(mm); if (auxtrace_record__find_snapshot(itr, mm->idx, mm, data, &head, &old)) return -1; - } else { - head = auxtrace_mmap__read_head(mm); }

if (old == head) diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h index 4f9176368134..d68a5e80b217 100644 --- a/tools/perf/util/auxtrace.h +++ b/tools/perf/util/auxtrace.h @@ -440,16 +440,6 @@ struct auxtrace_cache;

#ifdef HAVE_AUXTRACE_SUPPORT

-static inline u64 auxtrace_mmap__read_snapshot_head(struct auxtrace_mmap *mm) -{ - struct perf_event_mmap_page *pc = mm->userpg; - u64 head = READ_ONCE(pc->aux_head); - - /* Ensure all reads are done after we read the head */ - smp_rmb(); - return head; -} - static inline u64 auxtrace_mmap__read_head(struct auxtrace_mmap *mm) { struct perf_event_mmap_page *pc = mm->userpg;

Since the __sync functions have been removed from perf, it's needless for perf tool to test the feature sync-compare-and-swap.

The feature test is not used by any other components, remove it.

Signed-off-by: Leo Yan leo.yan@linaro.org --- tools/build/Makefile.feature | 1 - tools/build/feature/Makefile | 4 ---- tools/build/feature/test-all.c | 4 ---- tools/build/feature/test-sync-compare-and-swap.c | 15 --------------- 4 files changed, 24 deletions(-) delete mode 100644 tools/build/feature/test-sync-compare-and-swap.c

diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature index 04a8e3db8a54..3dd2f68366f9 100644 --- a/tools/build/Makefile.feature +++ b/tools/build/Makefile.feature @@ -34,7 +34,6 @@ FEATURE_TESTS_BASIC := \ dwarf_getlocations \ eventfd \ fortify-source \ - sync-compare-and-swap \ get_current_dir_name \ gettid \ glibc \ diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile index ec203e28407f..eff55d287db1 100644 --- a/tools/build/feature/Makefile +++ b/tools/build/feature/Makefile @@ -9,7 +9,6 @@ FILES= \ test-dwarf_getlocations.bin \ test-eventfd.bin \ test-fortify-source.bin \ - test-sync-compare-and-swap.bin \ test-get_current_dir_name.bin \ test-glibc.bin \ test-gtk2.bin \ @@ -260,9 +259,6 @@ $(OUTPUT)test-libdw-dwarf-unwind.bin: $(OUTPUT)test-libbabeltrace.bin: $(BUILD) # -lbabeltrace provided by $(FEATURE_CHECK_LDFLAGS-libbabeltrace)

-$(OUTPUT)test-sync-compare-and-swap.bin: - $(BUILD) - $(OUTPUT)test-compile-32.bin: $(CC) -m32 -o $@ test-compile.c

diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c index 464873883396..920439527291 100644 --- a/tools/build/feature/test-all.c +++ b/tools/build/feature/test-all.c @@ -106,10 +106,6 @@ # include "test-libdw-dwarf-unwind.c" #undef main

-#define main main_test_sync_compare_and_swap -# include "test-sync-compare-and-swap.c" -#undef main - #define main main_test_zlib # include "test-zlib.c" #undef main diff --git a/tools/build/feature/test-sync-compare-and-swap.c b/tools/build/feature/test-sync-compare-and-swap.c deleted file mode 100644 index 3bc6b0768a53..000000000000 --- a/tools/build/feature/test-sync-compare-and-swap.c +++ /dev/null @@ -1,15 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0 -#include <stdint.h> - -volatile uint64_t x; - -int main(int argc, char *argv[]) -{ - uint64_t old, new = argc; - - (void)argv; - do { - old = __sync_val_compare_and_swap(&x, 0, 0); - } while (!__sync_bool_compare_and_swap(&x, old, new)); - return old == new; -}

When perf runs in compat mode (kernel in 64-bit mode and the perf is in 32-bit mode), the 64-bit value atomicity in the user space cannot be assured, E.g. on some architectures, the 64-bit value accessing is split into two instructions, one is for the low 32-bit word accessing and another is for the high 32-bit word.

This patch introduces two functions compat_auxtrace_mmap__read_head() and compat_auxtrace_mmap__write_tail(), as their naming indicates, when perf tool works in compat mode, it uses these two functions to access the AUX head and tail. These two functions can allow the perf tool to work properly in certain conditions, e.g. when perf tool works in snapshot mode with only using AUX head pointer, or perf tool uses the AUX buffer and the incremented tail is not bigger than 4GB.

When perf tool cannot handle the case when the AUX tail is bigger than 4GB, the function compat_auxtrace_mmap__write_tail() returns -1 and tells the caller to bail out for the error.

Suggested-by: Adrian Hunter adrian.hunter@intel.com Signed-off-by: Leo Yan leo.yan@linaro.org --- tools/perf/util/auxtrace.c | 9 ++-- tools/perf/util/auxtrace.h | 94 +++++++++++++++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c index 6a63be8b2430..d6fc250fbf97 100644 --- a/tools/perf/util/auxtrace.c +++ b/tools/perf/util/auxtrace.c @@ -1766,10 +1766,13 @@ static int __auxtrace_mmap__read(struct mmap *map, mm->prev = head;

if (!snapshot) { - auxtrace_mmap__write_tail(mm, head); - if (itr->read_finish) { - int err; + int err;

+ err = auxtrace_mmap__write_tail(mm, head); + if (err < 0) + return err; + + if (itr->read_finish) { err = itr->read_finish(itr, mm->idx); if (err < 0) return err; diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h index d68a5e80b217..66de7b6e65ec 100644 --- a/tools/perf/util/auxtrace.h +++ b/tools/perf/util/auxtrace.h @@ -18,6 +18,8 @@ #include <asm/bitsperlong.h> #include <asm/barrier.h>

+#include "env.h" + union perf_event; struct perf_session; struct evlist; @@ -440,23 +442,111 @@ struct auxtrace_cache;

#ifdef HAVE_AUXTRACE_SUPPORT

+/* + * In the compat mode kernel runs in 64-bit and perf tool runs in 32-bit mode, + * 32-bit perf tool cannot access 64-bit value atomically, which might lead to + * the issues caused by the below sequence on multiple CPUs: when perf tool + * accesses either the load operation or the store operation for 64-bit value, + * on some architectures the operation is divided into two instructions, one + * is for accessing the low 32-bit value and another is for the high 32-bit; + * thus these two user operations can give the kernel chances to access the + * 64-bit value, and thus leads to the unexpected load values. + * + * kernel (64-bit) user (32-bit) + * + * if (LOAD ->aux_tail) { --, LOAD ->aux_head_lo + * STORE $aux_data | ,---> + * FLUSH $aux_data | | LOAD ->aux_head_hi + * STORE ->aux_head --|-------` smp_rmb() + * } | LOAD $data + * | smp_mb() + * | STORE ->aux_tail_lo + * `-----------> + * STORE ->aux_tail_hi + * + * For this reason, it's impossible for the perf tool to work correctly when + * the AUX head or tail is bigger than 4GB (more than 32 bits length); and we + * can not simply limit the AUX ring buffer to less than 4GB, the reason is + * the pointers can be increased monotonically (e.g in snapshot mode), whatever + * the buffer size it is, at the end the head and tail can be bigger than 4GB + * and carry out to the high 32-bit. + * + * To mitigate the issues and improve the user experience, we can allow the + * perf tool working in certain conditions and bail out with error if detect + * any overflow cannot be handled. + * + * For reading the AUX head, it reads out the values for three times, and + * compares the high 4 bytes of the values between the first time and the last + * time, if there has no change for high 4 bytes injected by the kernel during + * the user reading sequence, it's safe for use the second value. + * + * When update the AUX tail and detects any carrying in the high 32 bits, it + * means there have two store operations in user space and it cannot promise + * the atomicity for 64-bit write, so return '-1' in this case to tell the + * caller an overflow error has happened. + */ +static inline u64 compat_auxtrace_mmap__read_head(struct auxtrace_mmap *mm) +{ + struct perf_event_mmap_page *pc = mm->userpg; + u64 first, second, last; + u64 mask = (u64)(UINT32_MAX) << 32; + + do { + first = READ_ONCE(pc->aux_head); + /* Ensure all reads are done after we read the head */ + smp_rmb(); + second = READ_ONCE(pc->aux_head); + /* Ensure all reads are done after we read the head */ + smp_rmb(); + last = READ_ONCE(pc->aux_head); + } while ((first & mask) != (last & mask)); + + return second; +} + +static inline int compat_auxtrace_mmap__write_tail(struct auxtrace_mmap *mm, + u64 tail) +{ + struct perf_event_mmap_page *pc = mm->userpg; + u64 mask = (u64)(UINT32_MAX) << 32; + + if (tail & mask) + return -1; + + /* Ensure all reads are done before we write the tail out */ + smp_mb(); + WRITE_ONCE(pc->aux_tail, tail); + return 0; +} + static inline u64 auxtrace_mmap__read_head(struct auxtrace_mmap *mm) { struct perf_event_mmap_page *pc = mm->userpg; - u64 head = READ_ONCE(pc->aux_head); + u64 head; + +#if BITS_PER_LONG == 32 + if (kernel_is_64_bit) + return compat_auxtrace_mmap__read_head(mm); +#endif + head = READ_ONCE(pc->aux_head);

/* Ensure all reads are done after we read the head */ smp_rmb(); return head; }

-static inline void auxtrace_mmap__write_tail(struct auxtrace_mmap *mm, u64 tail) +static inline int auxtrace_mmap__write_tail(struct auxtrace_mmap *mm, u64 tail) { struct perf_event_mmap_page *pc = mm->userpg;

+#if BITS_PER_LONG == 32 + if (kernel_is_64_bit) + return compat_auxtrace_mmap__write_tail(mm, tail); +#endif /* Ensure all reads are done before we write the tail out */ smp_mb(); WRITE_ONCE(pc->aux_tail, tail); + return 0; }

int auxtrace_mmap__mmap(struct auxtrace_mmap *mm,