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

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)

> 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...)

-- 
Cheers,
Harry / Hyeonggon

-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQQQ1ub6gR5ogjaKRmOGXBN6rc5S1gUCamBuMAAKCRCGXBN6rc5S
1sPeAQDxKa0DMrDQTezN00ksgEUR3eb05xC3gVwrShymZlkh5wEA7tBOlrr/Ntis
S3x7DXHLG0TmEitCZ5rUhYKcT3diOQI=
=g7jN
-----END PGP SIGNATURE-----