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

[Eduard Zingerman <eddyz87@gmail.com>]

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
complicating the test, but since you already have it working, we should
probably add it.

Thanks,
Eduard