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

[Song Liu <songliubraving@fb.com>]

> On Jan 21, 2022, at 4:41 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Fri, Jan 21, 2022 at 4:23 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> 
>> 
>>> On Jan 21, 2022, at 3:55 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>> 
>>> On Fri, Jan 21, 2022 at 11:49 AM Song Liu <song@kernel.org> wrote:
>>>> 
>>>> +static struct bpf_binary_header *
>>>> +__bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
>>>> +                      unsigned int alignment,
>>>> +                      bpf_jit_fill_hole_t bpf_fill_ill_insns,
>>>> +                      u32 round_up_to)
>>>> +{
>>>> +       struct bpf_binary_header *hdr;
>>>> +       u32 size, hole, start;
>>>> +
>>>> +       WARN_ON_ONCE(!is_power_of_2(alignment) ||
>>>> +                    alignment > BPF_IMAGE_ALIGNMENT);
>>>> +
>>>> +       /* Most of BPF filters are really small, but if some of them
>>>> +        * fill a page, allow at least 128 extra bytes to insert a
>>>> +        * random section of illegal instructions.
>>>> +        */
>>>> +       size = round_up(proglen + sizeof(*hdr) + 128, round_up_to);
>>>> +
>>>> +       if (bpf_jit_charge_modmem(size))
>>>> +               return NULL;
>>>> +       hdr = bpf_jit_alloc_exec(size);
>>>> +       if (!hdr) {
>>>> +               bpf_jit_uncharge_modmem(size);
>>>> +               return NULL;
>>>> +       }
>>>> +
>>>> +       /* Fill space with illegal/arch-dep instructions. */
>>>> +       bpf_fill_ill_insns(hdr, size);
>>>> +
>>>> +       hdr->size = size;
>>>> +       hole = min_t(unsigned int, size - (proglen + sizeof(*hdr)),
>>>> +                    PAGE_SIZE - sizeof(*hdr));
>>> 
>>> It probably should be 'round_up_to' instead of PAGE_SIZE ?
>> 
>> Actually, some of these change is not longer needed after the following
>> change in v6:
>> 
>>  4. Change fall back round_up_to in bpf_jit_binary_alloc_pack() from
>>     BPF_PROG_MAX_PACK_PROG_SIZE to PAGE_SIZE.
>> 
>> My initial thought (last year) was if we allocate more than 2MB (either
>> 2.1MB or 3.9MB), we round up to 4MB to save page table entries.
>> However, when I revisited this earlier today, I thought we should still
>> round up to PAGE_SIZE to save memory
>> 
>> Right now, I am not sure which way is better. What do you think? If we
>> round up to PAGE_SIZE, we don't need split out __bpf_jit_binary_alloc().
> 
> The less code duplication the better.

Got it. Will go with PAGE_SIZE. 

[...]

>>>> +
>>>>       if (bpf_jit_charge_modmem(size))
>>>>               return NULL;
>>>> -       hdr = bpf_jit_alloc_exec(size);
>>>> +       hdr = bpf_prog_pack_alloc(size);
>>>>       if (!hdr) {
>>>>               bpf_jit_uncharge_modmem(size);
>>>>               return NULL;
>>>> @@ -888,9 +1052,8 @@ bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
>>>>       /* Fill space with illegal/arch-dep instructions. */
>>>>       bpf_fill_ill_insns(hdr, size);
>>>> 
>>>> -       hdr->size = size;
>>> 
>>> I'm missing where it's assigned.
>>> Looks like hdr->size stays zero, so free is never performed?
>> 
>> This is read only memory, so we set it in bpf_fill_ill_insns(). There was a
>> comment in x86/bpf_jit_comp.c. I guess we also need a comment here.
> 
> Ahh. Found it. Pls don't do it in fill_insn.
> It's the wrong layering.
> It feels that callbacks need to be redesigned.
> I would operate on rw_header here and use
> existing arch specific callback fill_insn to write into rw_image.
> Both insns during JITing and 0xcc on both sides of the prog.
> Populate rw_header->size (either before or after JITing)
> and then do single text_poke_copy to write the whole thing
> into the correct spot.

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. 

Thanks,
Song