Date:   Thu, 26 May 2022 21:15:44 +0800
From:   Pu Lehui <pulehui@...wei.com>
To:     Luke Nelson <luke.r.nels@...il.com>
CC:     bpf <bpf@...r.kernel.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Networking <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        "Daniel Borkmann" <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Björn Töpel <bjorn@...nel.org>,
        Xi Wang <xi.wang@...il.com>, Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>
Subject: Re: [PATCH bpf-next v2 2/2] riscv, bpf: Support riscv jit to provide
 bpf_line_info

Hi Luke,

On 2022/5/7 5:44, Luke Nelson wrote:
> Thanks for the patch! I have a couple of notes written down below.
> 
>> +               ctx->prologue_offset = ctx->ninsns;
>> ...
>> +               prologue_len = ctx->epilogue_offset - ctx->prologue_offset;
>> +               for (i = 0; i < prog->len; i++)
>> +                       ctx->offset[i] = ninsns_rvoff(prologue_len + ctx->offset[i]);
> 
> The logic looks correct to me; my only nit is that the name
> prologue_offset might be a bit confusing. The prologue is always at
> the beginning of the final JITed program, it just happens to be that
> the prologue is emitted "out of order" on the initial/internal passes
> that compute offsets.
> 
> What prologue_offset really measures in your code is the length of the
> body of the JITed program. What do you think about renaming
> prologue_offset to something like body_len? Then the line to compute
> prologue_len becomes:
> 
>          prologue_len = ctx->epilogue_offset - ctx->body_len;
> 
> This version makes more sense to me why it's correct. Curious what you think.
>

Sorry for getting back to you so late. Thanks so much for your review. 
It seems that ctx->body_len makes more sence, I will rename it.

> 
>> +               bpf_prog_fill_jited_linfo(prog, ctx->offset);
> 
> Here's a quote from the comment that documents
> bpf_prog_fill_jited_linfo in kernel/bpf/core.c:
> 
> /* The jit engine is responsible to provide an array
>   * for insn_off to the jited_off mapping (insn_to_jit_off).
> ...
>   * jited_off is the byte off to the last byte of the jited insn.
> 
> This comment says that ctx->offset (passed to this function as
> insn_to_jit_off) should map each instruction to the offset of the last
> byte of the JITed instructions, but as I understand it your patch sets
> ctx->offset[i] to be the offset _one past_ the last byte of the JITed
> instructions (i.e., the first byte of the next instruction). I'm not
> sure if this is a bug in your code, in this comment, or in my
> understanding :)
> 
> As a concrete example, suppose the BPF instruction at index 0 compiles
> to 2 (non-compressed) RISC-V instructions, or 8 bytes. Then
> ctx->offset[0] will be 2 after the initial JIT passes, and your code
> would update ctx->offset[0] to be 4*prologue_len + 8. This offset
> corresponds to the first byte of insns[1], not the last byte of
> insn[0], which would be 4*prologue_len + 7.
> 
> My guess would be that the comment is out of date and your code is
> doing the correct thing, since it seems in line with what other JITs
> are doing. If that's the case, maybe we can consider updating that
> comment at some point. I'm curious if the tests you ran would break if
> you changed your code to match what the comment says (i.e.,
> subtracting 1 byte from each element in ctx->offset before passing to
> bpf_prog_fill_jited_linfo).
> 

IIUC,ctx->offset(passed to bpf_prog_fill_jited_linfo as insn_to_jit_off)
should be the first byte of the next instruction, or the byte off to the 
end of the current instruction.

Here's the code as below
bpf_prog_fill_jited_linfo in kernel/bpf/core.c:

	jited_linfo[i] = prog->bpf_func +
		insn_to_jit_off[linfo[i].insn_off - insn_start - 1];

we can see here that "linfo[i].insn_off - insn_start - 1" refers to the 
previous instruction, and the corresponding insn_to_jit_off refers to 
the first byte of the current instruction.

It seems the following quote might make more sense
bpf_prog_fill_jited_linfo in kernel/bpf/core.c:
* jited_off is the byte off to the "end" of the jited insn.

> 
>> ./test_progs -a btf
>> #19 btf:OK
>> Summary: 1/215 PASSED, 0 SKIPPED, 0 FAILED
> 
> Last, did you have a chance to run any of the other tests with your
> change (e.g., test_verifier, test_bpf.ko, other tests in test_progs)?
> I don't expect this change to break any tests, but may as well run
> them if it's easy enough just to be sure.
> 

Yeah, "test_verifier", "test_bpf.ko" and "test_progs -a btf" all test 
pass, as well as "test_progs" with no new failure ceses. I will attach 
the test result in v3.

Thanks,
Lehui

> 
> Thanks!
> - Luke
> .
> 

Powered by blists - more mailing lists