Re: [PATCH slab/for-next v4 1/8] mm/slab, slub_kunit: register kprobe to trigger _nolock APIs

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