Re: [PATCH bpf-next 5/5] selftests/bpf: add kfunc_call test for simple dtor in bpf_testmod

[Alan Maguire <alan.maguire@oracle.com>]

On 19/06/2024 17:54, Eduard Zingerman wrote:
> On Wed, 2024-06-19 at 17:45 +0100, Alan Maguire wrote:
> 
> [...]
> 
>> oops, missed a GFP_ATOMIC here to avoid possible sleeping. To use
>> existing kfunc call test structure it's simpler to do this than add a
>> sleepable test context I think, especially since the focus here is on
>> adding a basic test. More below..
> 
> Hi Alan,
> 
> And I agree, GFP_ATOMIC should probably help and it is simpler than
> adding a sleepable context.
> 
> [...]
> 
>> Yeah, my focus here was testing the registration to be honest and
>> thankfully as you noted it caught a case where I had forgotten to do id
>> relocation, so thanks for suggesting this!
>>
>> To trigger the dtor cleanup via a map, I came up with the following:
>>
>> - call bpf_testmod_ctx_create()
>> - do bpf_kptr_xchg(&ctx_val->ctx, ctx) to transfer the ctx kptr into the
>> map value;
>> - only release the reference if the kptr exchange fails
>> - and then it gets cleaned up on exit.
>>
>> I haven't used kptrs much so hopefully that's right.
>>
>> Tracing I confirmed cleanup happens via:
>>
>> $ sudo dtrace -n 'fbt::bpf_testmod_ctx_release:entry { stack(); }'
>> dtrace: description 'fbt::bpf_testmod_ctx_release:entry ' matched 1 probe
>> CPU     ID                    FUNCTION:NAME
>>   3 113779    bpf_testmod_ctx_release:entry
>>               vmlinux`array_map_free+0x69
>>               vmlinux`bpf_map_free_deferred+0x62
>>               vmlinux`process_one_work+0x192
>>               vmlinux`worker_thread+0x27a
>>               vmlinux`kthread+0xf7
>>               vmlinux`ret_from_fork+0x41
>>               vmlinux`ret_from_fork_asm+0x1a
>>
>> Does the above sound right? Thanks!
> 
> It does, might as well set some flag in the dtor kfunc and check it in
> the program (using another map?). Tbh, I thought we could get away w/o
> 

Sorry, I'm not following here. So I think what you'd like is a way to
verify that the dtor actually runs, is that right? The problem there is
that the map cleanup gets run when the skeleton gets destroyed, but then
it's too late then to collect a count value via that BPF object.

The only thing I can think of is to create an additional tracing object
that we separately load/attach to bpf_testmod_ctx_release() prior to
running kfunc call tests to verify that the destructor fires on cleanup
of the kfunc test skeletons. Is that what you have in mind? Thanks!

Alan