perf/hw_breakpoint: Optimize for thousands of tasks

Add KUnit test for hw_breakpoint constraints accounting, with various interesting mixes of breakpoint targets (some care was taken to catch interesting corner cases via bug-injection).

The test cannot be built as a module because it requires access to hw_breakpoint_slots(), which is not inlinable or exported on all architectures.

Signed-off-by Marco Elver <[email protected]>

---
v3:
* Don't use raw_smp_processor_id().

v2:
* New patch.

---
 kernel/events/Makefile             |   1 +
 kernel/events/hw_breakpoint_test.c | 323 +++++++++++++++++++++++++++++
 lib/Kconfig.debug                  |  10 +
 3 files changed, 334 insertions(+)
 create mode 100644 kernel/events/hw_breakpoint_test.c

kernel/events/Makefile +1 -0

index 8591c180b52b..91a62f566743 100644
@@ -2,4 +2,5 @@
 obj-y := core.o ring_buffer.o callchain.o
 
 obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
+obj-$(CONFIG_HW_BREAKPOINT_KUNIT_TEST) += hw_breakpoint_test.o
 obj-$(CONFIG_UPROBES) += uprobes.o

kernel/events/hw_breakpoint_test.c new file +323 -0

new file mode 100644
index 000000000000..433c5c45e2a5
@@ -0,0 +1,323 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for hw_breakpoint constraints accounting logic.
+ *
+ * Copyright (C) 2022, Google LLC.
+ */
+
+#include <kunit/test.h>
+#include <linux/cpumask.h>
+#include <linux/hw_breakpoint.h>
+#include <linux/kthread.h>
+#include <linux/perf_event.h>
+#include <asm/hw_breakpoint.h>
+
+#define TEST_REQUIRES_BP_SLOTS(test, slots)                                            \
+       do {                                                                            \
+               if ((slots) > get_test_bp_slots()) {                                    \
+                       kunit_skip((test), "Requires breakpoint slots: %d > %d", slots, \
+                                  get_test_bp_slots());                                \
+               }                                                                       \
+       } while (0)
+
+#define TEST_EXPECT_NOSPC(expr) KUNIT_EXPECT_EQ(test, -ENOSPC, PTR_ERR(expr))
+
+#define MAX_TEST_BREAKPOINTS 512
+
+static char break_vars[MAX_TEST_BREAKPOINTS];
+static struct perf_event *test_bps[MAX_TEST_BREAKPOINTS];
+static struct task_struct *__other_task;
+
+static struct perf_event *register_test_bp(int cpu, struct task_struct *tsk, int idx)
+{
+       struct perf_event_attr attr = {};
+
+       if (WARN_ON(idx < 0 || idx >= MAX_TEST_BREAKPOINTS))
+               return NULL;
+
+       hw_breakpoint_init(&attr);
+       attr.bp_addr = (unsigned long)&break_vars[idx];
+       attr.bp_len = HW_BREAKPOINT_LEN_1;
+       attr.bp_type = HW_BREAKPOINT_RW;
+       return perf_event_create_kernel_counter(&attr, cpu, tsk, NULL, NULL);
+}
+
+static void unregister_test_bp(struct perf_event **bp)
+{
+       if (WARN_ON(IS_ERR(*bp)))
+               return;
+       if (WARN_ON(!*bp))
+               return;
+       unregister_hw_breakpoint(*bp);
+       *bp = NULL;
+}
+
+static int get_test_bp_slots(void)
+{
+       static int slots;
+
+       if (!slots)
+               slots = hw_breakpoint_slots(TYPE_DATA);
+
+       return slots;
+}
+
+static void fill_one_bp_slot(struct kunit *test, int *id, int cpu, struct task_struct *tsk)
+{
+       struct perf_event *bp = register_test_bp(cpu, tsk, *id);
+
+       KUNIT_ASSERT_NOT_NULL(test, bp);
+       KUNIT_ASSERT_FALSE(test, IS_ERR(bp));
+       KUNIT_ASSERT_NULL(test, test_bps[*id]);
+       test_bps[(*id)++] = bp;
+}
+
+/*
+ * Fills up the given @cpu/@tsk with breakpoints, only leaving @skip slots free.
+ *
+ * Returns true if this can be called again, continuing at @id.
+ */
+static bool fill_bp_slots(struct kunit *test, int *id, int cpu, struct task_struct *tsk, int skip)
+{
+       for (int i = 0; i < get_test_bp_slots() - skip; ++i)
+               fill_one_bp_slot(test, id, cpu, tsk);
+
+       return *id + get_test_bp_slots() <= MAX_TEST_BREAKPOINTS;
+}
+
+static int dummy_kthread(void *arg)
+{
+       return 0;
+}
+
+static struct task_struct *get_other_task(struct kunit *test)
+{
+       struct task_struct *tsk;
+
+       if (__other_task)
+               return __other_task;
+
+       tsk = kthread_create(dummy_kthread, NULL, "hw_breakpoint_dummy_task");
+       KUNIT_ASSERT_FALSE(test, IS_ERR(tsk));
+       __other_task = tsk;
+       return __other_task;
+}
+
+static int get_test_cpu(int num)
+{
+       int cpu;
+
+       WARN_ON(num < 0);
+
+       for_each_online_cpu(cpu) {
+               if (num-- <= 0)
+                       break;
+       }
+
+       return cpu;
+}
+
+/* ===== Test cases ===== */
+
+static void test_one_cpu(struct kunit *test)
+{
+       int idx = 0;
+
+       fill_bp_slots(test, &idx, get_test_cpu(0), NULL, 0);
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+}
+
+static void test_many_cpus(struct kunit *test)
+{
+       int idx = 0;
+       int cpu;
+
+       /* Test that CPUs are independent. */
+       for_each_online_cpu(cpu) {
+               bool do_continue = fill_bp_slots(test, &idx, cpu, NULL, 0);
+
+               TEST_EXPECT_NOSPC(register_test_bp(cpu, NULL, idx));
+               if (!do_continue)
+                       break;
+       }
+}
+
+static void test_one_task_on_all_cpus(struct kunit *test)
+{
+       int idx = 0;
+
+       fill_bp_slots(test, &idx, -1, current, 0);
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       /* Remove one and adding back CPU-target should work. */
+       unregister_test_bp(&test_bps[0]);
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+}
+
+static void test_two_tasks_on_all_cpus(struct kunit *test)
+{
+       int idx = 0;
+
+       /* Test that tasks are independent. */
+       fill_bp_slots(test, &idx, -1, current, 0);
+       fill_bp_slots(test, &idx, -1, get_other_task(test), 0);
+
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(-1, get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       /* Remove one from first task and adding back CPU-target should not work. */
+       unregister_test_bp(&test_bps[0]);
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+}
+
+static void test_one_task_on_one_cpu(struct kunit *test)
+{
+       int idx = 0;
+
+       fill_bp_slots(test, &idx, get_test_cpu(0), current, 0);
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       /*
+        * Remove one and adding back CPU-target should work; this case is
+        * special vs. above because the task's constraints are CPU-dependent.
+        */
+       unregister_test_bp(&test_bps[0]);
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+}
+
+static void test_one_task_mixed(struct kunit *test)
+{
+       int idx = 0;
+
+       TEST_REQUIRES_BP_SLOTS(test, 3);
+
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), current);
+       fill_bp_slots(test, &idx, -1, current, 1);
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+
+       /* Transition from CPU-dependent pinned count to CPU-independent. */
+       unregister_test_bp(&test_bps[0]);
+       unregister_test_bp(&test_bps[1]);
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+}
+
+static void test_two_tasks_on_one_cpu(struct kunit *test)
+{
+       int idx = 0;
+
+       fill_bp_slots(test, &idx, get_test_cpu(0), current, 0);
+       fill_bp_slots(test, &idx, get_test_cpu(0), get_other_task(test), 0);
+
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(-1, get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       /* Can still create breakpoints on some other CPU. */
+       fill_bp_slots(test, &idx, get_test_cpu(1), NULL, 0);
+}
+
+static void test_two_tasks_on_one_all_cpus(struct kunit *test)
+{
+       int idx = 0;
+
+       fill_bp_slots(test, &idx, get_test_cpu(0), current, 0);
+       fill_bp_slots(test, &idx, -1, get_other_task(test), 0);
+
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(-1, get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       /* Cannot create breakpoints on some other CPU either. */
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(1), NULL, idx));
+}
+
+static void test_task_on_all_and_one_cpu(struct kunit *test)
+{
+       int tsk_on_cpu_idx, cpu_idx;
+       int idx = 0;
+
+       TEST_REQUIRES_BP_SLOTS(test, 3);
+
+       fill_bp_slots(test, &idx, -1, current, 2);
+       /* Transitioning from only all CPU breakpoints to mixed. */
+       tsk_on_cpu_idx = idx;
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), current);
+       fill_one_bp_slot(test, &idx, -1, current);
+
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+
+       /* We should still be able to use up another CPU's slots. */
+       cpu_idx = idx;
+       fill_one_bp_slot(test, &idx, get_test_cpu(1), NULL);
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(1), NULL, idx));
+
+       /* Transitioning back to task target on all CPUs. */
+       unregister_test_bp(&test_bps[tsk_on_cpu_idx]);
+       /* Still have a CPU target breakpoint in get_test_cpu(1). */
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       /* Remove it and try again. */
+       unregister_test_bp(&test_bps[cpu_idx]);
+       fill_one_bp_slot(test, &idx, -1, current);
+
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(1), NULL, idx));
+}
+
+static struct kunit_case hw_breakpoint_test_cases[] = {
+       KUNIT_CASE(test_one_cpu),
+       KUNIT_CASE(test_many_cpus),
+       KUNIT_CASE(test_one_task_on_all_cpus),
+       KUNIT_CASE(test_two_tasks_on_all_cpus),
+       KUNIT_CASE(test_one_task_on_one_cpu),
+       KUNIT_CASE(test_one_task_mixed),
+       KUNIT_CASE(test_two_tasks_on_one_cpu),
+       KUNIT_CASE(test_two_tasks_on_one_all_cpus),
+       KUNIT_CASE(test_task_on_all_and_one_cpu),
+       {},
+};
+
+static int test_init(struct kunit *test)
+{
+       /* Most test cases want 2 distinct CPUs. */
+       return num_online_cpus() < 2 ? -EINVAL : 0;
+}
+
+static void test_exit(struct kunit *test)
+{
+       for (int i = 0; i < MAX_TEST_BREAKPOINTS; ++i) {
+               if (test_bps[i])
+                       unregister_test_bp(&test_bps[i]);
+       }
+
+       if (__other_task) {
+               kthread_stop(__other_task);
+               __other_task = NULL;
+       }
+}
+
+static struct kunit_suite hw_breakpoint_test_suite = {
+       .name = "hw_breakpoint",
+       .test_cases = hw_breakpoint_test_cases,
+       .init = test_init,
+       .exit = test_exit,
+};
+
+kunit_test_suites(&hw_breakpoint_test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Marco Elver <[email protected]>");

lib/Kconfig.debug +10 -1

index 2e24db4bff19..4c87a6edf046 100644
@@ -2513,6 +2513,16 @@ config STACKINIT_KUNIT_TEST
          CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF,
          or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
 
+config HW_BREAKPOINT_KUNIT_TEST
+       bool "Test hw_breakpoint constraints accounting" if !KUNIT_ALL_TESTS
+       depends on HAVE_HW_BREAKPOINT
+       depends on KUNIT=y
+       default KUNIT_ALL_TESTS
+       help
+         Tests for hw_breakpoint constraints accounting.
+
+         If unsure, say N.
+
 config TEST_UDELAY
        tristate "udelay test driver"
        help
--

2.37.0.rc0.161.g10f37bed90-goog

Quoted 5 lines · Marco Elver

Add KUnit test for hw_breakpoint constraints accounting, with various interesting mixes of breakpoint targets (some care was taken to catch interesting corner cases via bug-injection).

Quoted 5 lines

The test cannot be built as a module because it requires access to hw_breakpoint_slots(), which is not inlinable or exported on all architectures.

Signed-off-by Marco Elver <[email protected]>

Reviewed-by Dmitry Vyukov <[email protected]>

Quoted 376 lines

---
v3:
* Don't use raw_smp_processor_id().

v2:
* New patch.

---
 kernel/events/Makefile             |   1 +
 kernel/events/hw_breakpoint_test.c | 323 +++++++++++++++++++++++++++++
 lib/Kconfig.debug                  |  10 +
 3 files changed, 334 insertions(+)
 create mode 100644 kernel/events/hw_breakpoint_test.c

kernel/events/Makefile +1 -0

index 8591c180b52b..91a62f566743 100644
@@ -2,4 +2,5 @@
 obj-y := core.o ring_buffer.o callchain.o
 
 obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
+obj-$(CONFIG_HW_BREAKPOINT_KUNIT_TEST) += hw_breakpoint_test.o
 obj-$(CONFIG_UPROBES) += uprobes.o

kernel/events/hw_breakpoint_test.c new file +323 -0

new file mode 100644
index 000000000000..433c5c45e2a5
@@ -0,0 +1,323 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for hw_breakpoint constraints accounting logic.
+ *
+ * Copyright (C) 2022, Google LLC.
+ */
+
+#include <kunit/test.h>
+#include <linux/cpumask.h>
+#include <linux/hw_breakpoint.h>
+#include <linux/kthread.h>
+#include <linux/perf_event.h>
+#include <asm/hw_breakpoint.h>
+
+#define TEST_REQUIRES_BP_SLOTS(test, slots)                                            \
+       do {                                                                            \
+               if ((slots) > get_test_bp_slots()) {                                    \
+                       kunit_skip((test), "Requires breakpoint slots: %d > %d", slots, \
+                                  get_test_bp_slots());                                \
+               }                                                                       \
+       } while (0)
+
+#define TEST_EXPECT_NOSPC(expr) KUNIT_EXPECT_EQ(test, -ENOSPC, PTR_ERR(expr))
+
+#define MAX_TEST_BREAKPOINTS 512
+
+static char break_vars[MAX_TEST_BREAKPOINTS];
+static struct perf_event *test_bps[MAX_TEST_BREAKPOINTS];
+static struct task_struct *__other_task;
+
+static struct perf_event *register_test_bp(int cpu, struct task_struct *tsk, int idx)
+{
+       struct perf_event_attr attr = {};
+
+       if (WARN_ON(idx < 0 || idx >= MAX_TEST_BREAKPOINTS))
+               return NULL;
+
+       hw_breakpoint_init(&attr);
+       attr.bp_addr = (unsigned long)&break_vars[idx];
+       attr.bp_len = HW_BREAKPOINT_LEN_1;
+       attr.bp_type = HW_BREAKPOINT_RW;
+       return perf_event_create_kernel_counter(&attr, cpu, tsk, NULL, NULL);
+}
+
+static void unregister_test_bp(struct perf_event **bp)
+{
+       if (WARN_ON(IS_ERR(*bp)))
+               return;
+       if (WARN_ON(!*bp))
+               return;
+       unregister_hw_breakpoint(*bp);
+       *bp = NULL;
+}
+
+static int get_test_bp_slots(void)
+{
+       static int slots;
+
+       if (!slots)
+               slots = hw_breakpoint_slots(TYPE_DATA);
+
+       return slots;
+}
+
+static void fill_one_bp_slot(struct kunit *test, int *id, int cpu, struct task_struct *tsk)
+{
+       struct perf_event *bp = register_test_bp(cpu, tsk, *id);
+
+       KUNIT_ASSERT_NOT_NULL(test, bp);
+       KUNIT_ASSERT_FALSE(test, IS_ERR(bp));
+       KUNIT_ASSERT_NULL(test, test_bps[*id]);
+       test_bps[(*id)++] = bp;
+}
+
+/*
+ * Fills up the given @cpu/@tsk with breakpoints, only leaving @skip slots free.
+ *
+ * Returns true if this can be called again, continuing at @id.
+ */
+static bool fill_bp_slots(struct kunit *test, int *id, int cpu, struct task_struct *tsk, int skip)
+{
+       for (int i = 0; i < get_test_bp_slots() - skip; ++i)
+               fill_one_bp_slot(test, id, cpu, tsk);
+
+       return *id + get_test_bp_slots() <= MAX_TEST_BREAKPOINTS;
+}
+
+static int dummy_kthread(void *arg)
+{
+       return 0;
+}
+
+static struct task_struct *get_other_task(struct kunit *test)
+{
+       struct task_struct *tsk;
+
+       if (__other_task)
+               return __other_task;
+
+       tsk = kthread_create(dummy_kthread, NULL, "hw_breakpoint_dummy_task");
+       KUNIT_ASSERT_FALSE(test, IS_ERR(tsk));
+       __other_task = tsk;
+       return __other_task;
+}
+
+static int get_test_cpu(int num)
+{
+       int cpu;
+
+       WARN_ON(num < 0);
+
+       for_each_online_cpu(cpu) {
+               if (num-- <= 0)
+                       break;
+       }
+
+       return cpu;
+}
+
+/* ===== Test cases ===== */
+
+static void test_one_cpu(struct kunit *test)
+{
+       int idx = 0;
+
+       fill_bp_slots(test, &idx, get_test_cpu(0), NULL, 0);
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+}
+
+static void test_many_cpus(struct kunit *test)
+{
+       int idx = 0;
+       int cpu;
+
+       /* Test that CPUs are independent. */
+       for_each_online_cpu(cpu) {
+               bool do_continue = fill_bp_slots(test, &idx, cpu, NULL, 0);
+
+               TEST_EXPECT_NOSPC(register_test_bp(cpu, NULL, idx));
+               if (!do_continue)
+                       break;
+       }
+}
+
+static void test_one_task_on_all_cpus(struct kunit *test)
+{
+       int idx = 0;
+
+       fill_bp_slots(test, &idx, -1, current, 0);
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       /* Remove one and adding back CPU-target should work. */
+       unregister_test_bp(&test_bps[0]);
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+}
+
+static void test_two_tasks_on_all_cpus(struct kunit *test)
+{
+       int idx = 0;
+
+       /* Test that tasks are independent. */
+       fill_bp_slots(test, &idx, -1, current, 0);
+       fill_bp_slots(test, &idx, -1, get_other_task(test), 0);
+
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(-1, get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       /* Remove one from first task and adding back CPU-target should not work. */
+       unregister_test_bp(&test_bps[0]);
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+}
+
+static void test_one_task_on_one_cpu(struct kunit *test)
+{
+       int idx = 0;
+
+       fill_bp_slots(test, &idx, get_test_cpu(0), current, 0);
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       /*
+        * Remove one and adding back CPU-target should work; this case is
+        * special vs. above because the task's constraints are CPU-dependent.
+        */
+       unregister_test_bp(&test_bps[0]);
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+}
+
+static void test_one_task_mixed(struct kunit *test)
+{
+       int idx = 0;
+
+       TEST_REQUIRES_BP_SLOTS(test, 3);
+
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), current);
+       fill_bp_slots(test, &idx, -1, current, 1);
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+
+       /* Transition from CPU-dependent pinned count to CPU-independent. */
+       unregister_test_bp(&test_bps[0]);
+       unregister_test_bp(&test_bps[1]);
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+}
+
+static void test_two_tasks_on_one_cpu(struct kunit *test)
+{
+       int idx = 0;
+
+       fill_bp_slots(test, &idx, get_test_cpu(0), current, 0);
+       fill_bp_slots(test, &idx, get_test_cpu(0), get_other_task(test), 0);
+
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(-1, get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       /* Can still create breakpoints on some other CPU. */
+       fill_bp_slots(test, &idx, get_test_cpu(1), NULL, 0);
+}
+
+static void test_two_tasks_on_one_all_cpus(struct kunit *test)
+{
+       int idx = 0;
+
+       fill_bp_slots(test, &idx, get_test_cpu(0), current, 0);
+       fill_bp_slots(test, &idx, -1, get_other_task(test), 0);
+
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(-1, get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), get_other_task(test), idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       /* Cannot create breakpoints on some other CPU either. */
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(1), NULL, idx));
+}
+
+static void test_task_on_all_and_one_cpu(struct kunit *test)
+{
+       int tsk_on_cpu_idx, cpu_idx;
+       int idx = 0;
+
+       TEST_REQUIRES_BP_SLOTS(test, 3);
+
+       fill_bp_slots(test, &idx, -1, current, 2);
+       /* Transitioning from only all CPU breakpoints to mixed. */
+       tsk_on_cpu_idx = idx;
+       fill_one_bp_slot(test, &idx, get_test_cpu(0), current);
+       fill_one_bp_slot(test, &idx, -1, current);
+
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+
+       /* We should still be able to use up another CPU's slots. */
+       cpu_idx = idx;
+       fill_one_bp_slot(test, &idx, get_test_cpu(1), NULL);
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(1), NULL, idx));
+
+       /* Transitioning back to task target on all CPUs. */
+       unregister_test_bp(&test_bps[tsk_on_cpu_idx]);
+       /* Still have a CPU target breakpoint in get_test_cpu(1). */
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       /* Remove it and try again. */
+       unregister_test_bp(&test_bps[cpu_idx]);
+       fill_one_bp_slot(test, &idx, -1, current);
+
+       TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+       TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(1), NULL, idx));
+}
+
+static struct kunit_case hw_breakpoint_test_cases[] = {
+       KUNIT_CASE(test_one_cpu),
+       KUNIT_CASE(test_many_cpus),
+       KUNIT_CASE(test_one_task_on_all_cpus),
+       KUNIT_CASE(test_two_tasks_on_all_cpus),
+       KUNIT_CASE(test_one_task_on_one_cpu),
+       KUNIT_CASE(test_one_task_mixed),
+       KUNIT_CASE(test_two_tasks_on_one_cpu),
+       KUNIT_CASE(test_two_tasks_on_one_all_cpus),
+       KUNIT_CASE(test_task_on_all_and_one_cpu),
+       {},
+};
+
+static int test_init(struct kunit *test)
+{
+       /* Most test cases want 2 distinct CPUs. */
+       return num_online_cpus() < 2 ? -EINVAL : 0;
+}
+
+static void test_exit(struct kunit *test)
+{
+       for (int i = 0; i < MAX_TEST_BREAKPOINTS; ++i) {
+               if (test_bps[i])
+                       unregister_test_bp(&test_bps[i]);
+       }
+
+       if (__other_task) {
+               kthread_stop(__other_task);
+               __other_task = NULL;
+       }
+}
+
+static struct kunit_suite hw_breakpoint_test_suite = {
+       .name = "hw_breakpoint",
+       .test_cases = hw_breakpoint_test_cases,
+       .init = test_init,
+       .exit = test_exit,
+};
+
+kunit_test_suites(&hw_breakpoint_test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Marco Elver <[email protected]>");

lib/Kconfig.debug +10 -1

index 2e24db4bff19..4c87a6edf046 100644
@@ -2513,6 +2513,16 @@ config STACKINIT_KUNIT_TEST
          CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF,
          or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
 
+config HW_BREAKPOINT_KUNIT_TEST
+       bool "Test hw_breakpoint constraints accounting" if !KUNIT_ALL_TESTS
+       depends on HAVE_HW_BREAKPOINT
+       depends on KUNIT=y
+       default KUNIT_ALL_TESTS
+       help
+         Tests for hw_breakpoint constraints accounting.
+
+         If unsure, say N.
+
 config TEST_UDELAY
        tristate "udelay test driver"
        help
--

2.37.0.rc0.161.g10f37bed90-goog

Add KUnit test for hw_breakpoint constraints accounting, with various interesting mixes of breakpoint targets (some care was taken to catch interesting corner cases via bug-injection).

The test cannot be built as a module because it requires access to hw_breakpoint_slots(), which is not inlinable or exported on all architectures.

Signed-off-by Marco Elver <[email protected]>

---
v3:
* Don't use raw_smp_processor_id().

v2:
* New patch.

---
 kernel/events/Makefile             |   1 +
 kernel/events/hw_breakpoint_test.c | 323 +++++++++++++++++++++++++++++
 lib/Kconfig.debug                  |  10 +
 3 files changed, 334 insertions(+)
 create mode 100644 kernel/events/hw_breakpoint_test.c

kernel/events/Makefile +1 -0

index 8591c180b52b..91a62f566743 100644
@@ -2,4 +2,5 @@
 obj-y := core.o ring_buffer.o callchain.o
 
 obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
+obj-$(CONFIG_HW_BREAKPOINT_KUNIT_TEST) += hw_breakpoint_test.o
 obj-$(CONFIG_UPROBES) += uprobes.o

kernel/events/hw_breakpoint_test.c new file +323 -0

new file mode 100644
index 000000000000..433c5c45e2a5
@@ -0,0 +1,323 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for hw_breakpoint constraints accounting logic.
+ *
+ * Copyright (C) 2022, Google LLC.
+ */
+
+#include <kunit/test.h>
+#include <linux/cpumask.h>
+#include <linux/hw_breakpoint.h>
+#include <linux/kthread.h>
+#include <linux/perf_event.h>
+#include <asm/hw_breakpoint.h>
+
+#define TEST_REQUIRES_BP_SLOTS(test, slots)						\
+	do {										\
+		if ((slots) > get_test_bp_slots()) {					\
+			kunit_skip((test), "Requires breakpoint slots: %d > %d", slots,	\
+				   get_test_bp_slots());				\
+		}									\
+	} while (0)
+
+#define TEST_EXPECT_NOSPC(expr) KUNIT_EXPECT_EQ(test, -ENOSPC, PTR_ERR(expr))
+
+#define MAX_TEST_BREAKPOINTS 512
+
+static char break_vars[MAX_TEST_BREAKPOINTS];
+static struct perf_event *test_bps[MAX_TEST_BREAKPOINTS];
+static struct task_struct *__other_task;
+
+static struct perf_event *register_test_bp(int cpu, struct task_struct *tsk, int idx)
+{
+	struct perf_event_attr attr = {};
+
+	if (WARN_ON(idx < 0 || idx >= MAX_TEST_BREAKPOINTS))
+		return NULL;
+
+	hw_breakpoint_init(&attr);
+	attr.bp_addr = (unsigned long)&break_vars[idx];
+	attr.bp_len = HW_BREAKPOINT_LEN_1;
+	attr.bp_type = HW_BREAKPOINT_RW;
+	return perf_event_create_kernel_counter(&attr, cpu, tsk, NULL, NULL);
+}
+
+static void unregister_test_bp(struct perf_event **bp)
+{
+	if (WARN_ON(IS_ERR(*bp)))
+		return;
+	if (WARN_ON(!*bp))
+		return;
+	unregister_hw_breakpoint(*bp);
+	*bp = NULL;
+}
+
+static int get_test_bp_slots(void)
+{
+	static int slots;
+
+	if (!slots)
+		slots = hw_breakpoint_slots(TYPE_DATA);
+
+	return slots;
+}
+
+static void fill_one_bp_slot(struct kunit *test, int *id, int cpu, struct task_struct *tsk)
+{
+	struct perf_event *bp = register_test_bp(cpu, tsk, *id);
+
+	KUNIT_ASSERT_NOT_NULL(test, bp);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(bp));
+	KUNIT_ASSERT_NULL(test, test_bps[*id]);
+	test_bps[(*id)++] = bp;
+}
+
+/*
+ * Fills up the given @cpu/@tsk with breakpoints, only leaving @skip slots free.
+ *
+ * Returns true if this can be called again, continuing at @id.
+ */
+static bool fill_bp_slots(struct kunit *test, int *id, int cpu, struct task_struct *tsk, int skip)
+{
+	for (int i = 0; i < get_test_bp_slots() - skip; ++i)
+		fill_one_bp_slot(test, id, cpu, tsk);
+
+	return *id + get_test_bp_slots() <= MAX_TEST_BREAKPOINTS;
+}
+
+static int dummy_kthread(void *arg)
+{
+	return 0;
+}
+
+static struct task_struct *get_other_task(struct kunit *test)
+{
+	struct task_struct *tsk;
+
+	if (__other_task)
+		return __other_task;
+
+	tsk = kthread_create(dummy_kthread, NULL, "hw_breakpoint_dummy_task");
+	KUNIT_ASSERT_FALSE(test, IS_ERR(tsk));
+	__other_task = tsk;
+	return __other_task;
+}
+
+static int get_test_cpu(int num)
+{
+	int cpu;
+
+	WARN_ON(num < 0);
+
+	for_each_online_cpu(cpu) {
+		if (num-- <= 0)
+			break;
+	}
+
+	return cpu;
+}
+
+/* ===== Test cases ===== */
+
+static void test_one_cpu(struct kunit *test)
+{
+	int idx = 0;
+
+	fill_bp_slots(test, &idx, get_test_cpu(0), NULL, 0);
+	TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+}
+
+static void test_many_cpus(struct kunit *test)
+{
+	int idx = 0;
+	int cpu;
+
+	/* Test that CPUs are independent. */
+	for_each_online_cpu(cpu) {
+		bool do_continue = fill_bp_slots(test, &idx, cpu, NULL, 0);
+
+		TEST_EXPECT_NOSPC(register_test_bp(cpu, NULL, idx));
+		if (!do_continue)
+			break;
+	}
+}
+
+static void test_one_task_on_all_cpus(struct kunit *test)
+{
+	int idx = 0;
+
+	fill_bp_slots(test, &idx, -1, current, 0);
+	TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+	/* Remove one and adding back CPU-target should work. */
+	unregister_test_bp(&test_bps[0]);
+	fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+}
+
+static void test_two_tasks_on_all_cpus(struct kunit *test)
+{
+	int idx = 0;
+
+	/* Test that tasks are independent. */
+	fill_bp_slots(test, &idx, -1, current, 0);
+	fill_bp_slots(test, &idx, -1, get_other_task(test), 0);
+
+	TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(-1, get_other_task(test), idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), get_other_task(test), idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+	/* Remove one from first task and adding back CPU-target should not work. */
+	unregister_test_bp(&test_bps[0]);
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+}
+
+static void test_one_task_on_one_cpu(struct kunit *test)
+{
+	int idx = 0;
+
+	fill_bp_slots(test, &idx, get_test_cpu(0), current, 0);
+	TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+	/*
+	 * Remove one and adding back CPU-target should work; this case is
+	 * special vs. above because the task's constraints are CPU-dependent.
+	 */
+	unregister_test_bp(&test_bps[0]);
+	fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+}
+
+static void test_one_task_mixed(struct kunit *test)
+{
+	int idx = 0;
+
+	TEST_REQUIRES_BP_SLOTS(test, 3);
+
+	fill_one_bp_slot(test, &idx, get_test_cpu(0), current);
+	fill_bp_slots(test, &idx, -1, current, 1);
+	TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+
+	/* Transition from CPU-dependent pinned count to CPU-independent. */
+	unregister_test_bp(&test_bps[0]);
+	unregister_test_bp(&test_bps[1]);
+	fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+	fill_one_bp_slot(test, &idx, get_test_cpu(0), NULL);
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+}
+
+static void test_two_tasks_on_one_cpu(struct kunit *test)
+{
+	int idx = 0;
+
+	fill_bp_slots(test, &idx, get_test_cpu(0), current, 0);
+	fill_bp_slots(test, &idx, get_test_cpu(0), get_other_task(test), 0);
+
+	TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(-1, get_other_task(test), idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), get_other_task(test), idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+	/* Can still create breakpoints on some other CPU. */
+	fill_bp_slots(test, &idx, get_test_cpu(1), NULL, 0);
+}
+
+static void test_two_tasks_on_one_all_cpus(struct kunit *test)
+{
+	int idx = 0;
+
+	fill_bp_slots(test, &idx, get_test_cpu(0), current, 0);
+	fill_bp_slots(test, &idx, -1, get_other_task(test), 0);
+
+	TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(-1, get_other_task(test), idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), get_other_task(test), idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+	/* Cannot create breakpoints on some other CPU either. */
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(1), NULL, idx));
+}
+
+static void test_task_on_all_and_one_cpu(struct kunit *test)
+{
+	int tsk_on_cpu_idx, cpu_idx;
+	int idx = 0;
+
+	TEST_REQUIRES_BP_SLOTS(test, 3);
+
+	fill_bp_slots(test, &idx, -1, current, 2);
+	/* Transitioning from only all CPU breakpoints to mixed. */
+	tsk_on_cpu_idx = idx;
+	fill_one_bp_slot(test, &idx, get_test_cpu(0), current);
+	fill_one_bp_slot(test, &idx, -1, current);
+
+	TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+
+	/* We should still be able to use up another CPU's slots. */
+	cpu_idx = idx;
+	fill_one_bp_slot(test, &idx, get_test_cpu(1), NULL);
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(1), NULL, idx));
+
+	/* Transitioning back to task target on all CPUs. */
+	unregister_test_bp(&test_bps[tsk_on_cpu_idx]);
+	/* Still have a CPU target breakpoint in get_test_cpu(1). */
+	TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+	/* Remove it and try again. */
+	unregister_test_bp(&test_bps[cpu_idx]);
+	fill_one_bp_slot(test, &idx, -1, current);
+
+	TEST_EXPECT_NOSPC(register_test_bp(-1, current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), current, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(0), NULL, idx));
+	TEST_EXPECT_NOSPC(register_test_bp(get_test_cpu(1), NULL, idx));
+}
+
+static struct kunit_case hw_breakpoint_test_cases[] = {
+	KUNIT_CASE(test_one_cpu),
+	KUNIT_CASE(test_many_cpus),
+	KUNIT_CASE(test_one_task_on_all_cpus),
+	KUNIT_CASE(test_two_tasks_on_all_cpus),
+	KUNIT_CASE(test_one_task_on_one_cpu),
+	KUNIT_CASE(test_one_task_mixed),
+	KUNIT_CASE(test_two_tasks_on_one_cpu),
+	KUNIT_CASE(test_two_tasks_on_one_all_cpus),
+	KUNIT_CASE(test_task_on_all_and_one_cpu),
+	{},
+};
+
+static int test_init(struct kunit *test)
+{
+	/* Most test cases want 2 distinct CPUs. */
+	return num_online_cpus() < 2 ? -EINVAL : 0;
+}
+
+static void test_exit(struct kunit *test)
+{
+	for (int i = 0; i < MAX_TEST_BREAKPOINTS; ++i) {
+		if (test_bps[i])
+			unregister_test_bp(&test_bps[i]);
+	}
+
+	if (__other_task) {
+		kthread_stop(__other_task);
+		__other_task = NULL;
+	}
+}
+
+static struct kunit_suite hw_breakpoint_test_suite = {
+	.name = "hw_breakpoint",
+	.test_cases = hw_breakpoint_test_cases,
+	.init = test_init,
+	.exit = test_exit,
+};
+
+kunit_test_suites(&hw_breakpoint_test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Marco Elver <[email protected]>");

lib/Kconfig.debug +10 -0

index 2e24db4bff19..4c87a6edf046 100644
@@ -2513,6 +2513,16 @@ config STACKINIT_KUNIT_TEST
 	  CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF,
 	  or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
 
+config HW_BREAKPOINT_KUNIT_TEST
+	bool "Test hw_breakpoint constraints accounting" if !KUNIT_ALL_TESTS
+	depends on HAVE_HW_BREAKPOINT
+	depends on KUNIT=y
+	default KUNIT_ALL_TESTS
+	help
+	  Tests for hw_breakpoint constraints accounting.
+
+	  If unsure, say N.
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	help

Signature · 1 line

2.37.0.rc0.161.g10f37bed90-goog

Hi Marco,

[adding Will]

Quoted 9 lines · Marco Elver

Add KUnit test for hw_breakpoint constraints accounting, with various interesting mixes of breakpoint targets (some care was taken to catch interesting corner cases via bug-injection).

The test cannot be built as a module because it requires access to hw_breakpoint_slots(), which is not inlinable or exported on all architectures.

Signed-off-by Marco Elver <[email protected]>

As mentioned on IRC, I'm seeing these tests fail on arm64 when applied atop v5.19-rc7:

| TAP version 14
| 1..1
|     # Subtest: hw_breakpoint
|     1..9
|     ok 1 - test_one_cpu
|     ok 2 - test_many_cpus
|     # test_one_task_on_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 3 - test_one_task_on_all_cpus
|     # test_two_tasks_on_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 4 - test_two_tasks_on_all_cpus
|     # test_one_task_on_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 5 - test_one_task_on_one_cpu
|     # test_one_task_mixed: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 6 - test_one_task_mixed
|     # test_two_tasks_on_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 7 - test_two_tasks_on_one_cpu
|     # test_two_tasks_on_one_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 8 - test_two_tasks_on_one_all_cpus
|     # test_task_on_all_and_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 9 - test_task_on_all_and_one_cpu
| # hw_breakpoint: pass:2 fail:7 skip:0 total:9
| # Totals: pass:2 fail:7 skip:0 total:9

... which seems to be becasue arm64 currently forbids per-task breakpoints/watchpoints in hw_breakpoint_arch_parse(), where we have:

        /*
         * Disallow per-task kernel breakpoints since these would
         * complicate the stepping code.
         */
        if (hw->ctrl.privilege == AARCH64_BREAKPOINT_EL1 && bp->hw.target)
                return -EINVAL;

... which has been the case since day one in commit:

478fcb2cdb2351dc ("arm64: Debugging support")

I'm not immediately sure what would be necessary to support per-task kernel breakpoints, but given a lot of that state is currently per-cpu, I imagine it's invasive.

Quoted 60 lines

[adding Will]

Quoted 9 lines · Marco Elver

Add KUnit test for hw_breakpoint constraints accounting, with various interesting mixes of breakpoint targets (some care was taken to catch interesting corner cases via bug-injection).

The test cannot be built as a module because it requires access to hw_breakpoint_slots(), which is not inlinable or exported on all architectures.

Signed-off-by Marco Elver <[email protected]>

As mentioned on IRC, I'm seeing these tests fail on arm64 when applied atop v5.19-rc7:

| TAP version 14
| 1..1
|     # Subtest: hw_breakpoint
|     1..9
|     ok 1 - test_one_cpu
|     ok 2 - test_many_cpus
|     # test_one_task_on_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 3 - test_one_task_on_all_cpus
|     # test_two_tasks_on_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 4 - test_two_tasks_on_all_cpus
|     # test_one_task_on_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 5 - test_one_task_on_one_cpu
|     # test_one_task_mixed: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 6 - test_one_task_mixed
|     # test_two_tasks_on_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 7 - test_two_tasks_on_one_cpu
|     # test_two_tasks_on_one_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 8 - test_two_tasks_on_one_all_cpus
|     # test_task_on_all_and_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 9 - test_task_on_all_and_one_cpu
| # hw_breakpoint: pass:2 fail:7 skip:0 total:9
| # Totals: pass:2 fail:7 skip:0 total:9

... which seems to be becasue arm64 currently forbids per-task breakpoints/watchpoints in hw_breakpoint_arch_parse(), where we have:

        /*
         * Disallow per-task kernel breakpoints since these would
         * complicate the stepping code.
         */
        if (hw->ctrl.privilege == AARCH64_BREAKPOINT_EL1 && bp->hw.target)
                return -EINVAL;

... which has been the case since day one in commit:

478fcb2cdb2351dc ("arm64: Debugging support")

Quoted 3 lines

I'm not immediately sure what would be necessary to support per-task kernel breakpoints, but given a lot of that state is currently per-cpu, I imagine it's invasive.

I would actually like to remove HW_BREAKPOINT completely for arm64 as it doesn't really work and causes problems for other interfaces such as ptrace and kgdb.

Quoted 64 lines · Will Deacon

Quoted 63 lines

[adding Will]

Quoted 9 lines · Marco Elver

Add KUnit test for hw_breakpoint constraints accounting, with various interesting mixes of breakpoint targets (some care was taken to catch interesting corner cases via bug-injection).

The test cannot be built as a module because it requires access to hw_breakpoint_slots(), which is not inlinable or exported on all architectures.

Signed-off-by Marco Elver <[email protected]>

As mentioned on IRC, I'm seeing these tests fail on arm64 when applied atop v5.19-rc7:

| TAP version 14
| 1..1
|     # Subtest: hw_breakpoint
|     1..9
|     ok 1 - test_one_cpu
|     ok 2 - test_many_cpus
|     # test_one_task_on_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 3 - test_one_task_on_all_cpus
|     # test_two_tasks_on_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 4 - test_two_tasks_on_all_cpus
|     # test_one_task_on_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 5 - test_one_task_on_one_cpu
|     # test_one_task_mixed: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 6 - test_one_task_mixed
|     # test_two_tasks_on_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 7 - test_two_tasks_on_one_cpu
|     # test_two_tasks_on_one_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 8 - test_two_tasks_on_one_all_cpus
|     # test_task_on_all_and_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 9 - test_task_on_all_and_one_cpu
| # hw_breakpoint: pass:2 fail:7 skip:0 total:9
| # Totals: pass:2 fail:7 skip:0 total:9

... which seems to be becasue arm64 currently forbids per-task breakpoints/watchpoints in hw_breakpoint_arch_parse(), where we have:

        /*
         * Disallow per-task kernel breakpoints since these would
         * complicate the stepping code.
         */
        if (hw->ctrl.privilege == AARCH64_BREAKPOINT_EL1 && bp->hw.target)
                return -EINVAL;

... which has been the case since day one in commit:

478fcb2cdb2351dc ("arm64: Debugging support")

I'm not immediately sure what would be necessary to support per-task kernel breakpoints, but given a lot of that state is currently per-cpu, I imagine it's invasive.

Quoted 3 lines

I would actually like to remove HW_BREAKPOINT completely for arm64 as it doesn't really work and causes problems for other interfaces such as ptrace and kgdb.

Will it be a localized removal of code that will be easy to revert in future? Or will it touch lots of code here and there? Let's say we come up with a very important use case for HW_BREAKPOINT and will need to make it work on arm64 as well in future.

Quoted 64 lines

Quoted 64 lines

Quoted 63 lines

[adding Will]

Quoted 9 lines · Marco Elver

Add KUnit test for hw_breakpoint constraints accounting, with various interesting mixes of breakpoint targets (some care was taken to catch interesting corner cases via bug-injection).

The test cannot be built as a module because it requires access to hw_breakpoint_slots(), which is not inlinable or exported on all architectures.

Signed-off-by Marco Elver <[email protected]>

As mentioned on IRC, I'm seeing these tests fail on arm64 when applied atop v5.19-rc7:

| TAP version 14
| 1..1
|     # Subtest: hw_breakpoint
|     1..9
|     ok 1 - test_one_cpu
|     ok 2 - test_many_cpus
|     # test_one_task_on_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 3 - test_one_task_on_all_cpus
|     # test_two_tasks_on_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 4 - test_two_tasks_on_all_cpus
|     # test_one_task_on_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 5 - test_one_task_on_one_cpu
|     # test_one_task_mixed: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 6 - test_one_task_mixed
|     # test_two_tasks_on_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 7 - test_two_tasks_on_one_cpu
|     # test_two_tasks_on_one_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 8 - test_two_tasks_on_one_all_cpus
|     # test_task_on_all_and_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 9 - test_task_on_all_and_one_cpu
| # hw_breakpoint: pass:2 fail:7 skip:0 total:9
| # Totals: pass:2 fail:7 skip:0 total:9

... which seems to be becasue arm64 currently forbids per-task breakpoints/watchpoints in hw_breakpoint_arch_parse(), where we have:

        /*
         * Disallow per-task kernel breakpoints since these would
         * complicate the stepping code.
         */
        if (hw->ctrl.privilege == AARCH64_BREAKPOINT_EL1 && bp->hw.target)
                return -EINVAL;

... which has been the case since day one in commit:

478fcb2cdb2351dc ("arm64: Debugging support")

I'm not immediately sure what would be necessary to support per-task kernel breakpoints, but given a lot of that state is currently per-cpu, I imagine it's invasive.

Quoted 8 lines

Quoted 3 lines

I would actually like to remove HW_BREAKPOINT completely for arm64 as it doesn't really work and causes problems for other interfaces such as ptrace and kgdb.

Will it be a localized removal of code that will be easy to revert in future? Or will it touch lots of code here and there? Let's say we come up with a very important use case for HW_BREAKPOINT and will need to make it work on arm64 as well in future.

My (rough) plan is to implement a lower-level abstraction for handling the underlying hardware resources, so we can layer consumers on top of that instead of funneling through hw_breakpoint. So if we figure out how to make bits of hw_breakpoint work on arm64, then it should just go on top.

The main pain point for hw_breakpoint is kernel-side {break,watch}points and I think there are open design questions about how they should work on arm64, particularly when considering the interaction with user watchpoints triggering on uaccess routines and the possibility of hitting a kernel watchpoint in irq context.

Quoted 19 lines · Will Deacon

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines · Marco Elver

I'm not immediately sure what would be necessary to support per-task kernel breakpoints, but given a lot of that state is currently per-cpu, I imagine it's invasive.

Quoted 8 lines

Quoted 3 lines

I would actually like to remove HW_BREAKPOINT completely for arm64 as it doesn't really work and causes problems for other interfaces such as ptrace and kgdb.

Will it be a localized removal of code that will be easy to revert in future? Or will it touch lots of code here and there? Let's say we come up with a very important use case for HW_BREAKPOINT and will need to make it work on arm64 as well in future.

My (rough) plan is to implement a lower-level abstraction for handling the underlying hardware resources, so we can layer consumers on top of that instead of funneling through hw_breakpoint. So if we figure out how to make bits of hw_breakpoint work on arm64, then it should just go on top.

Quoted 5 lines

The main pain point for hw_breakpoint is kernel-side {break,watch}points and I think there are open design questions about how they should work on arm64, particularly when considering the interaction with user watchpoints triggering on uaccess routines and the possibility of hitting a kernel watchpoint in irq context.

I see. Our main interest would be break/watchpoints on user addresses firing from both user-space and kernel (uaccess), so at least on irqs.

Quoted 25 lines

Quoted 24 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines · Marco Elver

I'm not immediately sure what would be necessary to support per-task kernel breakpoints, but given a lot of that state is currently per-cpu, I imagine it's invasive.

Quoted 8 lines

Quoted 3 lines

I would actually like to remove HW_BREAKPOINT completely for arm64 as it doesn't really work and causes problems for other interfaces such as ptrace and kgdb.

Will it be a localized removal of code that will be easy to revert in future? Or will it touch lots of code here and there? Let's say we come up with a very important use case for HW_BREAKPOINT and will need to make it work on arm64 as well in future.

My (rough) plan is to implement a lower-level abstraction for handling the underlying hardware resources, so we can layer consumers on top of that instead of funneling through hw_breakpoint. So if we figure out how to make bits of hw_breakpoint work on arm64, then it should just go on top.

The main pain point for hw_breakpoint is kernel-side {break,watch}points and I think there are open design questions about how they should work on arm64, particularly when considering the interaction with user watchpoints triggering on uaccess routines and the possibility of hitting a kernel watchpoint in irq context.

Quoted 2 lines

I see. Our main interest would be break/watchpoints on user addresses firing from both user-space and kernel (uaccess), so at least on irqs.

Interesting. Do other architectures report watchpoint hits on user addresses from kernel uaccess? It feels like this might be surprising to some users, and it opens up questions about accesses using different virtual aliases (e.g. via GUP) or from other entities as well (e.g. firmware, KVM guests, DMA).

Hi Marco,

[adding Will]

Quoted 9 lines · Marco Elver

Add KUnit test for hw_breakpoint constraints accounting, with various interesting mixes of breakpoint targets (some care was taken to catch interesting corner cases via bug-injection).

The test cannot be built as a module because it requires access to hw_breakpoint_slots(), which is not inlinable or exported on all architectures.

Signed-off-by Marco Elver <[email protected]>

As mentioned on IRC, I'm seeing these tests fail on arm64 when applied atop v5.19-rc7:

| TAP version 14
| 1..1
|     # Subtest: hw_breakpoint
|     1..9
|     ok 1 - test_one_cpu
|     ok 2 - test_many_cpus
|     # test_one_task_on_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 3 - test_one_task_on_all_cpus
|     # test_two_tasks_on_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 4 - test_two_tasks_on_all_cpus
|     # test_one_task_on_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 5 - test_one_task_on_one_cpu
|     # test_one_task_mixed: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 6 - test_one_task_mixed
|     # test_two_tasks_on_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 7 - test_two_tasks_on_one_cpu
|     # test_two_tasks_on_one_all_cpus: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 8 - test_two_tasks_on_one_all_cpus
|     # test_task_on_all_and_one_cpu: ASSERTION FAILED at kernel/events/hw_breakpoint_test.c:70
|     Expected IS_ERR(bp) to be false, but is true
|     not ok 9 - test_task_on_all_and_one_cpu
| # hw_breakpoint: pass:2 fail:7 skip:0 total:9
| # Totals: pass:2 fail:7 skip:0 total:9

... which seems to be becasue arm64 currently forbids per-task breakpoints/watchpoints in hw_breakpoint_arch_parse(), where we have:

        /*
         * Disallow per-task kernel breakpoints since these would
         * complicate the stepping code.
         */
        if (hw->ctrl.privilege == AARCH64_BREAKPOINT_EL1 && bp->hw.target)
                return -EINVAL;

... which has been the case since day one in commit:

478fcb2cdb2351dc ("arm64: Debugging support")

I'm not immediately sure what would be necessary to support per-task kernel breakpoints, but given a lot of that state is currently per-cpu, I imagine it's invasive.

Provide hw_breakpoint_is_used() to check if breakpoints are in use on the system.

Use it in the KUnit test to verify the global state before and after a test case.

Signed-off-by Marco Elver <[email protected]>

---
v3:
* New patch.

---
 include/linux/hw_breakpoint.h      |  3 +++
 kernel/events/hw_breakpoint.c      | 29 +++++++++++++++++++++++++++++
 kernel/events/hw_breakpoint_test.c | 12 +++++++++++-
 3 files changed, 43 insertions(+), 1 deletion(-)

include/linux/hw_breakpoint.h +3 -0

index 78dd7035d1e5..a3fb846705eb 100644
@@ -74,6 +74,7 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,
 extern int register_perf_hw_breakpoint(struct perf_event *bp);
 extern void unregister_hw_breakpoint(struct perf_event *bp);
 extern void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events);
+extern bool hw_breakpoint_is_used(void);
 
 extern int dbg_reserve_bp_slot(struct perf_event *bp);
 extern int dbg_release_bp_slot(struct perf_event *bp);

@@ -121,6 +122,8 @@ register_perf_hw_breakpoint(struct perf_event *bp)  { return -ENOSYS; }
 static inline void unregister_hw_breakpoint(struct perf_event *bp)     { }
 static inline void
 unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)        { }
+static inline bool hw_breakpoint_is_used(void)         { return false; }
+
 static inline int
 reserve_bp_slot(struct perf_event *bp)                 {return -ENOSYS; }
 static inline void release_bp_slot(struct perf_event *bp)              { }

kernel/events/hw_breakpoint.c +29 -0

index f32320ac02fd..fd5cd1f9e7fc 100644
@@ -604,6 +604,35 @@ void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)
 }
 EXPORT_SYMBOL_GPL(unregister_wide_hw_breakpoint);
 
+/**
+ * hw_breakpoint_is_used - check if breakpoints are currently used
+ *
+ * Returns: true if breakpoints are used, false otherwise.
+ */
+bool hw_breakpoint_is_used(void)
+{
+       int cpu;
+
+       if (!constraints_initialized)
+               return false;
+
+       for_each_possible_cpu(cpu) {
+               for (int type = 0; type < TYPE_MAX; ++type) {
+                       struct bp_cpuinfo *info = get_bp_info(cpu, type);
+
+                       if (info->cpu_pinned)
+                               return true;
+
+                       for (int slot = 0; slot < nr_slots[type]; ++slot) {
+                               if (info->tsk_pinned[slot])
+                                       return true;
+                       }
+               }
+       }
+
+       return false;
+}
+
 static struct notifier_block hw_breakpoint_exceptions_nb = {
        .notifier_call = hw_breakpoint_exceptions_notify,
        /* we need to be notified first */

kernel/events/hw_breakpoint_test.c +11 -2

index 433c5c45e2a5..5ced822df788 100644
@@ -294,7 +294,14 @@ static struct kunit_case hw_breakpoint_test_cases[] = {
 static int test_init(struct kunit *test)
 {
        /* Most test cases want 2 distinct CPUs. */
-       return num_online_cpus() < 2 ? -EINVAL : 0;
+       if (num_online_cpus() < 2)
+               return -EINVAL;
+
+       /* Want the system to not use breakpoints elsewhere. */
+       if (hw_breakpoint_is_used())
+               return -EBUSY;
+
+       return 0;
 }
 
 static void test_exit(struct kunit *test)

@@ -308,6 +315,9 @@ static void test_exit(struct kunit *test)
                kthread_stop(__other_task);
                __other_task = NULL;
        }
+
+       /* Verify that internal state agrees that no breakpoints are in use. */
+       KUNIT_EXPECT_FALSE(test, hw_breakpoint_is_used());
 }
 
 static struct kunit_suite hw_breakpoint_test_suite = {
--

2.37.0.rc0.161.g10f37bed90-goog

Quoted 4 lines · Marco Elver

Provide hw_breakpoint_is_used() to check if breakpoints are in use on the system.

Quoted 4 lines

Use it in the KUnit test to verify the global state before and after a test case.

Signed-off-by Marco Elver <[email protected]>

Reviewed-by Dmitry Vyukov <[email protected]>

Quoted 103 lines

---
v3:
* New patch.

---
 include/linux/hw_breakpoint.h      |  3 +++
 kernel/events/hw_breakpoint.c      | 29 +++++++++++++++++++++++++++++
 kernel/events/hw_breakpoint_test.c | 12 +++++++++++-
 3 files changed, 43 insertions(+), 1 deletion(-)

include/linux/hw_breakpoint.h +3 -0

index 78dd7035d1e5..a3fb846705eb 100644
@@ -74,6 +74,7 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,
 extern int register_perf_hw_breakpoint(struct perf_event *bp);
 extern void unregister_hw_breakpoint(struct perf_event *bp);
 extern void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events);
+extern bool hw_breakpoint_is_used(void);
 
 extern int dbg_reserve_bp_slot(struct perf_event *bp);
 extern int dbg_release_bp_slot(struct perf_event *bp);

@@ -121,6 +122,8 @@ register_perf_hw_breakpoint(struct perf_event *bp)  { return -ENOSYS; }
 static inline void unregister_hw_breakpoint(struct perf_event *bp)     { }
 static inline void
 unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)        { }
+static inline bool hw_breakpoint_is_used(void)         { return false; }
+
 static inline int
 reserve_bp_slot(struct perf_event *bp)                 {return -ENOSYS; }
 static inline void release_bp_slot(struct perf_event *bp)              { }

kernel/events/hw_breakpoint.c +29 -0

index f32320ac02fd..fd5cd1f9e7fc 100644
@@ -604,6 +604,35 @@ void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)
 }
 EXPORT_SYMBOL_GPL(unregister_wide_hw_breakpoint);
 
+/**
+ * hw_breakpoint_is_used - check if breakpoints are currently used
+ *
+ * Returns: true if breakpoints are used, false otherwise.
+ */
+bool hw_breakpoint_is_used(void)
+{
+       int cpu;
+
+       if (!constraints_initialized)
+               return false;
+
+       for_each_possible_cpu(cpu) {
+               for (int type = 0; type < TYPE_MAX; ++type) {
+                       struct bp_cpuinfo *info = get_bp_info(cpu, type);
+
+                       if (info->cpu_pinned)
+                               return true;
+
+                       for (int slot = 0; slot < nr_slots[type]; ++slot) {
+                               if (info->tsk_pinned[slot])
+                                       return true;
+                       }
+               }
+       }
+
+       return false;
+}
+
 static struct notifier_block hw_breakpoint_exceptions_nb = {
        .notifier_call = hw_breakpoint_exceptions_notify,
        /* we need to be notified first */

kernel/events/hw_breakpoint_test.c +11 -2

index 433c5c45e2a5..5ced822df788 100644
@@ -294,7 +294,14 @@ static struct kunit_case hw_breakpoint_test_cases[] = {
 static int test_init(struct kunit *test)
 {
        /* Most test cases want 2 distinct CPUs. */
-       return num_online_cpus() < 2 ? -EINVAL : 0;
+       if (num_online_cpus() < 2)
+               return -EINVAL;
+
+       /* Want the system to not use breakpoints elsewhere. */
+       if (hw_breakpoint_is_used())
+               return -EBUSY;
+
+       return 0;
 }
 
 static void test_exit(struct kunit *test)

@@ -308,6 +315,9 @@ static void test_exit(struct kunit *test)
                kthread_stop(__other_task);
                __other_task = NULL;
        }
+
+       /* Verify that internal state agrees that no breakpoints are in use. */
+       KUNIT_EXPECT_FALSE(test, hw_breakpoint_is_used());
 }
 
 static struct kunit_suite hw_breakpoint_test_suite = {
--

2.37.0.rc0.161.g10f37bed90-goog

Clean up headers:

- Remove unused <linux/kallsyms.h>

- Remove unused <linux/kprobes.h>

- Remove unused <linux/module.h>

- Remove unused <linux/smp.h>

- Add <linux/export.h> for EXPORT_SYMBOL_GPL().

- Add <linux/mutex.h> for mutex.

- Sort alphabetically.

- Move <linux/hw_breakpoint.h> to top to test it compiles on its own.

Signed-off-by Marco Elver <[email protected]>

Acked-by Dmitry Vyukov <[email protected]>

---
v2:
* Move to start of series.

---
 kernel/events/hw_breakpoint.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

kernel/events/hw_breakpoint.c +9 -11

index fd5cd1f9e7fc..6076c6346291 100644
@@ -17,23 +17,22 @@
  * This file contains the arch-independent routines.
  */
 
+#include <linux/hw_breakpoint.h>
+
+#include <linux/bug.h>
+#include <linux/cpu.h>
+#include <linux/export.h>
+#include <linux/init.h>
 #include <linux/irqflags.h>
-#include <linux/kallsyms.h>
-#include <linux/notifier.h>
-#include <linux/kprobes.h>
 #include <linux/kdebug.h>
 #include <linux/kernel.h>
-#include <linux/module.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
 #include <linux/percpu.h>
 #include <linux/sched.h>
-#include <linux/init.h>
 #include <linux/slab.h>
-#include <linux/list.h>
-#include <linux/cpu.h>
-#include <linux/smp.h>
-#include <linux/bug.h>
 
-#include <linux/hw_breakpoint.h>
 /*
  * Constraints data
  */
--

2.37.0.rc0.161.g10f37bed90-goog

On a machine with 256 CPUs, running the recently added perf breakpoint benchmark results in:

 | $> perf bench -r 30 breakpoint thread -b 4 -p 64 -t 64
 | # Running 'breakpoint/thread' benchmark:
 | # Created/joined 30 threads with 4 breakpoints and 64 parallelism
 |      Total time: 236.418 [sec]
 |
 |   123134.794271 usecs/op
 |  7880626.833333 usecs/op/cpu

The benchmark tests inherited breakpoint perf events across many threads.

Looking at a perf profile, we can see that the majority of the time is spent in various hw_breakpoint.c functions, which execute within the 'nr_bp_mutex' critical sections which then results in contention on that mutex as well:

    37.27%  [kernel]       [k] osq_lock
    34.92%  [kernel]       [k] mutex_spin_on_owner
    12.15%  [kernel]       [k] toggle_bp_slot
    11.90%  [kernel]       [k] __reserve_bp_slot

The culprit here is task_bp_pinned(), which has a runtime complexity of O(#tasks) due to storing all task breakpoints in the same list and iterating through that list looking for a matching task. Clearly, this does not scale to thousands of tasks.

Instead, make use of the "rhashtable" variant "rhltable" which stores multiple items with the same key in a list. This results in average runtime complexity of O(1) for task_bp_pinned().

With the optimization, the benchmark shows:

 | $> perf bench -r 30 breakpoint thread -b 4 -p 64 -t 64
 | # Running 'breakpoint/thread' benchmark:
 | # Created/joined 30 threads with 4 breakpoints and 64 parallelism
 |      Total time: 0.208 [sec]
 |
 |      108.422396 usecs/op
 |     6939.033333 usecs/op/cpu

On this particular setup that's a speedup of ~1135x.

While one option would be to make task_struct a breakpoint list node, this would only further bloat task_struct for infrequently used data. Furthermore, after all optimizations in this series, there's no evidence it would result in better performance: later optimizations make the time spent looking up entries in the hash table negligible (we'll reach the theoretical ideal performance i.e. no constraints).

Signed-off-by Marco Elver <[email protected]>

Reviewed-by Dmitry Vyukov <[email protected]>

---
v2:
* Commit message tweaks.

---
 include/linux/perf_event.h    |  3 +-
 kernel/events/hw_breakpoint.c | 56 ++++++++++++++++++++++-------------
 2 files changed, 37 insertions(+), 22 deletions(-)

include/linux/perf_event.h +1 -0

index 01231f1d976c..e27360436dc6 100644
@@ -36,6 +36,7 @@ struct perf_guest_info_callbacks {
 };
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
+#include <linux/rhashtable-types.h>
 #include <asm/hw_breakpoint.h>
 #endif
 

include/linux/perf_event.h +0 -0

                };
 #endif
                struct { /* amd_iommu */

kernel/events/hw_breakpoint.c +35 -22

index 6076c6346291..6d09edc80d19 100644
@@ -26,10 +26,10 @@
 #include <linux/irqflags.h>
 #include <linux/kdebug.h>
 #include <linux/kernel.h>
-#include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/notifier.h>
 #include <linux/percpu.h>
+#include <linux/rhashtable.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 

@@ -54,7 +54,13 @@ static struct bp_cpuinfo *get_bp_info(int cpu, enum bp_type_idx type)
 }
 
 /* Keep track of the breakpoints attached to tasks */
-static LIST_HEAD(bp_task_head);
+static struct rhltable task_bps_ht;
+static const struct rhashtable_params task_bps_ht_params = {
+       .head_offset = offsetof(struct hw_perf_event, bp_list),
+       .key_offset = offsetof(struct hw_perf_event, target),
+       .key_len = sizeof_field(struct hw_perf_event, target),
+       .automatic_shrinking = true,
+};
 
 static int constraints_initialized;
 

@@ -103,17 +109,23 @@ static unsigned int max_task_bp_pinned(int cpu, enum bp_type_idx type)
  */
 static int task_bp_pinned(int cpu, struct perf_event *bp, enum bp_type_idx type)
 {
-       struct task_struct *tsk = bp->hw.target;
+       struct rhlist_head *head, *pos;
        struct perf_event *iter;
        int count = 0;
 
-       list_for_each_entry(iter, &bp_task_head, hw.bp_list) {
-               if (iter->hw.target == tsk &&
-                   find_slot_idx(iter->attr.bp_type) == type &&
+       rcu_read_lock();
+       head = rhltable_lookup(&task_bps_ht, &bp->hw.target, task_bps_ht_params);
+       if (!head)
+               goto out;
+
+       rhl_for_each_entry_rcu(iter, pos, head, hw.bp_list) {
+               if (find_slot_idx(iter->attr.bp_type) == type &&
                    (iter->cpu < 0 || cpu == iter->cpu))
                        count += hw_breakpoint_weight(iter);
        }
 
+out:
+       rcu_read_unlock();
        return count;
 }
 

@@ -186,7 +198,7 @@ static void toggle_bp_task_slot(struct perf_event *bp, int cpu,
 /*
  * Add/remove the given breakpoint in our constraint table
  */
-static void
+static int
 toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
               int weight)
 {

@@ -199,7 +211,7 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
        /* Pinned counter cpu profiling */
        if (!bp->hw.target) {
                get_bp_info(bp->cpu, type)->cpu_pinned += weight;
-               return;
+               return 0;
        }
 
        /* Pinned counter task profiling */

@@ -207,9 +219,9 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
                toggle_bp_task_slot(bp, cpu, type, weight);
 
        if (enable)
-               list_add_tail(&bp->hw.bp_list, &bp_task_head);
+               return rhltable_insert(&task_bps_ht, &bp->hw.bp_list, task_bps_ht_params);
        else
-               list_del(&bp->hw.bp_list);
+               return rhltable_remove(&task_bps_ht, &bp->hw.bp_list, task_bps_ht_params);
 }
 
 __weak int arch_reserve_bp_slot(struct perf_event *bp)

@@ -307,9 +319,7 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
        if (ret)
                return ret;
 
-       toggle_bp_slot(bp, true, type, weight);
-
-       return 0;
+       return toggle_bp_slot(bp, true, type, weight);
 }
 
 int reserve_bp_slot(struct perf_event *bp)

@@ -334,7 +344,7 @@ static void __release_bp_slot(struct perf_event *bp, u64 bp_type)
 
        type = find_slot_idx(bp_type);
        weight = hw_breakpoint_weight(bp);
-       toggle_bp_slot(bp, false, type, weight);
+       WARN_ON(toggle_bp_slot(bp, false, type, weight));
 }
 
 void release_bp_slot(struct perf_event *bp)

@@ -707,7 +717,7 @@ static struct pmu perf_breakpoint = {
 int __init init_hw_breakpoint(void)
 {
        int cpu, err_cpu;
-       int i;
+       int i, ret;
 
        for (i = 0; i < TYPE_MAX; i++)
                nr_slots[i] = hw_breakpoint_slots(i);

@@ -718,18 +728,24 @@ int __init init_hw_breakpoint(void)
 
                        info->tsk_pinned = kcalloc(nr_slots[i], sizeof(int),
                                                        GFP_KERNEL);
-                       if (!info->tsk_pinned)
-                               goto err_alloc;
+                       if (!info->tsk_pinned) {
+                               ret = -ENOMEM;
+                               goto err;
+                       }
                }
        }
 
+       ret = rhltable_init(&task_bps_ht, &task_bps_ht_params);
+       if (ret)
+               goto err;
+
        constraints_initialized = 1;
 
        perf_pmu_register(&perf_breakpoint, "breakpoint", PERF_TYPE_BREAKPOINT);
 
        return register_die_notifier(&hw_breakpoint_exceptions_nb);
 
- err_alloc:
+err:
        for_each_possible_cpu(err_cpu) {
                for (i = 0; i < TYPE_MAX; i++)
                        kfree(get_bp_info(err_cpu, i)->tsk_pinned);

@@ -737,7 +753,5 @@ int __init init_hw_breakpoint(void)
                        break;
        }
 
-       return -ENOMEM;
+       return ret;
 }
-
-
--

2.37.0.rc0.161.g10f37bed90-goog

Quoted 75 lines · Marco Elver

On a machine with 256 CPUs, running the recently added perf breakpoint benchmark results in:

 | $> perf bench -r 30 breakpoint thread -b 4 -p 64 -t 64
 | # Running 'breakpoint/thread' benchmark:
 | # Created/joined 30 threads with 4 breakpoints and 64 parallelism
 |      Total time: 236.418 [sec]
 |
 |   123134.794271 usecs/op
 |  7880626.833333 usecs/op/cpu

The benchmark tests inherited breakpoint perf events across many threads.

Looking at a perf profile, we can see that the majority of the time is spent in various hw_breakpoint.c functions, which execute within the 'nr_bp_mutex' critical sections which then results in contention on that mutex as well:

    37.27%  [kernel]       [k] osq_lock
    34.92%  [kernel]       [k] mutex_spin_on_owner
    12.15%  [kernel]       [k] toggle_bp_slot
    11.90%  [kernel]       [k] __reserve_bp_slot

The culprit here is task_bp_pinned(), which has a runtime complexity of O(#tasks) due to storing all task breakpoints in the same list and iterating through that list looking for a matching task. Clearly, this does not scale to thousands of tasks.

Instead, make use of the "rhashtable" variant "rhltable" which stores multiple items with the same key in a list. This results in average runtime complexity of O(1) for task_bp_pinned().

With the optimization, the benchmark shows:

 | $> perf bench -r 30 breakpoint thread -b 4 -p 64 -t 64
 | # Running 'breakpoint/thread' benchmark:
 | # Created/joined 30 threads with 4 breakpoints and 64 parallelism
 |      Total time: 0.208 [sec]
 |
 |      108.422396 usecs/op
 |     6939.033333 usecs/op/cpu

On this particular setup that's a speedup of ~1135x.

While one option would be to make task_struct a breakpoint list node, this would only further bloat task_struct for infrequently used data. Furthermore, after all optimizations in this series, there's no evidence it would result in better performance: later optimizations make the time spent looking up entries in the hash table negligible (we'll reach the theoretical ideal performance i.e. no constraints).

Signed-off-by Marco Elver <[email protected]>

Reviewed-by Dmitry Vyukov <[email protected]>

---
v2:
* Commit message tweaks.

---
 include/linux/perf_event.h    |  3 +-
 kernel/events/hw_breakpoint.c | 56 ++++++++++++++++++++++-------------
 2 files changed, 37 insertions(+), 22 deletions(-)

include/linux/perf_event.h +1 -0

index 01231f1d976c..e27360436dc6 100644
@@ -36,6 +36,7 @@ struct perf_guest_info_callbacks {
 };
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
+#include <linux/rhashtable-types.h>
 #include <asm/hw_breakpoint.h>
 #endif
 

include/linux/perf_event.h struct hw_perf_event { · line 182

Quoted 6 lines

include/linux/perf_event.h +1 -1

@@ -178,7 +179,7 @@ struct hw_perf_event {
                         * creation and event initalization.
                         */
                        struct arch_hw_breakpoint       info;
-                       struct list_head                bp_list;
+                       struct rhlist_head              bp_list;

nit: perhaps it would be more intention revealing here to rename this to bp_hashtable?

Acked-by Ian Rogers <[email protected]>

Mark read-only data after initialization as __ro_after_init.

While we are here, turn 'constraints_initialized' into a bool.

Signed-off-by Marco Elver <[email protected]>

Reviewed-by Dmitry Vyukov <[email protected]>

---
 kernel/events/hw_breakpoint.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

kernel/events/hw_breakpoint.c +3 -4

index 6d09edc80d19..7df46b276452 100644
@@ -46,7 +46,7 @@ struct bp_cpuinfo {
 };
 
 static DEFINE_PER_CPU(struct bp_cpuinfo, bp_cpuinfo[TYPE_MAX]);
-static int nr_slots[TYPE_MAX];
+static int nr_slots[TYPE_MAX] __ro_after_init;
 
 static struct bp_cpuinfo *get_bp_info(int cpu, enum bp_type_idx type)
 {

@@ -62,7 +62,7 @@ static const struct rhashtable_params task_bps_ht_params = {
        .automatic_shrinking = true,
 };
 
-static int constraints_initialized;
+static bool constraints_initialized __ro_after_init;
 
 /* Gather the number of total pinned and un-pinned bp in a cpuset */
 struct bp_busy_slots {

@@ -739,7 +739,7 @@ int __init init_hw_breakpoint(void)
        if (ret)
                goto err;
 
-       constraints_initialized = 1;
+       constraints_initialized = true;
 
        perf_pmu_register(&perf_breakpoint, "breakpoint", PERF_TYPE_BREAKPOINT);
 
--

2.37.0.rc0.161.g10f37bed90-goog

Optimize internal hw_breakpoint state if the architecture's number of breakpoint slots is constant. This avoids several kmalloc() calls and potentially unnecessary failures if the allocations fail, as well as subtly improves code generation and cache locality.

The protocol is that if an architecture defines hw_breakpoint_slots via the preprocessor, it must be constant and the same for all types.

Signed-off-by Marco Elver <[email protected]>

Acked-by Dmitry Vyukov <[email protected]>

---
 arch/sh/include/asm/hw_breakpoint.h  |  5 +-
 arch/x86/include/asm/hw_breakpoint.h |  5 +-
 kernel/events/hw_breakpoint.c        | 94 ++++++++++++++++++----------
 3 files changed, 63 insertions(+), 41 deletions(-)

arch/sh/include/asm/hw_breakpoint.h +1 -4

index 199d17b765f2..361a0f57bdeb 100644
@@ -48,10 +48,7 @@ struct pmu;
 /* Maximum number of UBC channels */
 #define HBP_NUM                2
 
-static inline int hw_breakpoint_slots(int type)
-{
-       return HBP_NUM;
-}
+#define hw_breakpoint_slots(type) (HBP_NUM)
 
 /* arch/sh/kernel/hw_breakpoint.c */
 extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);

arch/x86/include/asm/hw_breakpoint.h +1 -4

index a1f0e90d0818..0bc931cd0698 100644
@@ -44,10 +44,7 @@ struct arch_hw_breakpoint {
 /* Total number of available HW breakpoint registers */
 #define HBP_NUM 4
 
-static inline int hw_breakpoint_slots(int type)
-{
-       return HBP_NUM;
-}
+#define hw_breakpoint_slots(type) (HBP_NUM)
 
 struct perf_event_attr;
 struct perf_event;

kernel/events/hw_breakpoint.c +61 -34

index 7df46b276452..9fb66d358d81 100644
@@ -40,13 +40,16 @@ struct bp_cpuinfo {
        /* Number of pinned cpu breakpoints in a cpu */
        unsigned int    cpu_pinned;
        /* tsk_pinned[n] is the number of tasks having n+1 breakpoints */
+#ifdef hw_breakpoint_slots
+       unsigned int    tsk_pinned[hw_breakpoint_slots(0)];
+#else
        unsigned int    *tsk_pinned;
+#endif
        /* Number of non-pinned cpu/task breakpoints in a cpu */
        unsigned int    flexible; /* XXX: placeholder, see fetch_this_slot() */
 };
 
 static DEFINE_PER_CPU(struct bp_cpuinfo, bp_cpuinfo[TYPE_MAX]);
-static int nr_slots[TYPE_MAX] __ro_after_init;
 
 static struct bp_cpuinfo *get_bp_info(int cpu, enum bp_type_idx type)
 {

@@ -73,6 +76,54 @@ struct bp_busy_slots {
 /* Serialize accesses to the above constraints */
 static DEFINE_MUTEX(nr_bp_mutex);
 
+#ifdef hw_breakpoint_slots
+/*
+ * Number of breakpoint slots is constant, and the same for all types.
+ */
+static_assert(hw_breakpoint_slots(TYPE_INST) == hw_breakpoint_slots(TYPE_DATA));
+static inline int hw_breakpoint_slots_cached(int type) { return hw_breakpoint_slots(type); }
+static inline int init_breakpoint_slots(void)          { return 0; }
+#else
+/*
+ * Dynamic number of breakpoint slots.
+ */
+static int __nr_bp_slots[TYPE_MAX] __ro_after_init;
+
+static inline int hw_breakpoint_slots_cached(int type)
+{
+       return __nr_bp_slots[type];
+}
+
+static __init int init_breakpoint_slots(void)
+{
+       int i, cpu, err_cpu;
+
+       for (i = 0; i < TYPE_MAX; i++)
+               __nr_bp_slots[i] = hw_breakpoint_slots(i);
+
+       for_each_possible_cpu(cpu) {
+               for (i = 0; i < TYPE_MAX; i++) {
+                       struct bp_cpuinfo *info = get_bp_info(cpu, i);
+
+                       info->tsk_pinned = kcalloc(__nr_bp_slots[i], sizeof(int), GFP_KERNEL);
+                       if (!info->tsk_pinned)
+                               goto err;
+               }
+       }
+
+       return 0;
+err:
+       for_each_possible_cpu(err_cpu) {
+               for (i = 0; i < TYPE_MAX; i++)
+                       kfree(get_bp_info(err_cpu, i)->tsk_pinned);
+               if (err_cpu == cpu)
+                       break;
+       }
+
+       return -ENOMEM;
+}
+#endif
+
 __weak int hw_breakpoint_weight(struct perf_event *bp)
 {
        return 1;

@@ -95,7 +146,7 @@ static unsigned int max_task_bp_pinned(int cpu, enum bp_type_idx type)
        unsigned int *tsk_pinned = get_bp_info(cpu, type)->tsk_pinned;
        int i;
 
-       for (i = nr_slots[type] - 1; i >= 0; i--) {
+       for (i = hw_breakpoint_slots_cached(type) - 1; i >= 0; i--) {
                if (tsk_pinned[i] > 0)
                        return i + 1;
        }

@@ -312,7 +363,7 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
        fetch_this_slot(&slots, weight);
 
        /* Flexible counters need to keep at least one slot */
-       if (slots.pinned + (!!slots.flexible) > nr_slots[type])
+       if (slots.pinned + (!!slots.flexible) > hw_breakpoint_slots_cached(type))
                return -ENOSPC;
 
        ret = arch_reserve_bp_slot(bp);

@@ -632,7 +683,7 @@ bool hw_breakpoint_is_used(void)
                        if (info->cpu_pinned)
                                return true;
 
-                       for (int slot = 0; slot < nr_slots[type]; ++slot) {
+                       for (int slot = 0; slot < hw_breakpoint_slots_cached(type); ++slot) {
                                if (info->tsk_pinned[slot])
                                        return true;
                        }

@@ -716,42 +767,19 @@ static struct pmu perf_breakpoint = {
 
 int __init init_hw_breakpoint(void)
 {
-       int cpu, err_cpu;
-       int i, ret;
-
-       for (i = 0; i < TYPE_MAX; i++)
-               nr_slots[i] = hw_breakpoint_slots(i);
-
-       for_each_possible_cpu(cpu) {
-               for (i = 0; i < TYPE_MAX; i++) {
-                       struct bp_cpuinfo *info = get_bp_info(cpu, i);
-
-                       info->tsk_pinned = kcalloc(nr_slots[i], sizeof(int),
-                                                       GFP_KERNEL);
-                       if (!info->tsk_pinned) {
-                               ret = -ENOMEM;
-                               goto err;
-                       }
-               }
-       }
+       int ret;
 
        ret = rhltable_init(&task_bps_ht, &task_bps_ht_params);
        if (ret)
-               goto err;
+               return ret;
+
+       ret = init_breakpoint_slots();
+       if (ret)
+               return ret;
 
        constraints_initialized = true;
 
        perf_pmu_register(&perf_breakpoint, "breakpoint", PERF_TYPE_BREAKPOINT);
 
        return register_die_notifier(&hw_breakpoint_exceptions_nb);
-
-err:
-       for_each_possible_cpu(err_cpu) {
-               for (i = 0; i < TYPE_MAX; i++)
-                       kfree(get_bp_info(err_cpu, i)->tsk_pinned);
-               if (err_cpu == cpu)
-                       break;
-       }
-
-       return ret;
 }
--

2.37.0.rc0.161.g10f37bed90-goog

Due to being a __weak function, hw_breakpoint_weight() will cause the compiler to always emit a call to it. This generates unnecessarily bad code (register spills etc.) for no good reason; in fact it appears in profiles of perf bench -r 100 breakpoint thread -b 4 -p 128 -t 512:

    ...
    0.70%  [kernel]       [k] hw_breakpoint_weight
    ...

While a small percentage, no architecture defines its own hw_breakpoint_weight() nor are there users outside hw_breakpoint.c, which makes the fact it is currently __weak a poor choice.

Change hw_breakpoint_weight()'s definition to follow a similar protocol to hw_breakpoint_slots(), such that if <asm/hw_breakpoint.h> defines hw_breakpoint_weight(), we'll use it instead.

The result is that it is inlined and no longer shows up in profiles.

Signed-off-by Marco Elver <[email protected]>

Reviewed-by Dmitry Vyukov <[email protected]>

---
 include/linux/hw_breakpoint.h | 1 -
 kernel/events/hw_breakpoint.c | 4 +++-
 2 files changed, 3 insertions(+), 2 deletions(-)

include/linux/hw_breakpoint.h +0 -1

index a3fb846705eb..f319bd26b030 100644
@@ -80,7 +80,6 @@ extern int dbg_reserve_bp_slot(struct perf_event *bp);
 extern int dbg_release_bp_slot(struct perf_event *bp);
 extern int reserve_bp_slot(struct perf_event *bp);
 extern void release_bp_slot(struct perf_event *bp);
-int hw_breakpoint_weight(struct perf_event *bp);
 int arch_reserve_bp_slot(struct perf_event *bp);
 void arch_release_bp_slot(struct perf_event *bp);
 void arch_unregister_hw_breakpoint(struct perf_event *bp);

kernel/events/hw_breakpoint.c +3 -2

index 9fb66d358d81..9c9bf17666a5 100644
@@ -124,10 +124,12 @@ static __init int init_breakpoint_slots(void)
 }
 #endif
 
-__weak int hw_breakpoint_weight(struct perf_event *bp)
+#ifndef hw_breakpoint_weight
+static inline int hw_breakpoint_weight(struct perf_event *bp)
 {
        return 1;
 }
+#endif
 
 static inline enum bp_type_idx find_slot_idx(u64 bp_type)
 {
--

2.37.0.rc0.161.g10f37bed90-goog

Flexible breakpoints have never been implemented, with bp_cpuinfo::flexible always being 0. Unfortunately, they still occupy 4 bytes in each bp_cpuinfo and bp_busy_slots, as well as computing the max flexible count in fetch_bp_busy_slots().

This again causes suboptimal code generation, when we always know that !!slots.flexible will be 0.

Just get rid of the flexible "placeholder" and remove all real code related to it. Make a note in the comment related to the constraints algorithm but don't remove them from the algorithm, so that if in future flexible breakpoints need supporting, it should be trivial to revive them (along with reverting this change).

Signed-off-by Marco Elver <[email protected]>

Reviewed-by Dmitry Vyukov <[email protected]>

---
v2:
* Also remove struct bp_busy_slots, and simplify functions.

---
 kernel/events/hw_breakpoint.c | 57 +++++++++++------------------------
 1 file changed, 17 insertions(+), 40 deletions(-)

kernel/events/hw_breakpoint.c +17 -41

index 9c9bf17666a5..8b40fca1a063 100644
@@ -45,8 +45,6 @@ struct bp_cpuinfo {
 #else
        unsigned int    *tsk_pinned;
 #endif
-       /* Number of non-pinned cpu/task breakpoints in a cpu */
-       unsigned int    flexible; /* XXX: placeholder, see fetch_this_slot() */
 };
 
 static DEFINE_PER_CPU(struct bp_cpuinfo, bp_cpuinfo[TYPE_MAX]);

@@ -67,12 +65,6 @@ static const struct rhashtable_params task_bps_ht_params = {
 
 static bool constraints_initialized __ro_after_init;
 
-/* Gather the number of total pinned and un-pinned bp in a cpuset */
-struct bp_busy_slots {
-       unsigned int pinned;
-       unsigned int flexible;
-};
-
 /* Serialize accesses to the above constraints */
 static DEFINE_MUTEX(nr_bp_mutex);
 

@@ -190,14 +182,14 @@ static const struct cpumask *cpumask_of_bp(struct perf_event *bp)
 }
 
 /*
- * Report the number of pinned/un-pinned breakpoints we have in
- * a given cpu (cpu > -1) or in all of them (cpu = -1).
+ * Returns the max pinned breakpoint slots in a given
+ * CPU (cpu > -1) or across all of them (cpu = -1).
  */
-static void
-fetch_bp_busy_slots(struct bp_busy_slots *slots, struct perf_event *bp,
-                   enum bp_type_idx type)
+static int
+max_bp_pinned_slots(struct perf_event *bp, enum bp_type_idx type)
 {
        const struct cpumask *cpumask = cpumask_of_bp(bp);
+       int pinned_slots = 0;
        int cpu;
 
        for_each_cpu(cpu, cpumask) {

@@ -210,24 +202,10 @@ fetch_bp_busy_slots(struct bp_busy_slots *slots, struct perf_event *bp,
                else
                        nr += task_bp_pinned(cpu, bp, type);
 
-               if (nr > slots->pinned)
-                       slots->pinned = nr;
-
-               nr = info->flexible;
-               if (nr > slots->flexible)
-                       slots->flexible = nr;
+               pinned_slots = max(nr, pinned_slots);
        }
-}
 
-/*
- * For now, continue to consider flexible as pinned, until we can
- * ensure no flexible event can ever be scheduled before a pinned event
- * in a same cpu.
- */
-static void
-fetch_this_slot(struct bp_busy_slots *slots, int weight)
-{
-       slots->pinned += weight;
+       return pinned_slots;
 }
 
 /*

@@ -298,7 +276,12 @@ __weak void arch_unregister_hw_breakpoint(struct perf_event *bp)
 }
 
 /*
- * Constraints to check before allowing this new breakpoint counter:
+ * Constraints to check before allowing this new breakpoint counter.
+ *
+ * Note: Flexible breakpoints are currently unimplemented, but outlined in the
+ * below algorithm for completeness.  The implementation treats flexible as
+ * pinned due to no guarantee that we currently always schedule flexible events
+ * before a pinned event in a same CPU.
  *
  *  == Non-pinned counter == (Considered as pinned for now)
  *

@@ -340,8 +323,8 @@ __weak void arch_unregister_hw_breakpoint(struct perf_event *bp)
  */
 static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
 {
-       struct bp_busy_slots slots = {0};
        enum bp_type_idx type;
+       int max_pinned_slots;
        int weight;
        int ret;
 

@@ -357,15 +340,9 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
        type = find_slot_idx(bp_type);
        weight = hw_breakpoint_weight(bp);
 
-       fetch_bp_busy_slots(&slots, bp, type);
-       /*
-        * Simulate the addition of this breakpoint to the constraints
-        * and see the result.
-        */
-       fetch_this_slot(&slots, weight);
-
-       /* Flexible counters need to keep at least one slot */
-       if (slots.pinned + (!!slots.flexible) > hw_breakpoint_slots_cached(type))
+       /* Check if this new breakpoint can be satisfied across all CPUs. */
+       max_pinned_slots = max_bp_pinned_slots(bp, type) + weight;
+       if (max_pinned_slots > hw_breakpoint_slots_cached(type))
                return -ENOSPC;
 
        ret = arch_reserve_bp_slot(bp);
--

2.37.0.rc0.161.g10f37bed90-goog

Internal data structures (cpu_bps, task_bps) of powerpc's hw_breakpoint implementation have relied on nr_bp_mutex serializing access to them.

Before overhauling synchronization of kernel/events/hw_breakpoint.c, introduce 2 spinlocks to synchronize cpu_bps and task_bps respectively, thus avoiding reliance on callers synchronizing powerpc's hw_breakpoint.

Reported-by Dmitry Vyukov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

Acked-by Dmitry Vyukov <[email protected]>

---
v2:
* New patch.

---
 arch/powerpc/kernel/hw_breakpoint.c | 53 ++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 13 deletions(-)

arch/powerpc/kernel/hw_breakpoint.c +40 -14

index 2669f80b3a49..8db1a15d7acb 100644
@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/sched.h>
 #include <linux/smp.h>
+#include <linux/spinlock.h>
 #include <linux/debugfs.h>
 #include <linux/init.h>
 

@@ -129,7 +130,14 @@ struct breakpoint {
        bool ptrace_bp;
 };
 
+/*
+ * While kernel/events/hw_breakpoint.c does its own synchronization, we cannot
+ * rely on it safely synchronizing internals here; however, we can rely on it
+ * not requesting more breakpoints than available.
+ */
+static DEFINE_SPINLOCK(cpu_bps_lock);
 static DEFINE_PER_CPU(struct breakpoint *, cpu_bps[HBP_NUM_MAX]);
+static DEFINE_SPINLOCK(task_bps_lock);
 static LIST_HEAD(task_bps);
 
 static struct breakpoint *alloc_breakpoint(struct perf_event *bp)

@@ -174,7 +182,9 @@ static int task_bps_add(struct perf_event *bp)
        if (IS_ERR(tmp))
                return PTR_ERR(tmp);
 
+       spin_lock(&task_bps_lock);
        list_add(&tmp->list, &task_bps);
+       spin_unlock(&task_bps_lock);
        return 0;
 }
 

@@ -182,6 +192,7 @@ static void task_bps_remove(struct perf_event *bp)
 {
        struct list_head *pos, *q;
 
+       spin_lock(&task_bps_lock);
        list_for_each_safe(pos, q, &task_bps) {
                struct breakpoint *tmp = list_entry(pos, struct breakpoint, list);
 

@@ -191,6 +202,7 @@ static void task_bps_remove(struct perf_event *bp)
                        break;
                }
        }
+       spin_unlock(&task_bps_lock);
 }
 
 /*

@@ -200,12 +212,17 @@ static void task_bps_remove(struct perf_event *bp)
 static bool all_task_bps_check(struct perf_event *bp)
 {
        struct breakpoint *tmp;
+       bool ret = false;
 
+       spin_lock(&task_bps_lock);
        list_for_each_entry(tmp, &task_bps, list) {
-               if (!can_co_exist(tmp, bp))
-                       return true;
+               if (!can_co_exist(tmp, bp)) {
+                       ret = true;
+                       break;
+               }
        }
-       return false;
+       spin_unlock(&task_bps_lock);
+       return ret;
 }
 
 /*

@@ -215,13 +232,18 @@ static bool all_task_bps_check(struct perf_event *bp)
 static bool same_task_bps_check(struct perf_event *bp)
 {
        struct breakpoint *tmp;
+       bool ret = false;
 
+       spin_lock(&task_bps_lock);
        list_for_each_entry(tmp, &task_bps, list) {
                if (tmp->bp->hw.target == bp->hw.target &&
-                   !can_co_exist(tmp, bp))
-                       return true;
+                   !can_co_exist(tmp, bp)) {
+                       ret = true;
+                       break;
+               }
        }
-       return false;
+       spin_unlock(&task_bps_lock);
+       return ret;
 }
 
 static int cpu_bps_add(struct perf_event *bp)

@@ -234,6 +256,7 @@ static int cpu_bps_add(struct perf_event *bp)
        if (IS_ERR(tmp))
                return PTR_ERR(tmp);
 
+       spin_lock(&cpu_bps_lock);
        cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
        for (i = 0; i < nr_wp_slots(); i++) {
                if (!cpu_bp[i]) {

@@ -241,6 +264,7 @@ static int cpu_bps_add(struct perf_event *bp)
                        break;
                }
        }
+       spin_unlock(&cpu_bps_lock);
        return 0;
 }
 

@@ -249,6 +273,7 @@ static void cpu_bps_remove(struct perf_event *bp)
        struct breakpoint **cpu_bp;
        int i = 0;
 
+       spin_lock(&cpu_bps_lock);
        cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
        for (i = 0; i < nr_wp_slots(); i++) {
                if (!cpu_bp[i])

@@ -260,19 +285,25 @@ static void cpu_bps_remove(struct perf_event *bp)
                        break;
                }
        }
+       spin_unlock(&cpu_bps_lock);
 }
 
 static bool cpu_bps_check(int cpu, struct perf_event *bp)
 {
        struct breakpoint **cpu_bp;
+       bool ret = false;
        int i;
 
+       spin_lock(&cpu_bps_lock);
        cpu_bp = per_cpu_ptr(cpu_bps, cpu);
        for (i = 0; i < nr_wp_slots(); i++) {
-               if (cpu_bp[i] && !can_co_exist(cpu_bp[i], bp))
-                       return true;
+               if (cpu_bp[i] && !can_co_exist(cpu_bp[i], bp)) {
+                       ret = true;
+                       break;
+               }
        }
-       return false;
+       spin_unlock(&cpu_bps_lock);
+       return ret;
 }
 
 static bool all_cpu_bps_check(struct perf_event *bp)

@@ -286,10 +317,6 @@ static bool all_cpu_bps_check(struct perf_event *bp)
        return false;
 }
 
-/*
- * We don't use any locks to serialize accesses to cpu_bps or task_bps
- * because are already inside nr_bp_mutex.
- */
 int arch_reserve_bp_slot(struct perf_event *bp)
 {
        int ret;
--

2.37.0.rc0.161.g10f37bed90-goog