Harry wrote:
> On 7/22/26 12:27 PM, [email protected] wrote:
> > 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]>
>
> Thanks a lot for reviewing, Shengming!
>
> Just FYI, I'm going to adjust a nit pointed out by sashiko
> that kmalloc_obj(*objects[j], gfp) is wrong because objects is an array
> of void pointers. (which is fixed later in the series)
Indeed, Nice catch by sashiko. :)
> > Nit below:
> >> +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.
>
> I'm not sure if that would be useful though.
>
> Ideally we want to make sure every code path that handles
> allow_spin = false is tested, but it's tricky to measure that and
> also it's tricky to make sure those paths are properly tested
> (especially with perf events).
>
> callback_count is not a good indicator to measure test coverage.
>
> (I use GCOV to make sure we have reasonable coverage,
> but it's hard to turn that into a test case...)
Thanks for the explanation.
I was considering it only as a basic sanity check, but agreed that
callback_count does not reflect the actual path coverage. I'm fine
with leaving it as is.
--
With Best Regards,
Shengming
