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 11:15, Martin KaFai Lau wrote:
> 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?

The bookkeeping that I am saying is about maintaining image_pages and
image_pages_cnt.

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

Simplifying bpf_dummy_ops by removing the duty of allocation but adding
the duty of uncharge memory doesn't make sense to me in terms of
simplification. Although the lines of code would be similar, it actually
makes it more complicated than before.

> 
> 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);
>>>>>> }
>>>>>>
>>>
>