[PATCH v5 bpf-next 0/3] bpf: introduce bpf_get_branch_snapshot

[PATCH v5 bpf-next 0/3] bpf: introduce bpf_get_branch_snapshot

[Song Liu]

Changes v4 => v5
1. Modify perf_snapshot_branch_stack_t to save some memcpy. (Andrii)
2. Minor fixes in selftests. (Andrii)

Changes v3 => v4:
1. Do not reshuffle intel_pmu_disable_all(). Use some inline to save LBR
   entries. (Peter)
2. Move static_call(perf_snapshot_branch_stack) to the helper. (Alexei)
3. Add argument flags to bpf_get_branch_snapshot. (Andrii)
4. Make MAX_BRANCH_SNAPSHOT an enum (Andrii). And rename it as
   PERF_MAX_BRANCH_SNAPSHOT
5. Make bpf_get_branch_snapshot similar to bpf_read_branch_records.
   (Andrii)
6. Move the test target function to bpf_testmod. Updated kallsyms_find_next
   to work properly with modules. (Andrii)

Changes v2 => v3:
1. Fix the use of static_call. (Peter)
2. Limit the use to perfmon version >= 2. (Peter)
3. Modify intel_pmu_snapshot_branch_stack() to use intel_pmu_disable_all
   and intel_pmu_enable_all().

Changes v1 => v2:
1. Rename the helper as bpf_get_branch_snapshot;
2. Fix/simplify the use of static_call;
3. Instead of percpu variables, let intel_pmu_snapshot_branch_stack output
   branch records to an output argument of type perf_branch_snapshot.

Branch stack can be very useful in understanding software events. For
example, when a long function, e.g. sys_perf_event_open, returns an errno,
it is not obvious why the function failed. Branch stack could provide very
helpful information in this type of scenarios.

This set adds support to read branch stack with a new BPF helper
bpf_get_branch_trace(). Currently, this is only supported in Intel systems.
It is also possible to support the same feaure for PowerPC.

The hardware that records the branch stace is not stopped automatically on
software events. Therefore, it is necessary to stop it in software soon.
Otherwise, the hardware buffers/registers will be flushed. One of the key
design consideration in this set is to minimize the number of branch record
entries between the event triggers and the hardware recorder is stopped.
Based on this goal, current design is different from the discussions in
original RFC [1]:
 1) Static call is used when supported, to save function pointer
    dereference;
 2) intel_pmu_lbr_disable_all is used instead of perf_pmu_disable(),
    because the latter uses about 10 entries before stopping LBR.

With current code, on Intel CPU, LBR is stopped after 10 branch entries
after fexit triggers:

ID: 0 from intel_pmu_lbr_disable_all+58 to intel_pmu_lbr_disable_all+93
ID: 1 from intel_pmu_lbr_disable_all+54 to intel_pmu_lbr_disable_all+58
ID: 2 from intel_pmu_snapshot_branch_stack+88 to intel_pmu_lbr_disable_all+0
ID: 3 from bpf_get_branch_snapshot+77 to intel_pmu_snapshot_branch_stack+0
ID: 4 from __brk_limit+478052814 to bpf_get_branch_snapshot+0
ID: 5 from __brk_limit+478036039 to __brk_limit+478052760
ID: 6 from __bpf_prog_enter+34 to __brk_limit+478036027
ID: 7 from migrate_disable+60 to __bpf_prog_enter+9
ID: 8 from __bpf_prog_enter+4 to migrate_disable+0
ID: 9 from __brk_limit+478036022 to __bpf_prog_enter+0
ID: 10 from bpf_testmod_loop_test+22 to __brk_limit+478036003
ID: 11 from bpf_testmod_loop_test+20 to bpf_testmod_loop_test+13
ID: 12 from bpf_testmod_loop_test+20 to bpf_testmod_loop_test+13
ID: 13 from bpf_testmod_loop_test+20 to bpf_testmod_loop_test+13
...

[1] https://lore.kernel.org/bpf/20210818012937.2522409-1-songliubraving@fb.com/

Song Liu (3):
  perf: enable branch record for software events
  bpf: introduce helper bpf_get_branch_snapshot
  selftests/bpf: add test for bpf_get_branch_snapshot

 arch/x86/events/intel/core.c                  |  26 ++++-
 arch/x86/events/intel/ds.c                    |   8 --
 arch/x86/events/perf_event.h                  |  10 +-
 include/linux/perf_event.h                    |  23 ++++
 include/uapi/linux/bpf.h                      |  22 ++++
 kernel/bpf/trampoline.c                       |   3 +-
 kernel/events/core.c                          |   2 +
 kernel/trace/bpf_trace.c                      |  33 ++++++
 tools/include/uapi/linux/bpf.h                |  22 ++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  19 +++-
 .../selftests/bpf/prog_tests/core_reloc.c     |  14 +--
 .../bpf/prog_tests/get_branch_snapshot.c      | 100 ++++++++++++++++++
 .../selftests/bpf/prog_tests/module_attach.c  |  39 -------
 .../selftests/bpf/progs/get_branch_snapshot.c |  40 +++++++
 tools/testing/selftests/bpf/test_progs.c      |  39 +++++++
 tools/testing/selftests/bpf/test_progs.h      |   2 +
 tools/testing/selftests/bpf/trace_helpers.c   |  37 +++++++
 tools/testing/selftests/bpf/trace_helpers.h   |   5 +
 18 files changed, 378 insertions(+), 66 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/get_branch_snapshot.c
 create mode 100644 tools/testing/selftests/bpf/progs/get_branch_snapshot.c

--
2.30.2

[PATCH v5 bpf-next 1/3] perf: enable branch record for software events

[Song Liu]

The typical way to access branch record (e.g. Intel LBR) is via hardware
perf_event. For CPUs with FREEZE_LBRS_ON_PMI support, PMI could capture
reliable LBR. On the other hand, LBR could also be useful in non-PMI
scenario. For example, in kretprobe or bpf fexit program, LBR could
provide a lot of information on what happened with the function. Add API
to use branch record for software use.

Note that, when the software event triggers, it is necessary to stop the
branch record hardware asap. Therefore, static_call is used to remove some
branch instructions in this process.

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 arch/x86/events/intel/core.c | 26 +++++++++++++++++++++++---
 arch/x86/events/intel/ds.c   |  8 --------
 arch/x86/events/perf_event.h | 10 ++++++++--
 include/linux/perf_event.h   | 23 +++++++++++++++++++++++
 kernel/events/core.c         |  2 ++
 5 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 7011e87be6d03..d3422915969b9 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2143,7 +2143,7 @@ static __initconst const u64 knl_hw_cache_extra_regs
  * However, there are some cases which may change PEBS status, e.g. PMI
  * throttle. The PEBS_ENABLE should be updated where the status changes.
  */
-static void __intel_pmu_disable_all(void)
+static __always_inline void __intel_pmu_disable_all(void)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 
@@ -2153,7 +2153,7 @@ static void __intel_pmu_disable_all(void)
 		intel_pmu_disable_bts();
 }
 
-static void intel_pmu_disable_all(void)
+static __always_inline void intel_pmu_disable_all(void)
 {
 	__intel_pmu_disable_all();
 	intel_pmu_pebs_disable_all();
@@ -2186,6 +2186,20 @@ static void intel_pmu_enable_all(int added)
 	__intel_pmu_enable_all(added, false);
 }
 
+static int
+intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
+{
+	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
+
+	intel_pmu_disable_all();
+	intel_pmu_lbr_read();
+	cnt = min_t(unsigned int, cnt, x86_pmu.lbr_nr);
+
+	memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
+	intel_pmu_enable_all(0);
+	return cnt;
+}
+
 /*
  * Workaround for:
  *   Intel Errata AAK100 (model 26)
@@ -6283,9 +6297,15 @@ __init int intel_pmu_init(void)
 			x86_pmu.lbr_nr = 0;
 	}
 
-	if (x86_pmu.lbr_nr)
+	if (x86_pmu.lbr_nr) {
 		pr_cont("%d-deep LBR, ", x86_pmu.lbr_nr);
 
+		/* only support branch_stack snapshot for perfmon >= v2 */
+		if (x86_pmu.disable_all == intel_pmu_disable_all)
+			static_call_update(perf_snapshot_branch_stack,
+					   intel_pmu_snapshot_branch_stack);
+	}
+
 	intel_pmu_check_extra_regs(x86_pmu.extra_regs);
 
 	/* Support full width counters using alternative MSR range */
diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index 8647713276a73..8a832986578a9 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -1296,14 +1296,6 @@ void intel_pmu_pebs_enable_all(void)
 		wrmsrl(MSR_IA32_PEBS_ENABLE, cpuc->pebs_enabled);
 }
 
-void intel_pmu_pebs_disable_all(void)
-{
-	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
-
-	if (cpuc->pebs_enabled)
-		wrmsrl(MSR_IA32_PEBS_ENABLE, 0);
-}
-
 static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index e3ac05c97b5e5..171abbb359fe5 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -1240,6 +1240,14 @@ static inline bool intel_pmu_has_bts(struct perf_event *event)
 	return intel_pmu_has_bts_period(event, hwc->sample_period);
 }
 
+static __always_inline void intel_pmu_pebs_disable_all(void)
+{
+	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
+
+	if (cpuc->pebs_enabled)
+		wrmsrl(MSR_IA32_PEBS_ENABLE, 0);
+}
+
 int intel_pmu_save_and_restart(struct perf_event *event);
 
 struct event_constraint *
@@ -1314,8 +1322,6 @@ void intel_pmu_pebs_disable(struct perf_event *event);
 
 void intel_pmu_pebs_enable_all(void);
 
-void intel_pmu_pebs_disable_all(void);
-
 void intel_pmu_pebs_sched_task(struct perf_event_context *ctx, bool sched_in);
 
 void intel_pmu_auto_reload_read(struct perf_event *event);
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index fe156a8170aa3..4fe11f4f896b1 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -57,6 +57,7 @@ struct perf_guest_info_callbacks {
 #include <linux/cgroup.h>
 #include <linux/refcount.h>
 #include <linux/security.h>
+#include <linux/static_call.h>
 #include <asm/local.h>
 
 struct perf_callchain_entry {
@@ -1612,4 +1613,26 @@ extern void __weak arch_perf_update_userpage(struct perf_event *event,
 extern __weak u64 arch_perf_get_page_size(struct mm_struct *mm, unsigned long addr);
 #endif
 
+/*
+ * Snapshot branch stack on software events.
+ *
+ * Branch stack can be very useful in understanding software events. For
+ * example, when a long function, e.g. sys_perf_event_open, returns an
+ * errno, it is not obvious why the function failed. Branch stack could
+ * provide very helpful information in this type of scenarios.
+ *
+ * On software event, it is necessary to stop the hardware branch recorder
+ * fast. Otherwise, the hardware register/buffer will be flushed with
+ * entries af the triggering event. Therefore, static call is used to
+ * stop the hardware recorder.
+ */
+
+/*
+ * cnt is the number of entries allocated for entries.
+ * Return number of entries copied to .
+ */
+typedef int (perf_snapshot_branch_stack_t)(struct perf_branch_entry *entries,
+					   unsigned int cnt);
+DECLARE_STATIC_CALL(perf_snapshot_branch_stack, perf_snapshot_branch_stack_t);
+
 #endif /* _LINUX_PERF_EVENT_H */
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 011cc5069b7ba..d32a3cf37eb90 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -13437,3 +13437,5 @@ struct cgroup_subsys perf_event_cgrp_subsys = {
 	.threaded	= true,
 };
 #endif /* CONFIG_CGROUP_PERF */
+
+DEFINE_STATIC_CALL_RET0(perf_snapshot_branch_stack, perf_snapshot_branch_stack_t);
-- 
2.30.2

[PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Song Liu]

Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
branch trace from hardware (e.g. Intel LBR). To use the feature, the
user need to create perf_event with proper branch_record filtering
on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 include/uapi/linux/bpf.h       | 22 ++++++++++++++++++++++
 kernel/bpf/trampoline.c        |  3 ++-
 kernel/trace/bpf_trace.c       | 33 +++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
 4 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 791f31dd0abee..c986e6fad5bc0 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4877,6 +4877,27 @@ union bpf_attr {
  *		Get the struct pt_regs associated with **task**.
  *	Return
  *		A pointer to struct pt_regs.
+ *
+ * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
+ *	Description
+ *		Get branch trace from hardware engines like Intel LBR. The
+ *		branch trace is taken soon after the trigger point of the
+ *		BPF program, so it may contain some entries after the
+ *		trigger point. The user need to filter these entries
+ *		accordingly.
+ *
+ *		The data is stored as struct perf_branch_entry into output
+ *		buffer *entries*. *size* is the size of *entries* in bytes.
+ *		*flags* is reserved for now and must be zero.
+ *
+ *	Return
+ *		On success, number of bytes written to *buf*. On error, a
+ *		negative value.
+ *
+ *		**-EINVAL** if arguments invalid or **size** not a multiple
+ *		of **sizeof**\ (**struct perf_branch_entry**\ ).
+ *
+ *		**-ENOENT** if architecture does not support branch records.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5055,6 +5076,7 @@ union bpf_attr {
 	FN(get_func_ip),		\
 	FN(get_attach_cookie),		\
 	FN(task_pt_regs),		\
+	FN(get_branch_snapshot),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index fe1e857324e66..39eaaff81953d 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -10,6 +10,7 @@
 #include <linux/rcupdate_trace.h>
 #include <linux/rcupdate_wait.h>
 #include <linux/module.h>
+#include <linux/static_call.h>
 
 /* dummy _ops. The verifier will operate on target program's ops. */
 const struct bpf_verifier_ops bpf_extension_verifier_ops = {
@@ -526,7 +527,7 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
 }
 
 #define NO_START_TIME 1
-static u64 notrace bpf_prog_start_time(void)
+static __always_inline u64 notrace bpf_prog_start_time(void)
 {
 	u64 start = NO_START_TIME;
 
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 8e2eb950aa829..1954bc113087c 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1017,6 +1017,37 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto_pe = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
+{
+#ifndef CONFIG_X86
+	return -ENOENT;
+#else
+	static const u32 br_entry_size = sizeof(struct perf_branch_entry);
+	u32 entry_cnt = size / br_entry_size;
+
+	if (unlikely(flags))
+		return -EINVAL;
+
+	if (!buf || (size % br_entry_size != 0))
+		return -EINVAL;
+
+	entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
+
+	if (!entry_cnt)
+		return -ENOENT;
+
+	return entry_cnt * br_entry_size;
+#endif
+}
+
+static const struct bpf_func_proto bpf_get_branch_snapshot_proto = {
+	.func		= bpf_get_branch_snapshot,
+	.gpl_only	= true,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_UNINIT_MEM,
+	.arg2_type	= ARG_CONST_SIZE_OR_ZERO,
+};
+
 static const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -1132,6 +1163,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_snprintf_proto;
 	case BPF_FUNC_get_func_ip:
 		return &bpf_get_func_ip_proto_tracing;
+	case BPF_FUNC_get_branch_snapshot:
+		return &bpf_get_branch_snapshot_proto;
 	default:
 		return bpf_base_func_proto(func_id);
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 791f31dd0abee..c986e6fad5bc0 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4877,6 +4877,27 @@ union bpf_attr {
  *		Get the struct pt_regs associated with **task**.
  *	Return
  *		A pointer to struct pt_regs.
+ *
+ * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
+ *	Description
+ *		Get branch trace from hardware engines like Intel LBR. The
+ *		branch trace is taken soon after the trigger point of the
+ *		BPF program, so it may contain some entries after the
+ *		trigger point. The user need to filter these entries
+ *		accordingly.
+ *
+ *		The data is stored as struct perf_branch_entry into output
+ *		buffer *entries*. *size* is the size of *entries* in bytes.
+ *		*flags* is reserved for now and must be zero.
+ *
+ *	Return
+ *		On success, number of bytes written to *buf*. On error, a
+ *		negative value.
+ *
+ *		**-EINVAL** if arguments invalid or **size** not a multiple
+ *		of **sizeof**\ (**struct perf_branch_entry**\ ).
+ *
+ *		**-ENOENT** if architecture does not support branch records.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5055,6 +5076,7 @@ union bpf_attr {
 	FN(get_func_ip),		\
 	FN(get_attach_cookie),		\
 	FN(task_pt_regs),		\
+	FN(get_branch_snapshot),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
-- 
2.30.2

[PATCH v5 bpf-next 3/3] selftests/bpf: add test for bpf_get_branch_snapshot

[Song Liu]

This test uses bpf_get_branch_snapshot from a fexit program. The test uses
a target function (bpf_testmod_loop_test) and compares the record against
kallsyms. If there isn't enough record matching kallsyms, the test fails.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Song Liu <songliubraving@fb.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  19 +++-
 .../selftests/bpf/prog_tests/core_reloc.c     |  14 +--
 .../bpf/prog_tests/get_branch_snapshot.c      | 100 ++++++++++++++++++
 .../selftests/bpf/prog_tests/module_attach.c  |  39 -------
 .../selftests/bpf/progs/get_branch_snapshot.c |  40 +++++++
 tools/testing/selftests/bpf/test_progs.c      |  39 +++++++
 tools/testing/selftests/bpf/test_progs.h      |   2 +
 tools/testing/selftests/bpf/trace_helpers.c   |  37 +++++++
 tools/testing/selftests/bpf/trace_helpers.h   |   5 +
 9 files changed, 243 insertions(+), 52 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/get_branch_snapshot.c
 create mode 100644 tools/testing/selftests/bpf/progs/get_branch_snapshot.c

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 141d8da687d21..50fc5561110a4 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -13,6 +13,18 @@
 
 DEFINE_PER_CPU(int, bpf_testmod_ksym_percpu) = 123;
 
+noinline int bpf_testmod_loop_test(int n)
+{
+	int i, sum = 0;
+
+	/* the primary goal of this test is to test LBR. Create a lot of
+	 * branches in the function, so we can catch it easily.
+	 */
+	for (i = 0; i < n; i++)
+		sum += i;
+	return sum;
+}
+
 noinline ssize_t
 bpf_testmod_test_read(struct file *file, struct kobject *kobj,
 		      struct bin_attribute *bin_attr,
@@ -24,7 +36,11 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
 		.len = len,
 	};
 
-	trace_bpf_testmod_test_read(current, &ctx);
+	/* This is always true. Use the check to make sure the compiler
+	 * doesn't remove bpf_testmod_loop_test.
+	 */
+	if (bpf_testmod_loop_test(101) > 100)
+		trace_bpf_testmod_test_read(current, &ctx);
 
 	return -EIO; /* always fail */
 }
@@ -71,4 +87,3 @@ module_exit(bpf_testmod_exit);
 MODULE_AUTHOR("Andrii Nakryiko");
 MODULE_DESCRIPTION("BPF selftests module");
 MODULE_LICENSE("Dual BSD/GPL");
-
diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
index 4739b15b2a979..15d355af8d1d2 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
@@ -30,7 +30,7 @@ static int duration = 0;
 	.output_len = sizeof(struct core_reloc_module_output),		\
 	.prog_sec_name = sec_name,					\
 	.raw_tp_name = tp_name,						\
-	.trigger = trigger_module_test_read,				\
+	.trigger = __trigger_module_test_read,				\
 	.needs_testmod = true,						\
 }
 
@@ -475,19 +475,11 @@ static int setup_type_id_case_failure(struct core_reloc_test_case *test)
 	return 0;
 }
 
-static int trigger_module_test_read(const struct core_reloc_test_case *test)
+static int __trigger_module_test_read(const struct core_reloc_test_case *test)
 {
 	struct core_reloc_module_output *exp = (void *)test->output;
-	int fd, err;
-
-	fd = open("/sys/kernel/bpf_testmod", O_RDONLY);
-	err = -errno;
-	if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err))
-		return err;
-
-	read(fd, NULL, exp->len); /* request expected number of bytes */
-	close(fd);
 
+	trigger_module_test_read(exp->len);
 	return 0;
 }
 
diff --git a/tools/testing/selftests/bpf/prog_tests/get_branch_snapshot.c b/tools/testing/selftests/bpf/prog_tests/get_branch_snapshot.c
new file mode 100644
index 0000000000000..26af9b3d572e3
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/get_branch_snapshot.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+#include <test_progs.h>
+#include "get_branch_snapshot.skel.h"
+
+static int *pfd_array;
+static int cpu_cnt;
+
+static int create_perf_events(void)
+{
+	struct perf_event_attr attr = {0};
+	int cpu;
+
+	/* create perf event */
+	attr.size = sizeof(attr);
+	attr.type = PERF_TYPE_RAW;
+	attr.config = 0x1b00;
+	attr.sample_type = PERF_SAMPLE_BRANCH_STACK;
+	attr.branch_sample_type = PERF_SAMPLE_BRANCH_KERNEL |
+		PERF_SAMPLE_BRANCH_USER | PERF_SAMPLE_BRANCH_ANY;
+
+	cpu_cnt = libbpf_num_possible_cpus();
+	pfd_array = malloc(sizeof(int) * cpu_cnt);
+	if (!pfd_array) {
+		cpu_cnt = 0;
+		return 1;
+	}
+
+	for (cpu = 0; cpu < cpu_cnt; cpu++) {
+		pfd_array[cpu] = syscall(__NR_perf_event_open, &attr,
+					 -1, cpu, -1, PERF_FLAG_FD_CLOEXEC);
+		if (pfd_array[cpu] < 0)
+			break;
+	}
+
+	return cpu == 0;
+}
+
+static void close_perf_events(void)
+{
+	int cpu = 0;
+	int fd;
+
+	while (cpu++ < cpu_cnt) {
+		fd = pfd_array[cpu];
+		if (fd < 0)
+			break;
+		close(fd);
+	}
+	free(pfd_array);
+}
+
+void test_get_branch_snapshot(void)
+{
+	struct get_branch_snapshot *skel = NULL;
+	int err;
+
+	if (create_perf_events()) {
+		test__skip();  /* system doesn't support LBR */
+		goto cleanup;
+	}
+
+	skel = get_branch_snapshot__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "get_branch_snapshot__open_and_load"))
+		goto cleanup;
+
+	err = kallsyms_find("bpf_testmod_loop_test", &skel->bss->address_low);
+	if (!ASSERT_OK(err, "kallsyms_find"))
+		goto cleanup;
+
+	err = kallsyms_find_next("bpf_testmod_loop_test", &skel->bss->address_high);
+	if (!ASSERT_OK(err, "kallsyms_find_next"))
+		goto cleanup;
+
+	err = get_branch_snapshot__attach(skel);
+	if (!ASSERT_OK(err, "get_branch_snapshot__attach"))
+		goto cleanup;
+
+	trigger_module_test_read(100);
+
+	if (skel->bss->total_entries < 16) {
+		/* too few entries for the hit/waste test */
+		test__skip();
+		goto cleanup;
+	}
+
+	ASSERT_GT(skel->bss->test1_hits, 1, "find_looptest_in_lbr");
+
+	/* Given we stop LBR in software, we will waste a few entries.
+	 * But we should try to waste as few as possible entries. We are at
+	 * about 11 on x86_64 systems.
+	 * Add a check for < 15 so that we get heads-up when something
+	 * changes and wastes too many entries.
+	 */
+	ASSERT_LT(skel->bss->wasted_entries, 15, "check_wasted_entries");
+
+cleanup:
+	get_branch_snapshot__destroy(skel);
+	close_perf_events();
+}
diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c
index d85a69b7ce449..1797a6e4d6d84 100644
--- a/tools/testing/selftests/bpf/prog_tests/module_attach.c
+++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c
@@ -6,45 +6,6 @@
 
 static int duration;
 
-static int trigger_module_test_read(int read_sz)
-{
-	int fd, err;
-
-	fd = open("/sys/kernel/bpf_testmod", O_RDONLY);
-	err = -errno;
-	if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err))
-		return err;
-
-	read(fd, NULL, read_sz);
-	close(fd);
-
-	return 0;
-}
-
-static int trigger_module_test_write(int write_sz)
-{
-	int fd, err;
-	char *buf = malloc(write_sz);
-
-	if (!buf)
-		return -ENOMEM;
-
-	memset(buf, 'a', write_sz);
-	buf[write_sz-1] = '\0';
-
-	fd = open("/sys/kernel/bpf_testmod", O_WRONLY);
-	err = -errno;
-	if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err)) {
-		free(buf);
-		return err;
-	}
-
-	write(fd, buf, write_sz);
-	close(fd);
-	free(buf);
-	return 0;
-}
-
 static int delete_module(const char *name, int flags)
 {
 	return syscall(__NR_delete_module, name, flags);
diff --git a/tools/testing/selftests/bpf/progs/get_branch_snapshot.c b/tools/testing/selftests/bpf/progs/get_branch_snapshot.c
new file mode 100644
index 0000000000000..a1b139888048c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/get_branch_snapshot.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+__u64 test1_hits = 0;
+__u64 address_low = 0;
+__u64 address_high = 0;
+int wasted_entries = 0;
+long total_entries = 0;
+
+#define ENTRY_CNT 32
+struct perf_branch_entry entries[ENTRY_CNT] = {};
+
+static inline bool in_range(__u64 val)
+{
+	return (val >= address_low) && (val < address_high);
+}
+
+SEC("fexit/bpf_testmod_loop_test")
+int BPF_PROG(test1, int n, int ret)
+{
+	long i;
+
+	total_entries = bpf_get_branch_snapshot(entries, sizeof(entries), 0);
+	total_entries /= sizeof(struct perf_branch_entry);
+
+	for (i = 0; i < ENTRY_CNT; i++) {
+		if (i >= total_entries)
+			break;
+		if (in_range(entries[i].from) && in_range(entries[i].to))
+			test1_hits++;
+		else if (!test1_hits)
+			wasted_entries++;
+	}
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index cc1cd240445d2..2ed01f615d20f 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -743,6 +743,45 @@ int cd_flavor_subdir(const char *exec_name)
 	return chdir(flavor);
 }
 
+int trigger_module_test_read(int read_sz)
+{
+	int fd, err;
+
+	fd = open("/sys/kernel/bpf_testmod", O_RDONLY);
+	err = -errno;
+	if (!ASSERT_GE(fd, 0, "testmod_file_open"))
+		return err;
+
+	read(fd, NULL, read_sz);
+	close(fd);
+
+	return 0;
+}
+
+int trigger_module_test_write(int write_sz)
+{
+	int fd, err;
+	char *buf = malloc(write_sz);
+
+	if (!buf)
+		return -ENOMEM;
+
+	memset(buf, 'a', write_sz);
+	buf[write_sz-1] = '\0';
+
+	fd = open("/sys/kernel/bpf_testmod", O_WRONLY);
+	err = -errno;
+	if (!ASSERT_GE(fd, 0, "testmod_file_open")) {
+		free(buf);
+		return err;
+	}
+
+	write(fd, buf, write_sz);
+	close(fd);
+	free(buf);
+	return 0;
+}
+
 #define MAX_BACKTRACE_SZ 128
 void crash_handler(int signum)
 {
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index c8c2bf878f67c..94bef0aa74cf5 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -291,6 +291,8 @@ int compare_map_keys(int map1_fd, int map2_fd);
 int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len);
 int extract_build_id(char *build_id, size_t size);
 int kern_sync_rcu(void);
+int trigger_module_test_read(int read_sz);
+int trigger_module_test_write(int write_sz);
 
 #ifdef __x86_64__
 #define SYS_NANOSLEEP_KPROBE_NAME "__x64_sys_nanosleep"
diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
index e7a19b04d4eaf..5100a169b72b1 100644
--- a/tools/testing/selftests/bpf/trace_helpers.c
+++ b/tools/testing/selftests/bpf/trace_helpers.c
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
+#include <ctype.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -117,6 +118,42 @@ int kallsyms_find(const char *sym, unsigned long long *addr)
 	return err;
 }
 
+/* find the address of the next symbol of the same type, this can be used
+ * to determine the end of a function.
+ */
+int kallsyms_find_next(const char *sym, unsigned long long *addr)
+{
+	char type, found_type, name[500];
+	unsigned long long value;
+	bool found = false;
+	int err = 0;
+	FILE *f;
+
+	f = fopen("/proc/kallsyms", "r");
+	if (!f)
+		return -EINVAL;
+
+	while (fscanf(f, "%llx %c %499s%*[^\n]\n", &value, &type, name) > 0) {
+		/* Different types of symbols in kernel modules are mixed
+		 * in /proc/kallsyms. Only return the next matching type.
+		 * Use tolower() for type so that 'T' matches 't'.
+		 */
+		if (found && found_type == tolower(type)) {
+			*addr = value;
+			goto out;
+		}
+		if (strcmp(name, sym) == 0) {
+			found = true;
+			found_type = tolower(type);
+		}
+	}
+	err = -ENOENT;
+
+out:
+	fclose(f);
+	return err;
+}
+
 void read_trace_pipe(void)
 {
 	int trace_fd;
diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h
index d907b445524d5..bc8ed86105d94 100644
--- a/tools/testing/selftests/bpf/trace_helpers.h
+++ b/tools/testing/selftests/bpf/trace_helpers.h
@@ -16,6 +16,11 @@ long ksym_get_addr(const char *name);
 /* open kallsyms and find addresses on the fly, faster than load + search. */
 int kallsyms_find(const char *sym, unsigned long long *addr);
 
+/* find the address of the next symbol, this can be used to determine the
+ * end of a function
+ */
+int kallsyms_find_next(const char *sym, unsigned long long *addr);
+
 void read_trace_pipe(void);
 
 ssize_t get_uprobe_offset(const void *addr, ssize_t base);
-- 
2.30.2

RE: [PATCH v5 bpf-next 1/3] perf: enable branch record for software events

[John Fastabend]

Song Liu wrote:
> The typical way to access branch record (e.g. Intel LBR) is via hardware
> perf_event. For CPUs with FREEZE_LBRS_ON_PMI support, PMI could capture
> reliable LBR. On the other hand, LBR could also be useful in non-PMI
> scenario. For example, in kretprobe or bpf fexit program, LBR could
> provide a lot of information on what happened with the function. Add API
> to use branch record for software use.
> 
> Note that, when the software event triggers, it is necessary to stop the
> branch record hardware asap. Therefore, static_call is used to remove some
> branch instructions in this process.
> 
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---

[...]

>  void intel_pmu_auto_reload_read(struct perf_event *event);
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index fe156a8170aa3..4fe11f4f896b1 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -57,6 +57,7 @@ struct perf_guest_info_callbacks {
>  #include <linux/cgroup.h>
>  #include <linux/refcount.h>
>  #include <linux/security.h>
> +#include <linux/static_call.h>
>  #include <asm/local.h>
>  
>  struct perf_callchain_entry {
> @@ -1612,4 +1613,26 @@ extern void __weak arch_perf_update_userpage(struct perf_event *event,
>  extern __weak u64 arch_perf_get_page_size(struct mm_struct *mm, unsigned long addr);
>  #endif
>  
> +/*
> + * Snapshot branch stack on software events.
> + *
> + * Branch stack can be very useful in understanding software events. For
> + * example, when a long function, e.g. sys_perf_event_open, returns an
> + * errno, it is not obvious why the function failed. Branch stack could
> + * provide very helpful information in this type of scenarios.
> + *
> + * On software event, it is necessary to stop the hardware branch recorder
> + * fast. Otherwise, the hardware register/buffer will be flushed with
> + * entries af the triggering event. Therefore, static call is used to
              ^^
nit, af->of

> + * stop the hardware recorder.
> + */
> +
> +/*
> + * cnt is the number of entries allocated for entries.
> + * Return number of entries copied to .
> + */

A bit out of scope, but LGTM.

Acked-by: John Fastabend <john.fastabend@gmail.com>

RE: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[John Fastabend]

Song Liu wrote:
> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
> branch trace from hardware (e.g. Intel LBR). To use the feature, the
> user need to create perf_event with proper branch_record filtering
> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
> 
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---

[...]

>  
> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
> +{
> +#ifndef CONFIG_X86
> +	return -ENOENT;
> +#else
> +	static const u32 br_entry_size = sizeof(struct perf_branch_entry);
> +	u32 entry_cnt = size / br_entry_size;
> +
> +	if (unlikely(flags))
> +		return -EINVAL;
> +
> +	if (!buf || (size % br_entry_size != 0))
> +		return -EINVAL;

LGTM, but why fail if buffer is slightly larger than expected? I guess its a slightly
buggy program that would do this, but not actually harmful right?

Acked-by: John Fastabend <john.fastabend@gmail.com>

RE: [PATCH v5 bpf-next 3/3] selftests/bpf: add test for bpf_get_branch_snapshot

[John Fastabend]

Song Liu wrote:
> This test uses bpf_get_branch_snapshot from a fexit program. The test uses
> a target function (bpf_testmod_loop_test) and compares the record against
> kallsyms. If there isn't enough record matching kallsyms, the test fails.
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  19 +++-
>  .../selftests/bpf/prog_tests/core_reloc.c     |  14 +--
>  .../bpf/prog_tests/get_branch_snapshot.c      | 100 ++++++++++++++++++
>  .../selftests/bpf/prog_tests/module_attach.c  |  39 -------
>  .../selftests/bpf/progs/get_branch_snapshot.c |  40 +++++++
>  tools/testing/selftests/bpf/test_progs.c      |  39 +++++++
>  tools/testing/selftests/bpf/test_progs.h      |   2 +
>  tools/testing/selftests/bpf/trace_helpers.c   |  37 +++++++
>  tools/testing/selftests/bpf/trace_helpers.h   |   5 +
>  9 files changed, 243 insertions(+), 52 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/get_branch_snapshot.c
>  create mode 100644 tools/testing/selftests/bpf/progs/get_branch_snapshot.c
> 

[...]

> diff --git a/tools/testing/selftests/bpf/progs/get_branch_snapshot.c b/tools/testing/selftests/bpf/progs/get_branch_snapshot.c
> new file mode 100644
> index 0000000000000..a1b139888048c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/get_branch_snapshot.c
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +__u64 test1_hits = 0;
> +__u64 address_low = 0;
> +__u64 address_high = 0;
> +int wasted_entries = 0;
> +long total_entries = 0;
> +
> +#define ENTRY_CNT 32
> +struct perf_branch_entry entries[ENTRY_CNT] = {};

It looks like perf_branch_entry has never changed, but it could grow?
Then size check in helper would fail. I'm not sure its worth it, but
this could be done with CO-RE so the size is correct even if the
struct grows.

> +
> +static inline bool in_range(__u64 val)
> +{
> +	return (val >= address_low) && (val < address_high);
> +}
> +
> +SEC("fexit/bpf_testmod_loop_test")
> +int BPF_PROG(test1, int n, int ret)
> +{
> +	long i;
> +
> +	total_entries = bpf_get_branch_snapshot(entries, sizeof(entries), 0);
> +	total_entries /= sizeof(struct perf_branch_entry);
> +
> +	for (i = 0; i < ENTRY_CNT; i++) {
> +		if (i >= total_entries)
> +			break;
> +		if (in_range(entries[i].from) && in_range(entries[i].to))
> +			test1_hits++;
> +		else if (!test1_hits)
> +			wasted_entries++;
> +	}
> +	return 0;
> +}

Other than small comment LGTM.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Song Liu]

> On Sep 2, 2021, at 1:56 PM, John Fastabend <john.fastabend@gmail.com> wrote:
> 
> Song Liu wrote:
>> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
>> branch trace from hardware (e.g. Intel LBR). To use the feature, the
>> user need to create perf_event with proper branch_record filtering
>> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
>> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
>> 
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
> 
> [...]
> 
>> 
>> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
>> +{
>> +#ifndef CONFIG_X86
>> +	return -ENOENT;
>> +#else
>> +	static const u32 br_entry_size = sizeof(struct perf_branch_entry);
>> +	u32 entry_cnt = size / br_entry_size;
>> +
>> +	if (unlikely(flags))
>> +		return -EINVAL;
>> +
>> +	if (!buf || (size % br_entry_size != 0))
>> +		return -EINVAL;
> 
> LGTM, but why fail if buffer is slightly larger than expected? I guess its a slightly
> buggy program that would do this, but not actually harmful right?

This check was added because bpf_read_branch_records() has a similar check. 
I guess it is OK either way. 

> 
> Acked-by: John Fastabend <john.fastabend@gmail.com>

Thanks for the review!

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Andrii Nakryiko]

On Thu, Sep 2, 2021 at 9:58 AM Song Liu <songliubraving@fb.com> wrote:
>
> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
> branch trace from hardware (e.g. Intel LBR). To use the feature, the
> user need to create perf_event with proper branch_record filtering
> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  include/uapi/linux/bpf.h       | 22 ++++++++++++++++++++++
>  kernel/bpf/trampoline.c        |  3 ++-
>  kernel/trace/bpf_trace.c       | 33 +++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
>  4 files changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 791f31dd0abee..c986e6fad5bc0 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -4877,6 +4877,27 @@ union bpf_attr {
>   *             Get the struct pt_regs associated with **task**.
>   *     Return
>   *             A pointer to struct pt_regs.
> + *
> + * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
> + *     Description
> + *             Get branch trace from hardware engines like Intel LBR. The
> + *             branch trace is taken soon after the trigger point of the
> + *             BPF program, so it may contain some entries after the

This part is a leftover from previous design, so not relevant anymore?

> + *             trigger point. The user need to filter these entries
> + *             accordingly.
> + *
> + *             The data is stored as struct perf_branch_entry into output
> + *             buffer *entries*. *size* is the size of *entries* in bytes.
> + *             *flags* is reserved for now and must be zero.
> + *
> + *     Return
> + *             On success, number of bytes written to *buf*. On error, a
> + *             negative value.
> + *
> + *             **-EINVAL** if arguments invalid or **size** not a multiple
> + *             of **sizeof**\ (**struct perf_branch_entry**\ ).
> + *
> + *             **-ENOENT** if architecture does not support branch records.

[...]

Re: [PATCH v5 bpf-next 0/3] bpf: introduce bpf_get_branch_snapshot

[Andrii Nakryiko]

On Thu, Sep 2, 2021 at 9:58 AM Song Liu <songliubraving@fb.com> wrote:
>
> Changes v4 => v5
> 1. Modify perf_snapshot_branch_stack_t to save some memcpy. (Andrii)
> 2. Minor fixes in selftests. (Andrii)
>
> Changes v3 => v4:
> 1. Do not reshuffle intel_pmu_disable_all(). Use some inline to save LBR
>    entries. (Peter)
> 2. Move static_call(perf_snapshot_branch_stack) to the helper. (Alexei)
> 3. Add argument flags to bpf_get_branch_snapshot. (Andrii)
> 4. Make MAX_BRANCH_SNAPSHOT an enum (Andrii). And rename it as
>    PERF_MAX_BRANCH_SNAPSHOT
> 5. Make bpf_get_branch_snapshot similar to bpf_read_branch_records.
>    (Andrii)
> 6. Move the test target function to bpf_testmod. Updated kallsyms_find_next
>    to work properly with modules. (Andrii)
>
> Changes v2 => v3:
> 1. Fix the use of static_call. (Peter)
> 2. Limit the use to perfmon version >= 2. (Peter)
> 3. Modify intel_pmu_snapshot_branch_stack() to use intel_pmu_disable_all
>    and intel_pmu_enable_all().
>
> Changes v1 => v2:
> 1. Rename the helper as bpf_get_branch_snapshot;
> 2. Fix/simplify the use of static_call;
> 3. Instead of percpu variables, let intel_pmu_snapshot_branch_stack output
>    branch records to an output argument of type perf_branch_snapshot.
>
> Branch stack can be very useful in understanding software events. For
> example, when a long function, e.g. sys_perf_event_open, returns an errno,
> it is not obvious why the function failed. Branch stack could provide very
> helpful information in this type of scenarios.
>
> This set adds support to read branch stack with a new BPF helper
> bpf_get_branch_trace(). Currently, this is only supported in Intel systems.
> It is also possible to support the same feaure for PowerPC.
>
> The hardware that records the branch stace is not stopped automatically on
> software events. Therefore, it is necessary to stop it in software soon.
> Otherwise, the hardware buffers/registers will be flushed. One of the key
> design consideration in this set is to minimize the number of branch record
> entries between the event triggers and the hardware recorder is stopped.
> Based on this goal, current design is different from the discussions in
> original RFC [1]:
>  1) Static call is used when supported, to save function pointer
>     dereference;
>  2) intel_pmu_lbr_disable_all is used instead of perf_pmu_disable(),
>     because the latter uses about 10 entries before stopping LBR.
>
> With current code, on Intel CPU, LBR is stopped after 10 branch entries
> after fexit triggers:
>
> ID: 0 from intel_pmu_lbr_disable_all+58 to intel_pmu_lbr_disable_all+93
> ID: 1 from intel_pmu_lbr_disable_all+54 to intel_pmu_lbr_disable_all+58
> ID: 2 from intel_pmu_snapshot_branch_stack+88 to intel_pmu_lbr_disable_all+0
> ID: 3 from bpf_get_branch_snapshot+77 to intel_pmu_snapshot_branch_stack+0
> ID: 4 from __brk_limit+478052814 to bpf_get_branch_snapshot+0
> ID: 5 from __brk_limit+478036039 to __brk_limit+478052760
> ID: 6 from __bpf_prog_enter+34 to __brk_limit+478036027
> ID: 7 from migrate_disable+60 to __bpf_prog_enter+9
> ID: 8 from __bpf_prog_enter+4 to migrate_disable+0
> ID: 9 from __brk_limit+478036022 to __bpf_prog_enter+0
> ID: 10 from bpf_testmod_loop_test+22 to __brk_limit+478036003
> ID: 11 from bpf_testmod_loop_test+20 to bpf_testmod_loop_test+13
> ID: 12 from bpf_testmod_loop_test+20 to bpf_testmod_loop_test+13
> ID: 13 from bpf_testmod_loop_test+20 to bpf_testmod_loop_test+13
> ...
>
> [1] https://lore.kernel.org/bpf/20210818012937.2522409-1-songliubraving@fb.com/
>
> Song Liu (3):
>   perf: enable branch record for software events
>   bpf: introduce helper bpf_get_branch_snapshot
>   selftests/bpf: add test for bpf_get_branch_snapshot
>

Besides the BPF helper comment nit, looks good to me. For the series:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  arch/x86/events/intel/core.c                  |  26 ++++-
>  arch/x86/events/intel/ds.c                    |   8 --
>  arch/x86/events/perf_event.h                  |  10 +-
>  include/linux/perf_event.h                    |  23 ++++
>  include/uapi/linux/bpf.h                      |  22 ++++
>  kernel/bpf/trampoline.c                       |   3 +-
>  kernel/events/core.c                          |   2 +
>  kernel/trace/bpf_trace.c                      |  33 ++++++
>  tools/include/uapi/linux/bpf.h                |  22 ++++
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  19 +++-
>  .../selftests/bpf/prog_tests/core_reloc.c     |  14 +--
>  .../bpf/prog_tests/get_branch_snapshot.c      | 100 ++++++++++++++++++
>  .../selftests/bpf/prog_tests/module_attach.c  |  39 -------
>  .../selftests/bpf/progs/get_branch_snapshot.c |  40 +++++++
>  tools/testing/selftests/bpf/test_progs.c      |  39 +++++++
>  tools/testing/selftests/bpf/test_progs.h      |   2 +
>  tools/testing/selftests/bpf/trace_helpers.c   |  37 +++++++
>  tools/testing/selftests/bpf/trace_helpers.h   |   5 +
>  18 files changed, 378 insertions(+), 66 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/get_branch_snapshot.c
>  create mode 100644 tools/testing/selftests/bpf/progs/get_branch_snapshot.c
>
> --
> 2.30.2

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Song Liu]

> On Sep 2, 2021, at 3:53 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Thu, Sep 2, 2021 at 9:58 AM Song Liu <songliubraving@fb.com> wrote:
>> 
>> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
>> branch trace from hardware (e.g. Intel LBR). To use the feature, the
>> user need to create perf_event with proper branch_record filtering
>> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
>> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
>> 
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>> include/uapi/linux/bpf.h       | 22 ++++++++++++++++++++++
>> kernel/bpf/trampoline.c        |  3 ++-
>> kernel/trace/bpf_trace.c       | 33 +++++++++++++++++++++++++++++++++
>> tools/include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
>> 4 files changed, 79 insertions(+), 1 deletion(-)
>> 
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 791f31dd0abee..c986e6fad5bc0 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -4877,6 +4877,27 @@ union bpf_attr {
>>  *             Get the struct pt_regs associated with **task**.
>>  *     Return
>>  *             A pointer to struct pt_regs.
>> + *
>> + * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
>> + *     Description
>> + *             Get branch trace from hardware engines like Intel LBR. The
>> + *             branch trace is taken soon after the trigger point of the
>> + *             BPF program, so it may contain some entries after the
> 
> This part is a leftover from previous design, so not relevant anymore?

Hmm.. This is still relevant, but not very accurate. I guess we should 
provide more information, like "For more information about branches before
the trigger point, this should be called early in the BPF program". 

Song


> 
>> + *             trigger point. The user need to filter these entries
>> + *             accordingly.
>> + *
>> + *             The data is stored as struct perf_branch_entry into output
>> + *             buffer *entries*. *size* is the size of *entries* in bytes.
>> + *             *flags* is reserved for now and must be zero.
>> + *
>> + *     Return
>> + *             On success, number of bytes written to *buf*. On error, a
>> + *             negative value.
>> + *
>> + *             **-EINVAL** if arguments invalid or **size** not a multiple
>> + *             of **sizeof**\ (**struct perf_branch_entry**\ ).
>> + *
>> + *             **-ENOENT** if architecture does not support branch records.
> 
> [...]

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Andrii Nakryiko]

On Thu, Sep 2, 2021 at 4:03 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Sep 2, 2021, at 3:53 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Sep 2, 2021 at 9:58 AM Song Liu <songliubraving@fb.com> wrote:
> >>
> >> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
> >> branch trace from hardware (e.g. Intel LBR). To use the feature, the
> >> user need to create perf_event with proper branch_record filtering
> >> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
> >> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
> >>
> >> Signed-off-by: Song Liu <songliubraving@fb.com>
> >> ---
> >> include/uapi/linux/bpf.h       | 22 ++++++++++++++++++++++
> >> kernel/bpf/trampoline.c        |  3 ++-
> >> kernel/trace/bpf_trace.c       | 33 +++++++++++++++++++++++++++++++++
> >> tools/include/uapi/linux/bpf.h | 22 ++++++++++++++++++++++
> >> 4 files changed, 79 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >> index 791f31dd0abee..c986e6fad5bc0 100644
> >> --- a/include/uapi/linux/bpf.h
> >> +++ b/include/uapi/linux/bpf.h
> >> @@ -4877,6 +4877,27 @@ union bpf_attr {
> >>  *             Get the struct pt_regs associated with **task**.
> >>  *     Return
> >>  *             A pointer to struct pt_regs.
> >> + *
> >> + * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
> >> + *     Description
> >> + *             Get branch trace from hardware engines like Intel LBR. The
> >> + *             branch trace is taken soon after the trigger point of the
> >> + *             BPF program, so it may contain some entries after the
> >
> > This part is a leftover from previous design, so not relevant anymore?
>
> Hmm.. This is still relevant, but not very accurate. I guess we should
> provide more information, like "For more information about branches before
> the trigger point, this should be called early in the BPF program".

I read the part about "taken soon after the trigger point of BPF
program" as a reference to previous implementation. So maybe let's
clarify that because LBR is not frozen, from the time
bpf_get_branch_snapshot() is called to when we actually capture LBRs
we can waste few records due to internal kernel calls, so the user has
to be aware of that.

>
> Song
>
>
> >
> >> + *             trigger point. The user need to filter these entries
> >> + *             accordingly.
> >> + *
> >> + *             The data is stored as struct perf_branch_entry into output
> >> + *             buffer *entries*. *size* is the size of *entries* in bytes.
> >> + *             *flags* is reserved for now and must be zero.
> >> + *
> >> + *     Return
> >> + *             On success, number of bytes written to *buf*. On error, a
> >> + *             negative value.
> >> + *
> >> + *             **-EINVAL** if arguments invalid or **size** not a multiple
> >> + *             of **sizeof**\ (**struct perf_branch_entry**\ ).
> >> + *
> >> + *             **-ENOENT** if architecture does not support branch records.
> >
> > [...]
>

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Alexei Starovoitov]

On Thu, Sep 2, 2021 at 9:58 AM Song Liu <songliubraving@fb.com> wrote:
>
> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
> +{
> +#ifndef CONFIG_X86
> +       return -ENOENT;
> +#else
> +       static const u32 br_entry_size = sizeof(struct perf_branch_entry);
> +       u32 entry_cnt = size / br_entry_size;
> +
> +       if (unlikely(flags))
> +               return -EINVAL;
> +
> +       if (!buf || (size % br_entry_size != 0))
> +               return -EINVAL;
> +
> +       entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);

Not taken branches will not be counted even with PERF_SAMPLE_BRANCH_ANY,
right?
Probably the first unlikely(flags) will be a fallthrough in asm.
Maybe worth adding unlikely to 2nd condition as well to make
sure that the compiler will generate default fallthrough code for it ?
So both will not appear in lbr entries?
Or maybe do:
if (unlikely(!buf))
   return -EINVAL;
entry_cnt = static_call
if (size % br_entry_size)
   return -EINVAL;

The lbr trace will be collected anyway.
If there are jmps in lbr due to earlier "if"s we can move them
after static_call. Like if (unlikely(flags) can be done afterwards too.

Bigger bang for the buck would be static-inline-ing migrate_disable, though.

Re: [PATCH v5 bpf-next 1/3] perf: enable branch record for software events

[Peter Zijlstra]

On Thu, Sep 02, 2021 at 09:57:04AM -0700, Song Liu wrote:
> +static int
> +intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
> +{
> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +
> +	intel_pmu_disable_all();
> +	intel_pmu_lbr_read();
> +	cnt = min_t(unsigned int, cnt, x86_pmu.lbr_nr);
> +
> +	memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
> +	intel_pmu_enable_all(0);
> +	return cnt;
> +}

Would something like the below help get rid of that memcpy() ?

(compile tested only)

---
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 7011e87be6d0..c508ba6099d2 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2938,7 +2938,7 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
 
 	loops = 0;
 again:
-	intel_pmu_lbr_read();
+	intel_pmu_lbr_read(&cpuc->lbr_stack);
 	intel_pmu_ack_status(status);
 	if (++loops > 100) {
 		static bool warned;
diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
index 9e6d6eaeb4cb..0a5bacba86c2 100644
--- a/arch/x86/events/intel/lbr.c
+++ b/arch/x86/events/intel/lbr.c
@@ -170,7 +170,7 @@ enum {
 
 #define ARCH_LBR_CTL_MASK			0x7f000e
 
-static void intel_pmu_lbr_filter(struct cpu_hw_events *cpuc);
+static void intel_pmu_lbr_filter(struct cpu_hw_events *cpuc, struct perf_branch_stack *bs);
 
 static __always_inline bool is_lbr_call_stack_bit_set(u64 config)
 {
@@ -783,7 +783,7 @@ void intel_pmu_lbr_disable_all(void)
 		__intel_pmu_lbr_disable();
 }
 
-void intel_pmu_lbr_read_32(struct cpu_hw_events *cpuc)
+void intel_pmu_lbr_read_32(struct cpu_hw_events *cpuc, struct perf_branch_stack *bs)
 {
 	unsigned long mask = x86_pmu.lbr_nr - 1;
 	u64 tos = intel_pmu_lbr_tos();
@@ -801,18 +801,18 @@ void intel_pmu_lbr_read_32(struct cpu_hw_events *cpuc)
 
 		rdmsrl(x86_pmu.lbr_from + lbr_idx, msr_lastbranch.lbr);
 
-		cpuc->lbr_entries[i].from	= msr_lastbranch.from;
-		cpuc->lbr_entries[i].to		= msr_lastbranch.to;
-		cpuc->lbr_entries[i].mispred	= 0;
-		cpuc->lbr_entries[i].predicted	= 0;
-		cpuc->lbr_entries[i].in_tx	= 0;
-		cpuc->lbr_entries[i].abort	= 0;
-		cpuc->lbr_entries[i].cycles	= 0;
-		cpuc->lbr_entries[i].type	= 0;
-		cpuc->lbr_entries[i].reserved	= 0;
+		bs->entries[i].from	= msr_lastbranch.from;
+		bs->entries[i].to	= msr_lastbranch.to;
+		bs->entries[i].mispred	= 0;
+		bs->entries[i].predicted= 0;
+		bs->entries[i].in_tx	= 0;
+		bs->entries[i].abort	= 0;
+		bs->entries[i].cycles	= 0;
+		bs->entries[i].type	= 0;
+		bs->entries[i].reserved	= 0;
 	}
-	cpuc->lbr_stack.nr = i;
-	cpuc->lbr_stack.hw_idx = tos;
+	bs->nr = i;
+	bs->hw_idx = tos;
 }
 
 /*
@@ -820,7 +820,7 @@ void intel_pmu_lbr_read_32(struct cpu_hw_events *cpuc)
  * is the same as the linear address, allowing us to merge the LIP and EIP
  * LBR formats.
  */
-void intel_pmu_lbr_read_64(struct cpu_hw_events *cpuc)
+void intel_pmu_lbr_read_64(struct cpu_hw_events *cpuc, struct perf_branch_stack *bs)
 {
 	bool need_info = false, call_stack = false;
 	unsigned long mask = x86_pmu.lbr_nr - 1;
@@ -896,19 +896,19 @@ void intel_pmu_lbr_read_64(struct cpu_hw_events *cpuc)
 		if (abort && x86_pmu.lbr_double_abort && out > 0)
 			out--;
 
-		cpuc->lbr_entries[out].from	 = from;
-		cpuc->lbr_entries[out].to	 = to;
-		cpuc->lbr_entries[out].mispred	 = mis;
-		cpuc->lbr_entries[out].predicted = pred;
-		cpuc->lbr_entries[out].in_tx	 = in_tx;
-		cpuc->lbr_entries[out].abort	 = abort;
-		cpuc->lbr_entries[out].cycles	 = cycles;
-		cpuc->lbr_entries[out].type	 = 0;
-		cpuc->lbr_entries[out].reserved	 = 0;
+		bs->entries[out].from	 = from;
+		bs->entries[out].to	 = to;
+		bs->entries[out].mispred = mis;
+		bs->entries[out].predicted = pred;
+		bs->entries[out].in_tx	 = in_tx;
+		bs->entries[out].abort	 = abort;
+		bs->entries[out].cycles	 = cycles;
+		bs->entries[out].type	 = 0;
+		bs->entries[out].reserved = 0;
 		out++;
 	}
-	cpuc->lbr_stack.nr = out;
-	cpuc->lbr_stack.hw_idx = tos;
+	bs->nr = out;
+	bs->hw_idx = tos;
 }
 
 static __always_inline int get_lbr_br_type(u64 info)
@@ -945,7 +945,8 @@ static __always_inline u16 get_lbr_cycles(u64 info)
 }
 
 static void intel_pmu_store_lbr(struct cpu_hw_events *cpuc,
-				struct lbr_entry *entries)
+				struct lbr_entry *entries,
+				struct perf_branch_stack *bs)
 {
 	struct perf_branch_entry *e;
 	struct lbr_entry *lbr;
@@ -954,7 +955,7 @@ static void intel_pmu_store_lbr(struct cpu_hw_events *cpuc,
 
 	for (i = 0; i < x86_pmu.lbr_nr; i++) {
 		lbr = entries ? &entries[i] : NULL;
-		e = &cpuc->lbr_entries[i];
+		e = &bs->entries[i];
 
 		from = rdlbr_from(i, lbr);
 		/*
@@ -977,28 +978,28 @@ static void intel_pmu_store_lbr(struct cpu_hw_events *cpuc,
 		e->reserved	= 0;
 	}
 
-	cpuc->lbr_stack.nr = i;
+	bs->nr = i;
 }
 
-static void intel_pmu_arch_lbr_read(struct cpu_hw_events *cpuc)
+static void intel_pmu_arch_lbr_read(struct cpu_hw_events *cpuc, struct perf_branch_stack *bs)
 {
-	intel_pmu_store_lbr(cpuc, NULL);
+	intel_pmu_store_lbr(cpuc, NULL, bs);
 }
 
-static void intel_pmu_arch_lbr_read_xsave(struct cpu_hw_events *cpuc)
+static void intel_pmu_arch_lbr_read_xsave(struct cpu_hw_events *cpuc, struct perf_branch_stack *bs)
 {
 	struct x86_perf_task_context_arch_lbr_xsave *xsave = cpuc->lbr_xsave;
 
 	if (!xsave) {
-		intel_pmu_store_lbr(cpuc, NULL);
+		intel_pmu_store_lbr(cpuc, NULL, bs);
 		return;
 	}
 	xsaves(&xsave->xsave, XFEATURE_MASK_LBR);
 
-	intel_pmu_store_lbr(cpuc, xsave->lbr.entries);
+	intel_pmu_store_lbr(cpuc, xsave->lbr.entries, bs);
 }
 
-void intel_pmu_lbr_read(void)
+void intel_pmu_lbr_read(struct perf_branch_stack *bs)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 
@@ -1012,9 +1013,8 @@ void intel_pmu_lbr_read(void)
 	    cpuc->lbr_users == cpuc->lbr_pebs_users)
 		return;
 
-	x86_pmu.lbr_read(cpuc);
-
-	intel_pmu_lbr_filter(cpuc);
+	x86_pmu.lbr_read(cpuc, bs);
+	intel_pmu_lbr_filter(cpuc, bs);
 }
 
 /*
@@ -1402,7 +1402,7 @@ static const int arch_lbr_br_type_map[ARCH_LBR_BR_TYPE_MAP_MAX] = {
  * in PERF_SAMPLE_BRANCH_STACK sample may vary.
  */
 static void
-intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)
+intel_pmu_lbr_filter(struct cpu_hw_events *cpuc, struct perf_branch_stack *bs)
 {
 	u64 from, to;
 	int br_sel = cpuc->br_sel;
@@ -1414,11 +1414,11 @@ intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)
 	    ((br_sel & X86_BR_TYPE_SAVE) != X86_BR_TYPE_SAVE))
 		return;
 
-	for (i = 0; i < cpuc->lbr_stack.nr; i++) {
+	for (i = 0; i < bs->nr; i++) {
 
-		from = cpuc->lbr_entries[i].from;
-		to = cpuc->lbr_entries[i].to;
-		type = cpuc->lbr_entries[i].type;
+		from = bs->entries[i].from;
+		to = bs->entries[i].to;
+		type = bs->entries[i].type;
 
 		/*
 		 * Parse the branch type recorded in LBR_x_INFO MSR.
@@ -1430,9 +1430,9 @@ intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)
 			to_plm = kernel_ip(to) ? X86_BR_KERNEL : X86_BR_USER;
 			type = arch_lbr_br_type_map[type] | to_plm;
 		} else
-			type = branch_type(from, to, cpuc->lbr_entries[i].abort);
+			type = branch_type(from, to, bs->entries[i].abort);
 		if (type != X86_BR_NONE && (br_sel & X86_BR_ANYTX)) {
-			if (cpuc->lbr_entries[i].in_tx)
+			if (bs->entries[i].in_tx)
 				type |= X86_BR_IN_TX;
 			else
 				type |= X86_BR_NO_TX;
@@ -1440,25 +1440,25 @@ intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)
 
 		/* if type does not correspond, then discard */
 		if (type == X86_BR_NONE || (br_sel & type) != type) {
-			cpuc->lbr_entries[i].from = 0;
+			bs->entries[i].from = 0;
 			compress = true;
 		}
 
 		if ((br_sel & X86_BR_TYPE_SAVE) == X86_BR_TYPE_SAVE)
-			cpuc->lbr_entries[i].type = common_branch_type(type);
+			bs->entries[i].type = common_branch_type(type);
 	}
 
 	if (!compress)
 		return;
 
 	/* remove all entries with from=0 */
-	for (i = 0; i < cpuc->lbr_stack.nr; ) {
-		if (!cpuc->lbr_entries[i].from) {
+	for (i = 0; i < bs->nr; ) {
+		if (!bs->entries[i].from) {
 			j = i;
-			while (++j < cpuc->lbr_stack.nr)
-				cpuc->lbr_entries[j-1] = cpuc->lbr_entries[j];
-			cpuc->lbr_stack.nr--;
-			if (!cpuc->lbr_entries[i].from)
+			while (++j < bs->nr)
+				bs->entries[j-1] = bs->entries[j];
+			bs->nr--;
+			if (!bs->entries[i].from)
 				continue;
 		}
 		i++;
@@ -1476,8 +1476,8 @@ void intel_pmu_store_pebs_lbrs(struct lbr_entry *lbr)
 	else
 		cpuc->lbr_stack.hw_idx = intel_pmu_lbr_tos();
 
-	intel_pmu_store_lbr(cpuc, lbr);
-	intel_pmu_lbr_filter(cpuc);
+	intel_pmu_store_lbr(cpuc, lbr, &cpuc->lbr_stack);
+	intel_pmu_lbr_filter(cpuc, &cpuc->lbr_stack);
 }
 
 /*
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index e3ac05c97b5e..9700fb1f47c3 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -852,7 +852,7 @@ struct x86_pmu {
 	unsigned int	lbr_br_type:1;
 
 	void		(*lbr_reset)(void);
-	void		(*lbr_read)(struct cpu_hw_events *cpuc);
+	void		(*lbr_read)(struct cpu_hw_events *cpuc, struct perf_branch_stack *bs);
 	void		(*lbr_save)(void *ctx);
 	void		(*lbr_restore)(void *ctx);
 
@@ -1345,11 +1345,11 @@ void intel_pmu_lbr_enable_all(bool pmi);
 
 void intel_pmu_lbr_disable_all(void);
 
-void intel_pmu_lbr_read(void);
+void intel_pmu_lbr_read(struct perf_branch_stack *bs);
 
-void intel_pmu_lbr_read_32(struct cpu_hw_events *cpuc);
+void intel_pmu_lbr_read_32(struct cpu_hw_events *cpuc, struct perf_branch_stack *bs);
 
-void intel_pmu_lbr_read_64(struct cpu_hw_events *cpuc);
+void intel_pmu_lbr_read_64(struct cpu_hw_events *cpuc, struct perf_branch_stack *bs);
 
 void intel_pmu_lbr_save(void *ctx);
 

Re: [PATCH v5 bpf-next 1/3] perf: enable branch record for software events

[Peter Zijlstra]

On Thu, Sep 02, 2021 at 09:57:04AM -0700, Song Liu wrote:

> +static int
> +intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
> +{
> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +
> +	intel_pmu_disable_all();
> +	intel_pmu_lbr_read();
> +	cnt = min_t(unsigned int, cnt, x86_pmu.lbr_nr);
> +
> +	memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
> +	intel_pmu_enable_all(0);
> +	return cnt;
> +}

Given this disables the PMI from 'random' contexts, should we not add
IRQ disabling around this to avoid really bad behaviour?

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Peter Zijlstra]

On Thu, Sep 02, 2021 at 09:57:05AM -0700, Song Liu wrote:
> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
> +{
> +#ifndef CONFIG_X86
> +	return -ENOENT;
> +#else
> +	static const u32 br_entry_size = sizeof(struct perf_branch_entry);
> +	u32 entry_cnt = size / br_entry_size;
> +
> +	if (unlikely(flags))
> +		return -EINVAL;
> +
> +	if (!buf || (size % br_entry_size != 0))
> +		return -EINVAL;
> +
> +	entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
> +
> +	if (!entry_cnt)
> +		return -ENOENT;
> +
> +	return entry_cnt * br_entry_size;
> +#endif
> +}

Do we really need that CONFIG_X86 thing? Seems rather bad practise.

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Peter Zijlstra]

On Thu, Sep 02, 2021 at 09:57:05AM -0700, Song Liu wrote:
> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
> +{
> +	static const u32 br_entry_size = sizeof(struct perf_branch_entry);
> +	u32 entry_cnt = size / br_entry_size;
> +
> +	if (unlikely(flags))
> +		return -EINVAL;
> +
> +	if (!buf || (size % br_entry_size != 0))
> +		return -EINVAL;
> +
> +	entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);

That's at least 2, possibly 3 branches just from the sanity checks, plus
at least one from starting the BPF prog and one from calling this
function, gets you at ~5 branch entries gone before you even do the
snapshot thing.

Less if you're in branch-stack mode.

Can't the validator help with getting rid of the some of that?

I suppose you have to have this helper function because the JIT cannot
emit static_call()... although in this case one could cheat and simply
emit a call to static_call_query() and not bother with dynamic updates
(because there aren't any).

Re: [PATCH v5 bpf-next 1/3] perf: enable branch record for software events

[Song Liu]

Hi Peter,

> On Sep 3, 2021, at 1:27 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Thu, Sep 02, 2021 at 09:57:04AM -0700, Song Liu wrote:
> 
>> +static int
>> +intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
>> +{
>> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>> +
>> +	intel_pmu_disable_all();
>> +	intel_pmu_lbr_read();
>> +	cnt = min_t(unsigned int, cnt, x86_pmu.lbr_nr);
>> +
>> +	memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
>> +	intel_pmu_enable_all(0);
>> +	return cnt;
>> +}
> 
> Given this disables the PMI from 'random' contexts, should we not add
> IRQ disabling around this to avoid really bad behaviour?

Do you mean we should add (instead of not add) IRQ disable?

Thanks,
Song

Re: [PATCH v5 bpf-next 1/3] perf: enable branch record for software events

[Song Liu]

> On Sep 3, 2021, at 1:02 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Thu, Sep 02, 2021 at 09:57:04AM -0700, Song Liu wrote:
>> +static int
>> +intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
>> +{
>> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>> +
>> +	intel_pmu_disable_all();
>> +	intel_pmu_lbr_read();
>> +	cnt = min_t(unsigned int, cnt, x86_pmu.lbr_nr);
>> +
>> +	memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
>> +	intel_pmu_enable_all(0);
>> +	return cnt;
>> +}
> 
> Would something like the below help get rid of that memcpy() ?
> 
> (compile tested only)

We can get rid of the memcpy. But we will need an extra "size" or "num_entries" 
parameter for intel_pmu_lbr_read. I can add this change in the next version. 

Thanks,
Song

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Song Liu]

> On Sep 3, 2021, at 1:28 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Thu, Sep 02, 2021 at 09:57:05AM -0700, Song Liu wrote:
>> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
>> +{
>> +#ifndef CONFIG_X86
>> +	return -ENOENT;
>> +#else
>> +	static const u32 br_entry_size = sizeof(struct perf_branch_entry);
>> +	u32 entry_cnt = size / br_entry_size;
>> +
>> +	if (unlikely(flags))
>> +		return -EINVAL;
>> +
>> +	if (!buf || (size % br_entry_size != 0))
>> +		return -EINVAL;
>> +
>> +	entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
>> +
>> +	if (!entry_cnt)
>> +		return -ENOENT;
>> +
>> +	return entry_cnt * br_entry_size;
>> +#endif
>> +}
> 
> Do we really need that CONFIG_X86 thing? Seems rather bad practise.

The ifndef will save a few cycles on architectures that do not support
branch stack. I personally don't have very strong preference on either
way. 

Thanks,
Song

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Song Liu]

> On Sep 3, 2021, at 1:47 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Thu, Sep 02, 2021 at 09:57:05AM -0700, Song Liu wrote:
>> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
>> +{
>> +	static const u32 br_entry_size = sizeof(struct perf_branch_entry);
>> +	u32 entry_cnt = size / br_entry_size;
>> +
>> +	if (unlikely(flags))
>> +		return -EINVAL;
>> +
>> +	if (!buf || (size % br_entry_size != 0))
>> +		return -EINVAL;
>> +
>> +	entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
> 
> That's at least 2, possibly 3 branches just from the sanity checks, plus
> at least one from starting the BPF prog and one from calling this
> function, gets you at ~5 branch entries gone before you even do the
> snapshot thing.

Let me try to shuffle the function and get rid of some of these checks. 

> 
> Less if you're in branch-stack mode.
> 
> Can't the validator help with getting rid of the some of that?
> 
> I suppose you have to have this helper function because the JIT cannot
> emit static_call()... although in this case one could cheat and simply
> emit a call to static_call_query() and not bother with dynamic updates
> (because there aren't any).

We only JIT some key helper functions. I didn't think about that because 
current version is OK for mainstream and future hardware. I guess we 
can try JIT if it turns out some architecture needs more optimization. 

Thanks,
Song

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Andrii Nakryiko]

On Fri, Sep 3, 2021 at 1:49 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Sep 02, 2021 at 09:57:05AM -0700, Song Liu wrote:
> > +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
> > +{
> > +     static const u32 br_entry_size = sizeof(struct perf_branch_entry);
> > +     u32 entry_cnt = size / br_entry_size;
> > +
> > +     if (unlikely(flags))
> > +             return -EINVAL;
> > +
> > +     if (!buf || (size % br_entry_size != 0))

I think buf can't be NULL, this should be enforced already by verifier
due to ARG_PTR_TO_UNINIT_MEM, so we can drop that.

> > +             return -EINVAL;
> > +
> > +     entry_cnt = static_call(perf_snapshot_branch_stack)(buf, entry_cnt);
>
> That's at least 2, possibly 3 branches just from the sanity checks, plus
> at least one from starting the BPF prog and one from calling this
> function, gets you at ~5 branch entries gone before you even do the
> snapshot thing.
>
> Less if you're in branch-stack mode.
>
> Can't the validator help with getting rid of the some of that?
>

Good points. I think we can drop !buf check completely. The flags and
size checks can be moved after perf_snapshot_branch_stack call. In
common cases those checks should always pass, but even if they don't
we'll just capture the LBR anyways, but will return an error later,
which seems totally acceptable.

As Alexei mentioned, there is a whole non-inlined migrate_disable()
call before this, which we should inline as well. It's good for
performance, not just LBR.

> I suppose you have to have this helper function because the JIT cannot
> emit static_call()... although in this case one could cheat and simply
> emit a call to static_call_query() and not bother with dynamic updates
> (because there aren't any).

If that's safe, let's do it.

Re: [PATCH v5 bpf-next 1/3] perf: enable branch record for software events

[Peter Zijlstra]

On Fri, Sep 03, 2021 at 04:45:29PM +0000, Song Liu wrote:
> Hi Peter,
> 
> > On Sep 3, 2021, at 1:27 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > On Thu, Sep 02, 2021 at 09:57:04AM -0700, Song Liu wrote:
> > 
> >> +static int
> >> +intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
> >> +{
> >> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> >> +
> >> +	intel_pmu_disable_all();
> >> +	intel_pmu_lbr_read();
> >> +	cnt = min_t(unsigned int, cnt, x86_pmu.lbr_nr);
> >> +
> >> +	memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
> >> +	intel_pmu_enable_all(0);
> >> +	return cnt;
> >> +}
> > 
> > Given this disables the PMI from 'random' contexts, should we not add
> > IRQ disabling around this to avoid really bad behaviour?
> 
> Do you mean we should add (instead of not add) IRQ disable?

Yeah, I tihnk we want local_irq_save()/restore() here.

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Peter Zijlstra]

On Fri, Sep 03, 2021 at 10:10:16AM -0700, Andrii Nakryiko wrote:
> > I suppose you have to have this helper function because the JIT cannot
> > emit static_call()... although in this case one could cheat and simply
> > emit a call to static_call_query() and not bother with dynamic updates
> > (because there aren't any).
> 
> If that's safe, let's do it.

I'll try and remember to look into static_call_lock(), a means of
forever denying future static_call_update() calls. That should make this
more obvious.

Re: [PATCH v5 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Peter Zijlstra]

On Sat, Sep 04, 2021 at 12:24:30PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 03, 2021 at 10:10:16AM -0700, Andrii Nakryiko wrote:
> > > I suppose you have to have this helper function because the JIT cannot
> > > emit static_call()... although in this case one could cheat and simply
> > > emit a call to static_call_query() and not bother with dynamic updates
> > > (because there aren't any).
> > 
> > If that's safe, let's do it.
> 
> I'll try and remember to look into static_call_lock(), a means of
> forever denying future static_call_update() calls. That should make this
> more obvious.

A little something like so I suppose.... we don't really have spare
bits in the !INLINE case :/


---
diff --git a/include/linux/static_call.h b/include/linux/static_call.h
index 3e56a9751c06..b0feccd56d37 100644
--- a/include/linux/static_call.h
+++ b/include/linux/static_call.h
@@ -174,6 +174,10 @@ struct static_call_tramp_key {
 	s32 key;
 };
 
+extern void __static_call_lock(struct static_call_key *key);
+
+#define static_call_lock(name) __static_call_lock(&STATIC_CALL_KEY(name))
+
 extern void __static_call_update(struct static_call_key *key, void *tramp, void *func);
 extern int static_call_mod_init(struct module *mod);
 extern int static_call_text_reserved(void *start, void *end);
@@ -215,6 +219,8 @@ extern long __static_call_return0(void);
 
 #elif defined(CONFIG_HAVE_STATIC_CALL)
 
+#define static_call_lock(name)
+
 static inline int static_call_init(void) { return 0; }
 
 #define __DEFINE_STATIC_CALL(name, _func, _func_init)			\
@@ -268,6 +274,8 @@ static inline long __static_call_return0(void)
 
 #else /* Generic implementation */
 
+#define static_call_lock(name)
+
 static inline int static_call_init(void) { return 0; }
 
 static inline long __static_call_return0(void)
diff --git a/include/linux/static_call_types.h b/include/linux/static_call_types.h
index 5a00b8b2cf9f..e40a3b595c4a 100644
--- a/include/linux/static_call_types.h
+++ b/include/linux/static_call_types.h
@@ -62,6 +62,7 @@ struct static_call_key {
 	void *func;
 	union {
 		/* bit 0: 0 = mods, 1 = sites */
+		/* but 1: locked */
 		unsigned long type;
 		struct static_call_mod *mods;
 		struct static_call_site *sites;
diff --git a/kernel/static_call.c b/kernel/static_call.c
index 43ba0b1e0edb..a1ba93fbad29 100644
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -104,6 +104,11 @@ static inline bool static_call_key_has_mods(struct static_call_key *key)
 	return !(key->type & 1);
 }
 
+static inline bool static_call_key_is_locked(struct static_call_key *key)
+{
+	return !!(key->type & 2);
+}
+
 static inline struct static_call_mod *static_call_key_next(struct static_call_key *key)
 {
 	if (!static_call_key_has_mods(key))
@@ -117,7 +122,7 @@ static inline struct static_call_site *static_call_key_sites(struct static_call_
 	if (static_call_key_has_mods(key))
 		return NULL;
 
-	return (struct static_call_site *)(key->type & ~1);
+	return (struct static_call_site *)(key->type & ~3);
 }
 
 void __static_call_update(struct static_call_key *key, void *tramp, void *func)
@@ -125,6 +130,9 @@ void __static_call_update(struct static_call_key *key, void *tramp, void *func)
 	struct static_call_site *site, *stop;
 	struct static_call_mod *site_mod, first;
 
+	if (WARN_ON_ONCE(static_call_key_is_locked(key)))
+		return;
+
 	cpus_read_lock();
 	static_call_lock();
 
@@ -418,6 +426,18 @@ static void static_call_del_module(struct module *mod)
 	}
 }
 
+void __static_call_lock(struct static_call_key *key)
+{
+	cpus_read_lock();
+	static_call_lock();
+
+	WARN_ON_ONCE(static_call_key_is_locked(key));
+	key->type |= 2;
+
+	static_call_unlock();
+	cpus_read_unlock();
+}
+
 static int static_call_module_notify(struct notifier_block *nb,
 				     unsigned long val, void *data)
 {

Re: [PATCH v5 bpf-next 1/3] perf: enable branch record for software events

[Song Liu]

> On Sep 3, 2021, at 9:50 AM, Song Liu <songliubraving@fb.com> wrote:
> 
> 
> 
>> On Sep 3, 2021, at 1:02 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> 
>> On Thu, Sep 02, 2021 at 09:57:04AM -0700, Song Liu wrote:
>>> +static int
>>> +intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
>>> +{
>>> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>> +
>>> +	intel_pmu_disable_all();
>>> +	intel_pmu_lbr_read();
>>> +	cnt = min_t(unsigned int, cnt, x86_pmu.lbr_nr);
>>> +
>>> +	memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
>>> +	intel_pmu_enable_all(0);
>>> +	return cnt;
>>> +}
>> 
>> Would something like the below help get rid of that memcpy() ?
>> 
>> (compile tested only)
> 
> We can get rid of the memcpy. But we will need an extra "size" or "num_entries" 
> parameter for intel_pmu_lbr_read. I can add this change in the next version. 
> 

This is trickier than I thought. As current lbr_read() function works with 
perf_branch_stack, while the BPF helper side uses array of perf_branch_entry. 
And the array is passed into the helper by the BPF program. Therefore, to 
really get rid of the memcpy, we need to refactor the lbr driver code more. 
How about we keep the memcpy for now, and add the optimization later (if we 
think it is necessary)?

Thanks,
Song

Re: [PATCH v5 bpf-next 1/3] perf: enable branch record for software events

[Andrii Nakryiko]

On Tue, Sep 7, 2021 at 12:02 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Sep 3, 2021, at 9:50 AM, Song Liu <songliubraving@fb.com> wrote:
> >
> >
> >
> >> On Sep 3, 2021, at 1:02 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >>
> >> On Thu, Sep 02, 2021 at 09:57:04AM -0700, Song Liu wrote:
> >>> +static int
> >>> +intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
> >>> +{
> >>> +   struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> >>> +
> >>> +   intel_pmu_disable_all();
> >>> +   intel_pmu_lbr_read();
> >>> +   cnt = min_t(unsigned int, cnt, x86_pmu.lbr_nr);
> >>> +
> >>> +   memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
> >>> +   intel_pmu_enable_all(0);
> >>> +   return cnt;
> >>> +}
> >>
> >> Would something like the below help get rid of that memcpy() ?
> >>
> >> (compile tested only)
> >
> > We can get rid of the memcpy. But we will need an extra "size" or "num_entries"
> > parameter for intel_pmu_lbr_read. I can add this change in the next version.
> >
>
> This is trickier than I thought. As current lbr_read() function works with
> perf_branch_stack, while the BPF helper side uses array of perf_branch_entry.
> And the array is passed into the helper by the BPF program. Therefore, to
> really get rid of the memcpy, we need to refactor the lbr driver code more.
> How about we keep the memcpy for now, and add the optimization later (if we
> think it is necessary)?
>

Sounds good to me!

> Thanks,
> Song
>