> 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
