Re: [PATCH bpf-next v4 3/3] bpf, arm64: use bpf_jit_binary_pack_alloc

[Daniel Borkmann <daniel@iogearbox.net>]

On 8/15/23 4:09 PM, Puranjay Mohan wrote:
> Hi Everyone,
> 
> [+cc Björn]
> 
> On Mon, Jun 26, 2023 at 10:58 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>>
>> Use bpf_jit_binary_pack_alloc for memory management of JIT binaries in
>> ARM64 BPF JIT. The bpf_jit_binary_pack_alloc creates a pair of RW and RX
>> buffers. The JIT writes the program into the RW buffer. When the JIT is
>> done, the program is copied to the final RX buffer
>> with bpf_jit_binary_pack_finalize.
>>
>> Implement bpf_arch_text_copy() and bpf_arch_text_invalidate() for ARM64
>> JIT as these functions are required by bpf_jit_binary_pack allocator.
>>
>> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
>> Acked-by: Song Liu <song@kernel.org>
> 
> [...]
> 
>> +int bpf_arch_text_invalidate(void *dst, size_t len)
>> +{
>> +       __le32 *ptr;
>> +       int ret;
>> +
>> +       for (ptr = dst; len >= sizeof(u32); len -= sizeof(u32)) {
>> +               ret = aarch64_insn_patch_text_nosync(ptr++, AARCH64_BREAK_FAULT);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
> 
> While testing the same patch for riscv bpf jit, Björn found that
> ./test_tag is taking a lot of
> time to complete. He found that bpf_arch_text_invalidate() is calling
> patch_text_nosync() in RISCV
> and aarch64_insn_patch_text_nosync() here in ARM64. Both of the
> implementations call these functions
> in a loop for each word. The problem is that every call to
> patch_text_nosync()/aarch64_insn_patch_text_nosync()
> would clean the cache. This will make this
> function(bpf_arch_text_invalidate) really slow.
> 
> I did some testing using the vmtest.sh script on ARM64 with and
> without the patches, here are the results:
> 
> With Prog Pack patches applied:
> =========================
> 
> root@(none):/# time ./root/bpf/test_tag
> test_tag: OK (40945 tests)
> 
> real    3m2.001s
> user    0m1.644s
> sys     2m40.132s
> 
> Without Prog Pack:
> ===============
> 
> root@(none):/# time ./root/bpf/test_tag
> test_tag: OK (40945 tests)
> 
> real    0m26.809s
> user    0m1.591s
> sys     0m24.106s
> 
> As you can see the current implementation of
> bpf_arch_text_invalidate() is really slow. I need to
> implement a new function: aarch64_insn_set_text_nosync() and use it in
> bpf_arch_text_invalidate()
> rather than calling aarch64_insn_patch_text_nosync() in a loop.

Ok, thanks for looking into this. I'm going to toss the current version from
patchwork in that case and will wait for a v5 from your side.

> In the longer run, it would be really helpful if we have a standard
> text_poke API like x86 in every architecture.

Likely independent of this series, but in case you see potential to consolidate
generic pieces, it might be worth a look after the arm64/riscv bits landed.

Thanks for working on this, Puranjay!