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
OpenPGP_signature.asc
Description: OpenPGP digital signature
