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.
