Re: [PATCH bpf-next v2 2/3] bpf: struct_ops supports more than one page for trampolines.

[Martin KaFai Lau <martin.lau@linux.dev>]

On 2/23/24 9:36 AM, Kui-Feng Lee wrote:
> 
> To be clear, we are not talking computation or memory complexity here.
> I consider the complexity in another way. When I look at the code of
> bpf_dummy_ops, and see it free the memory at the very end of a function.
> I have to guess who allocate the memory by looking around without a
> clear sign or hint if we move the allocation to
> bpf_struct_ops_prepare_trampoline(). It is a source of complexity.

It still sounds like a naming perception issue more than a practical code-wise 
complexity/readability. Rename it to 
bpf_struct_ops_"s/alloc/prepare/"_trampoline() if it can make it more obvious 
that it is an alloc function. imo, that function returning a page is a clear 
sign that it can alloc but I don't mind renaming it if it can help to make it 
sounds more like alloc and free pair.

> Very often, a duplication is much more simple and easy to understand.
> Especially, when the duplication is in a very well know/recognized
> pattern. Here will create a unusual way to replace a well recognized one
> to simplify the code.

Sorry, I don't agree on this where this patch is duplicating lines of code which 
is not obvious like setting BPF_TRAMP_F_*. At least I often have to go back to 
arch_prepare_bpf_trampoline() to understand how it works.

Not copy-and-pasting this piece of codes everywhere is more important than 
making bpf_dummy_ops looks better.

> 
> My reason of duplicating the code from
> bpf_struct_ops_prepare_trampoline() was we don't need
> bpf_struct_ops_prepare_trampoline() in future if we were going to move
> bpf_dummy_ops out. But, just like you said, we still have bpf_dummy_ops

Yep, it will be great to move bpf_dummy_ops out but how it can be done and 
whether it can remove its bpf_struct_ops_prepare_trampoline() usage is still 
TBD. I think it should be possible. Even it is moved out in the future, 
bpf_struct_ops_(prepare|alloc)_trampoline() can be keep as is.

> now, so it is a good trade of to move memory allocation into
> bpf_struct_ops_prepare_trampoline() to avoid the duplication the code
> about flags and tlinks. But, the trade off we are talking here goes to
> an opposite way.