Re: [PATCH bpf-next v2 2/2] riscv, bpf: Support riscv jit to provide bpf_line_info

[Pu Lehui]

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
> .