Messages in this thread | | | Subject | Re: [PATCH bpf-next v2 2/2] riscv, bpf: Support riscv jit to provide bpf_line_info | From | Pu Lehui <> | Date | Thu, 26 May 2022 21:15:44 +0800 |
| |
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 > . >
| |