Harry wrote:
> Since kmalloc_nolock() always fails in NMI and hardirq contexts on
> PREEMPT_RT, slub_kunit cannot properly test _nolock() APIs.
>
> Register a kprobe pre-handler to invoke kmalloc_nolock() and
> kfree_nolock() in the middle of the slab allocator. However, do not
> register the handler on UP kernels because that use case is not
> well supported [1] in the kernel.
>
> To attach the pre-handler while s->cpu_sheaves->lock or n->list_lock
> is held, add a wrapper function for lockdep_assert_held() that calls
> a no-op function slab_attach_kprobe_locked() on debug builds. The
> function is optimized away when neither CONFIG_PROVE_LOCKING nor
> CONFIG_DEBUG_VM is selected and register_kprobe() fails.
>
> The function calls barrier() to prevent the compiler from optimizing
> away its callsites. Otherwise, the compiler may consider the function
> does not have any side effect and remove callsites.
>
> Compared to using plain kprobe, this has two advantages: 1) it avoids
> hardcoding function names in the test, and 2) it can trigger those APIs
> in the middle of a function, where the lock is expected to be held as
> annotated with lockdep.
>
> While it was proposed [2] to use kunit function redirection to test
> this, it is currently infeasible as some lock helpers don't have
> symbols.
>
> Factor out the nested loop that calls kmalloc and friends to
> test_kmalloc_kfree(), and call them in
> test_kmalloc_kfree_nolock_{perf,kprobe}(), each being an independent
> test case. During the refactoring, drop alloc_fail handling as it
> doesn't provide much benefits.
Nice test addition!
Reviewed-by: Shengming Hu <[email protected]>
Nit below:
> Link:
> https://lore.kernel.org/linux-mm/[email protected]
> [1]
> Link:
> https://lore.kernel.org/linux-mm/[email protected]
> [2]
> Acked-by: Vlastimil Babka (SUSE) <[email protected]>
> Signed-off-by: Harry Yoo (Oracle) <[email protected]>
> ---
> lib/tests/slub_kunit.c | 167
> +++++++++++++++++++++++++++++++++++--------------
> mm/slub.c | 36 ++++++++---
> 2 files changed, 148 insertions(+), 55 deletions(-)
>
> diff --git a/lib/tests/slub_kunit.c b/lib/tests/slub_kunit.c
> index fa6d31dbca16..a391467c1aa6 100644
> --- a/lib/tests/slub_kunit.c
> +++ b/lib/tests/slub_kunit.c
> @@ -8,6 +8,7 @@
> #include <linux/rcupdate.h>
> #include <linux/delay.h>
> #include <linux/perf_event.h>
> +#include <linux/kprobes.h>
> #include "../mm/slab.h"
>
> static struct kunit_resource resource;
> @@ -292,7 +293,7 @@ static void test_krealloc_redzone_zeroing(struct kunit
> *test)
> kmem_cache_destroy(s);
> }
>
> -#ifdef CONFIG_PERF_EVENTS
> +#if defined(CONFIG_PERF_EVENTS) || (defined(CONFIG_KPROBES) &&
> defined(CONFIG_SMP))
> #define NR_ITERATIONS 1000
> #define NR_OBJECTS 1000
> static void *objects[NR_OBJECTS];
> @@ -302,26 +303,40 @@ struct test_nolock_context {
> int callback_count;
> int alloc_ok;
> int alloc_fail;
> +#ifdef CONFIG_PERF_EVENTS
> struct perf_event *event;
> +#endif
> +#if defined(CONFIG_KPROBES) && defined(CONFIG_SMP)
> + struct kprobe kprobe;
> +#endif
> };
>
> -static struct perf_event_attr hw_attr = {
> - .type = PERF_TYPE_HARDWARE,
> - .config = PERF_COUNT_HW_CPU_CYCLES,
> - .size = sizeof(struct perf_event_attr),
> - .pinned = 1,
> - .disabled = 1,
> - .freq = 1,
> - .sample_freq = 100000,
> -};
> +static void test_kmalloc_kfree(void)
> +{
> + int i, j;
> +
> + for (i = 0; i < NR_ITERATIONS; i++) {
> + for (j = 0; j < NR_OBJECTS; j++) {
> + gfp_t gfp = (i % 2) ? GFP_KERNEL : GFP_KERNEL_ACCOUNT;
>
> -static void overflow_handler_test_kmalloc_kfree_nolock(struct perf_event
> *event,
> - struct perf_sample_data *data,
> - struct pt_regs *regs)
> + objects[j] = kmalloc_obj(*objects[j], gfp);
> + if (!objects[j]) {
> + j--;
> + while (j >= 0)
> + kfree(objects[j--]);
> + return;
> + }
> + }
> +
> + for (j = 0; j < NR_OBJECTS; j++)
> + kfree(objects[j]);
> + }
> +}
> +
> +static void test_nolock(struct test_nolock_context *ctx)
> {
> void *objp;
> gfp_t gfp;
> - struct test_nolock_context *ctx = event->overflow_handler_context;
>
> /* __GFP_ACCOUNT to test kmalloc_nolock() in alloc_slab_obj_exts() */
> gfp = (ctx->callback_count % 2) ? 0 : __GFP_ACCOUNT;
> @@ -335,47 +350,104 @@ static void
> overflow_handler_test_kmalloc_kfree_nolock(struct perf_event *event,
> kfree_nolock(objp);
> ctx->callback_count++;
> }
> +#endif
>
> -static void test_kmalloc_kfree_nolock(struct kunit *test)
> +#ifdef CONFIG_PERF_EVENTS
> +static struct perf_event_attr hw_attr = {
> + .type = PERF_TYPE_HARDWARE,
> + .config = PERF_COUNT_HW_CPU_CYCLES,
> + .size = sizeof(struct perf_event_attr),
> + .pinned = 1,
> + .disabled = 1,
> + .freq = 1,
> + .sample_freq = 100000,
> +};
> +
> +static void overflow_handler_test_nolock(struct perf_event *event,
> + struct perf_sample_data *data,
> + struct pt_regs *regs)
> +{
> + struct test_nolock_context *ctx = event->overflow_handler_context;
> +
> + test_nolock(ctx);
> +}
> +
> +static bool enable_perf_events(struct test_nolock_context *ctx)
> {
> - int i, j;
> - struct test_nolock_context ctx = { .test = test };
> struct perf_event *event;
> - bool alloc_fail = false;
>
> event = perf_event_create_kernel_counter(&hw_attr, -1, current,
> - overflow_handler_test_kmalloc_kfree_nolock,
> - &ctx);
> + overflow_handler_test_nolock,
> + ctx);
> +
> if (IS_ERR(event))
> - kunit_skip(test, "Failed to create perf event");
> - ctx.event = event;
> - perf_event_enable(ctx.event);
> - for (i = 0; i < NR_ITERATIONS; i++) {
> - for (j = 0; j < NR_OBJECTS; j++) {
> - gfp_t gfp = (i % 2) ? GFP_KERNEL : GFP_KERNEL_ACCOUNT;
> + return false;
>
> - objects[j] = kmalloc(64, gfp);
> - if (!objects[j]) {
> - j--;
> - while (j >= 0)
> - kfree(objects[j--]);
> - alloc_fail = true;
> - goto cleanup;
> - }
> - }
> - for (j = 0; j < NR_OBJECTS; j++)
> - kfree(objects[j]);
> - }
> + ctx->event = event;
> + perf_event_enable(ctx->event);
> + return true;
> +}
>
> -cleanup:
> - perf_event_disable(ctx.event);
> - perf_event_release_kernel(ctx.event);
> +static void disable_perf_events(struct test_nolock_context *ctx)
> +{
> + kunit_info(ctx->test, "HW perf events: callback_count: %d, alloc_ok: %d,
> alloc_fail: %d\n",
> + ctx->callback_count, ctx->alloc_ok, ctx->alloc_fail);
> +
> + perf_event_disable(ctx->event);
> + perf_event_release_kernel(ctx->event);
> +}
> +
> +static void test_kmalloc_kfree_nolock_perf(struct kunit *test)
> +{
> + struct test_nolock_context ctx = { .test = test };
>
> - kunit_info(test, "callback_count: %d, alloc_ok: %d, alloc_fail: %d\n",
> - ctx.callback_count, ctx.alloc_ok, ctx.alloc_fail);
> + if (!enable_perf_events(&ctx))
> + kunit_skip(test, "Failed to enable perf event, skipping");
>
> - if (alloc_fail)
> - kunit_skip(test, "Allocation failed");
> + test_kmalloc_kfree();
> +
> + disable_perf_events(&ctx);
> + KUNIT_EXPECT_EQ(test, 0, slab_errors);
> +}
> +#endif
> +
Maybe it would be worth checking that the perf event actually fired:
KUNIT_EXPECT_GT(test, ctx.callback_count, 0);
Otherwise, the test could pass without the callback.
> +#if defined(CONFIG_KPROBES) && defined(CONFIG_SMP)
> +static int slab_kprobe_pre_handler(struct kprobe *p, struct pt_regs *regs)
> +{
> + struct test_nolock_context *ctx;
> +
> + ctx = container_of(p, struct test_nolock_context, kprobe);
> + test_nolock(ctx);
> + return 0;
> +}
> +
> +static bool register_slab_kprobes(struct test_nolock_context *ctx)
> +{
> + ctx->kprobe.symbol_name = "slab_attach_kprobe_locked";
> + ctx->kprobe.pre_handler = slab_kprobe_pre_handler;
> +
> + if (register_kprobe(&ctx->kprobe))
> + return false;
> + return true;
> +}
> +
> +static void unregister_slab_kprobes(struct test_nolock_context *ctx)
> +{
> + kunit_info(ctx->test, "kprobes: callback_count: %d, alloc_ok: %d,
> alloc_fail: %d\n",
> + ctx->callback_count, ctx->alloc_ok, ctx->alloc_fail);
> + unregister_kprobe(&ctx->kprobe);
> +}
> +
> +static void test_kmalloc_kfree_nolock_kprobe(struct kunit *test)
> +{
> + struct test_nolock_context ctx = { .test = test };
> +
> + if (!register_slab_kprobes(&ctx))
> + kunit_skip(test, "Failed to register kprobe, skipping");
> +
> + test_kmalloc_kfree();
> +
> + unregister_slab_kprobes(&ctx);
> KUNIT_EXPECT_EQ(test, 0, slab_errors);
> }
Same here. Maybe it would be useful to check that the kprobe handler was
invoked at least once:
KUNIT_EXPECT_GT(test, ctx.callback_count, 0);
--
With Best Regards,
Shengming
> #endif
> @@ -405,7 +477,10 @@ static struct kunit_case test_cases[] = {
> KUNIT_CASE(test_leak_destroy),
> KUNIT_CASE(test_krealloc_redzone_zeroing),
> #ifdef CONFIG_PERF_EVENTS
> - KUNIT_CASE_SLOW(test_kmalloc_kfree_nolock),
> + KUNIT_CASE_SLOW(test_kmalloc_kfree_nolock_perf),
> +#endif
> +#if defined(CONFIG_KPROBES) && defined(CONFIG_SMP)
> + KUNIT_CASE_SLOW(test_kmalloc_kfree_nolock_kprobe),
> #endif
> {}
> };
> diff --git a/mm/slub.c b/mm/slub.c
> index da9efb040040..e32a68677537 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -908,6 +908,24 @@ static inline unsigned int
> obj_exts_offset_in_object(struct kmem_cache *s)
> }
> #endif
>
> +/*
> + * A no-op function used to attach kprobe handlers in slub_kunit tests.
> + * The barrier is needed to prevent the compiler from optimizing out
> callsites.
> + */
> +#if defined(CONFIG_DEBUG_VM) || defined(CONFIG_PROVE_LOCKING)
> +static noinline void slab_attach_kprobe_locked(void)
> +{
> + barrier();
> +}
> +#else
> +static inline void slab_attach_kprobe_locked(void) { }
> +#endif
> +
> +#define slab_lockdep_assert_held(lock) do { \
> + lockdep_assert_held(lock); \
> + slab_attach_kprobe_locked(); \
> +} while (0)
> +
> #ifdef CONFIG_SLUB_DEBUG
>
> /*
> @@ -1665,7 +1683,7 @@ static void add_full(struct kmem_cache *s,
> if (!(s->flags & SLAB_STORE_USER))
> return;
>
> - lockdep_assert_held(&n->list_lock);
> + slab_lockdep_assert_held(&n->list_lock);
> list_add(&slab->slab_list, &n->full);
> }
>
> @@ -1674,7 +1692,7 @@ static void remove_full(struct kmem_cache *s, struct
> kmem_cache_node *n, struct
> if (!(s->flags & SLAB_STORE_USER))
> return;
>
> - lockdep_assert_held(&n->list_lock);
> + slab_lockdep_assert_held(&n->list_lock);
> list_del(&slab->slab_list);
> }
>
> @@ -2868,7 +2886,7 @@ static unsigned int __sheaf_flush_main_batch(struct
> kmem_cache *s)
> void *objects[PCS_BATCH_MAX];
> struct slab_sheaf *sheaf;
>
> - lockdep_assert_held(this_cpu_ptr(&s->cpu_sheaves->lock));
> + slab_lockdep_assert_held(this_cpu_ptr(&s->cpu_sheaves->lock));
>
> pcs = this_cpu_ptr(s->cpu_sheaves);
> sheaf = pcs->main;
> @@ -3547,7 +3565,7 @@ __add_partial(struct kmem_cache_node *n, struct slab
> *slab, enum add_mode mode)
> static inline void add_partial(struct kmem_cache_node *n,
> struct slab *slab, enum add_mode mode)
> {
> - lockdep_assert_held(&n->list_lock);
> + slab_lockdep_assert_held(&n->list_lock);
> __add_partial(n, slab, mode);
> }
>
> @@ -3561,7 +3579,7 @@ static inline void clear_node_partial_state(struct
> kmem_cache_node *n,
> static inline void remove_partial(struct kmem_cache_node *n,
> struct slab *slab)
> {
> - lockdep_assert_held(&n->list_lock);
> + slab_lockdep_assert_held(&n->list_lock);
> list_del(&slab->slab_list);
> clear_node_partial_state(n, slab);
> }
> @@ -3577,7 +3595,7 @@ static void *alloc_single_from_partial(struct
> kmem_cache *s,
> {
> void *object;
>
> - lockdep_assert_held(&n->list_lock);
> + slab_lockdep_assert_held(&n->list_lock);
>
> #ifdef CONFIG_SLUB_DEBUG
> if (s->flags & SLAB_CONSISTENCY_CHECKS) {
> @@ -4642,7 +4660,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct
> slub_percpu_sheaves *pcs,
> struct node_barn *barn;
> bool allow_spin;
>
> - lockdep_assert_held(this_cpu_ptr(&s->cpu_sheaves->lock));
> + slab_lockdep_assert_held(this_cpu_ptr(&s->cpu_sheaves->lock));
>
> /* Bootstrap or debug cache, back off */
> if (unlikely(!cache_has_sheaves(s))) {
> @@ -5765,7 +5783,7 @@ static void __pcs_install_empty_sheaf(struct kmem_cache
> *s,
> struct slub_percpu_sheaves *pcs, struct slab_sheaf *empty,
> struct node_barn *barn)
> {
> - lockdep_assert_held(this_cpu_ptr(&s->cpu_sheaves->lock));
> + slab_lockdep_assert_held(this_cpu_ptr(&s->cpu_sheaves->lock));
>
> /* This is what we expect to find if nobody interrupted us. */
> if (likely(!pcs->spare)) {
> @@ -5816,7 +5834,7 @@ __pcs_replace_full_main(struct kmem_cache *s, struct
> slub_percpu_sheaves *pcs,
> bool put_fail;
>
> restart:
> - lockdep_assert_held(this_cpu_ptr(&s->cpu_sheaves->lock));
> + slab_lockdep_assert_held(this_cpu_ptr(&s->cpu_sheaves->lock));
>
> /* Bootstrap or debug cache, back off */
> if (unlikely(!cache_has_sheaves(s))) {
>
> --
> 2.53.0
