Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <adec88f9-b3e6-bfe4-c09e-54825a60f45d@linux.ibm.com>
Date:   Mon, 24 Jan 2022 13:29:23 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v6 bpf-next 6/7] bpf: introduce bpf_prog_pack allocator
Content-Language: en-US
To:     Song Liu <songliubraving@fb.com>,
        Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     Song Liu <song@kernel.org>, bpf <bpf@vger.kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Kernel Team <Kernel-team@fb.com>,
        Peter Zijlstra <peterz@infradead.org>, X86 ML <x86@kernel.org>
References: <20220121194926.1970172-1-song@kernel.org>
 <20220121194926.1970172-7-song@kernel.org>
 <CAADnVQK6+gWTUDo2z1H6AE5_DtuBBetW+VTwwKz03tpVdfuoHA@mail.gmail.com>
 <7393B983-3295-4B14-9528-B7BD04A82709@fb.com>
 <CAADnVQJLHXaU7tUJN=EM-Nt28xtu4vw9+Ox_uQsjh-E-4VNKoA@mail.gmail.com>
 <5407DA0E-C0F8-4DA9-B407-3DE657301BB2@fb.com>
 <CAADnVQLOpgGG9qfR4EAgzrdMrfSg9ftCY=9psR46GeBWP7aDvQ@mail.gmail.com>
 <5F4DEFB2-5F5A-4703-B5E5-BBCE05CD3651@fb.com>
 <CAADnVQLXGu_eF8VT6NmxKVxOHmfx7C=mWmmWF8KmsjFXg6P5OA@mail.gmail.com>
 <5E70BF53-E3FB-4F7A-B55D-199C54A8FDCA@fb.com>
From:   Ilya Leoshkevich <iii@linux.ibm.com>
In-Reply-To: <5E70BF53-E3FB-4F7A-B55D-199C54A8FDCA@fb.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/23/22 02:03, Song Liu wrote:
> 
> 
>> On Jan 21, 2022, at 6:12 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>
>> On Fri, Jan 21, 2022 at 5:30 PM Song Liu <songliubraving@fb.com> wrote:
>>>
>>>
>>>
>>>> On Jan 21, 2022, at 5:12 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>>>
>>>> On Fri, Jan 21, 2022 at 5:01 PM Song Liu <songliubraving@fb.com> 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.

Are arches expected to allocate rw buffers in different ways? If not,
I would consider putting this into the common code as well. Then
arch-specific code would do something like

   header = bpf_jit_binary_alloc_pack(size, &prg_buf, &prg_addr, ...);
   ...
   /*
    * Generate code into prg_buf, the code should assume that its first
    * byte is located at prg_addr.
    */
   ...
   bpf_jit_binary_finalize_pack(header, prg_buf);

where bpf_jit_binary_finalize_pack() would copy prg_buf to header and
free it.

If this won't work, I also don't see any big problems in the scheme
that you propose (especially if bpf_prog_pack_max_size() limitation is
gone).

[...]

Btw, are there any existing benchmarks that I can use to check whether
this is worth enabling on s390?

Best regards,
Ilya