Messages in this thread | | | From | Marco Elver <> | Date | Tue, 28 Jun 2022 15:26:09 +0200 | Subject | Re: [PATCH v2 01/13] perf/hw_breakpoint: Add KUnit test for constraints accounting |
| |
On Tue, 28 Jun 2022 at 14:53, Dmitry Vyukov <dvyukov@google.com> wrote: > > On Tue, 28 Jun 2022 at 11:59, Marco Elver <elver@google.com> wrote: > > > > 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 <elver@google.com> > > --- > > v2: > > * New patch. > > --- > > kernel/events/Makefile | 1 + > > kernel/events/hw_breakpoint_test.c | 321 +++++++++++++++++++++++++++++ > > lib/Kconfig.debug | 10 + > > 3 files changed, 332 insertions(+) > > create mode 100644 kernel/events/hw_breakpoint_test.c > > > > diff --git a/kernel/events/Makefile b/kernel/events/Makefile > > index 8591c180b52b..91a62f566743 100644 > > --- a/kernel/events/Makefile > > +++ b/kernel/events/Makefile > > @@ -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 > > diff --git a/kernel/events/hw_breakpoint_test.c b/kernel/events/hw_breakpoint_test.c > > new file mode 100644 > > index 000000000000..747a0249a606 > > --- /dev/null > > +++ b/kernel/events/hw_breakpoint_test.c > > @@ -0,0 +1,321 @@ > > +// 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; > > Why is this function needed? Is hw_breakpoint_slots() very slow?
It seems non-trivial on some architectures (e.g. arch/arm64/kernel/hw_breakpoint.c). Also the reason why hw_breakpoint.c itself caches it, so I decided to follow the same because it's called very often in the tests.
> > + > > + 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_other_cpu(void) > > +{ > > + int cpu; > > + > > + for_each_online_cpu(cpu) { > > + if (cpu != raw_smp_processor_id()) > > Are we guaranteed to not be rescheduled in the middle of a test? > If not, can't get_other_cpu() return the same CPU that was returned by > raw_smp_processor_id() earlier in the test?
Yes, good point. I think I'll change it to just not use raw_smp_processor_id() and instead have get_test_cpu(int num) and it tries to find the 'num' online CPU. In the tests I'll just use CPU #num 0 and 1.
| |