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 11:05 AM, Kui-Feng Lee wrote:
> 
> 
> 
> 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

I don't think I ever mentioned to do book keeping in 
bpf_struct_ops_map_update_elem(). Have you looked at my earlier code in 
bpf_struct_ops_prepare_trampoline() which also does the memory charging also?

> 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.

Which part make no sense? Having bpf_dummy_ops charge/uncharge memory also?

The bpf_dummy_ops() uses the below bpf_struct_ops_free_trampoline() which does 
uncharge and free. bpf_struct_ops_prepare_trampoline() does charge and alloc.
charge/alloc matches with uncharge/free.

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