Re: [PATCH v6 bpf-next 6/7] bpf: introduce bpf_prog_pack allocator

[Song Liu]

> On Jan 21, 2022, at 6:12 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> 
> On Fri, Jan 21, 2022 at 5:30 PM Song Liu <songliubraving@xxxxxx> wrote:
>> 
>> 
>> 
>>> On Jan 21, 2022, at 5:12 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
>>> 
>>> On Fri, Jan 21, 2022 at 5:01 PM Song Liu <songliubraving@xxxxxx> wrote:
>>>> 
>>>> In this way, we need to allocate rw_image here, and free it in
>>>> bpf_jit_comp.c. This feels a little weird to me, but I guess that
>>>> is still the cleanest solution for now.
>>> 
>>> You mean inside bpf_jit_binary_alloc?
>>> That won't be arch independent.
>>> It needs to be split into generic piece that stays in core.c
>>> and callbacks like bpf_jit_fill_hole_t
>>> or into multiple helpers with prep in-between.
>>> Don't worry if all archs need to be touched.
>> 
>> How about we introduce callback bpf_jit_set_header_size_t? Then we
>> can split x86's jit_fill_hole() into two functions, one to fill the
>> hole, the other to set size. The rest of the logic gonna stay the same.
>> 
>> Archs that do not use bpf_prog_pack won't need bpf_jit_set_header_size_t.
> 
> That's not any better.
> 
> Currently the choice of bpf_jit_binary_alloc_pack vs bpf_jit_binary_alloc
> leaks into arch bits and bpf_prog_pack_max_size() doesn't
> really make it generic.
> 
> Ideally all archs continue to use bpf_jit_binary_alloc()
> and magic happens in a generic code.
> If not then please remove bpf_prog_pack_max_size(),
> since it doesn't provide much value and pick
> bpf_jit_binary_alloc_pack() signature to fit x86 jit better.
> It wouldn't need bpf_jit_fill_hole_t callback at all.
> Please think it through so we don't need to redesign it
> when another arch will decide to use huge pages for bpf progs.
> 
> cc-ing Ilya for ideas on how that would fit s390.

I guess we have a few different questions here:

1. Can we use bpf_jit_binary_alloc() for both regular page and shared 
huge page? 

I think the answer is no, as bpf_jit_binary_alloc() allocates a rw 
buffer, and arch calls bpf_jit_binary_lock_ro after JITing. The new 
allocator will return a slice of a shared huge page, which is locked
RO before JITing. 

2. The problem with bpf_prog_pack_max_size() limitation. 

I think this is the worst part of current version of bpf_prog_pack, 
but it shouldn't be too hard to fix. I will remove this limitation 
in the next version. 

3. How to set proper header->size? 

I guess we can introduce something similar to bpf_arch_text_poke() 
for this? 


My proposal for the next version is:
1. No changes to archs that do not use huge page, just keep using 
   bpf_jit_binary_alloc.

2. For x86_64 (and other arch that would support bpf program on huge
   pages):
   2.1 arch/bpf_jit_comp calls bpf_jit_binary_alloc_pack() to allocate
       an RO bpf_binary_header;
   2.2 arch allocates a temporary buffer for JIT. Once JIT is done, 
       use text_poke_copy to copy the code to the RO bpf_binary_header. 

3. Remove bpf_prog_pack_max_size limitation. 


Does this sound reasonable?

Thanks,
Song