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

[Kui-Feng Lee <sinquersw@gmail.com>]

On 2/23/24 10:42, Martin KaFai Lau wrote:
> On 2/23/24 10:29 AM, Kui-Feng Lee wrote:
>> One thing I forgot to mention is that bpf_dummy_ops has to call
>> bpf_jit_uncharge_modmem(PAGE_SIZE) as well. The other option is to move
>> bpf_jit_charge_modmem() out of bpf_struct_ops_prepare_trampoline(),
>> meaning bpf_struct_ops_map_update_elem() should handle the case that the
>> allocation in bpf_struct_ops_prepare_trampoline() successes, but
>> bpf_jit_charge_modmem() fails.
> 
> Keep the charge/uncharge in bpf_struct_ops_prepare_trampoline().
> 
> It is fine to have bpf_dummy_ops charge and then uncharge a PAGE_SIZE. 
> There is no need to optimize for bpf_dummy_ops. Use 
> bpf_struct_ops_free_trampoline() in bpf_dummy_ops to uncharge and free.


Then, I don't get the point here.
I agree with moving the allocation into
bpf_struct_ops_prepare_trampoline() to avoid duplication of the code
about flags and tlinks. It really simplifies the code with the fact
that bpf_dummy_ops is still there. So, I tried to pass a st_map to
bpf_struct_ops_prepare_trampoline() to keep page managements code
together. But, you said to simplify the code of bpf_dummy_ops by
allocating pages in bpf_struct_ops_prepare_trampoline(), do bookkeeping
in bpf_struct_ops_map_update_elem(), so bpf_dummy_ops doesn't have to
allocate memory. But, we have to move a bpf_jit_uncharge_modmem() to
bpf_dummy_ops. For me, this trade-off that include removing an
allocation and adding a bpf_jit_uncharge_modmem() make no sense.

> 
> 
>>>> void bpf_struct_ops_free_trampoline(void *image)
>>>> {
>>>>      bpf_jit_uncharge_modmem(PAGE_SIZE);
>>>>      arch_free_bpf_trampoline(image, PAGE_SIZE);
>>>> }
>>>>
>