| Message ID | 20220429014240.3434866-3-pulehui@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support riscv jit to provide | expand |
Pu Lehui wrote: > Add support for riscv jit to provide bpf_line_info. > We need to consider the prologue offset in ctx->offset, > but unlike x86 and arm64, ctx->offset of riscv does not > provide an extra slot for the prologue, so here we just > calculate the len of prologue and add it to ctx->offset > at the end. Both RV64 and RV32 have been tested. > > Signed-off-by: Pu Lehui <pulehui@huawei.com> > --- Looks reasonable to me, but would be good if someone with RISC knowledge takes a lookt hough.
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. > + 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). > ./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. Thanks! - Luke
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 > . >
diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h index 2a3715bf29fe..7dbbad7595f0 100644 --- a/arch/riscv/net/bpf_jit.h +++ b/arch/riscv/net/bpf_jit.h @@ -69,6 +69,7 @@ struct rv_jit_context { struct bpf_prog *prog; u16 *insns; /* RV insns */ int ninsns; + int prologue_offset; int epilogue_offset; int *offset; /* BPF to RV */ int nexentries; diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c index be743d700aa7..6383eb591b0d 100644 --- a/arch/riscv/net/bpf_jit_core.c +++ b/arch/riscv/net/bpf_jit_core.c @@ -44,7 +44,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) unsigned int prog_size = 0, extable_size = 0; bool tmp_blinded = false, extra_pass = false; struct bpf_prog *tmp, *orig_prog = prog; - int pass = 0, prev_ninsns = 0, i; + int pass = 0, prev_ninsns = 0, prologue_len, i; struct rv_jit_data *jit_data; struct rv_jit_context *ctx; @@ -95,6 +95,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) prog = orig_prog; goto out_offset; } + ctx->prologue_offset = ctx->ninsns; bpf_jit_build_prologue(ctx); ctx->epilogue_offset = ctx->ninsns; bpf_jit_build_epilogue(ctx); @@ -161,6 +162,10 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) if (!prog->is_func || extra_pass) { bpf_jit_binary_lock_ro(jit_data->header); + 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]); + bpf_prog_fill_jited_linfo(prog, ctx->offset); out_offset: kfree(ctx->offset); kfree(jit_data);
Add support for riscv jit to provide bpf_line_info. We need to consider the prologue offset in ctx->offset, but unlike x86 and arm64, ctx->offset of riscv does not provide an extra slot for the prologue, so here we just calculate the len of prologue and add it to ctx->offset at the end. Both RV64 and RV32 have been tested. Signed-off-by: Pu Lehui <pulehui@huawei.com> --- arch/riscv/net/bpf_jit.h | 1 + arch/riscv/net/bpf_jit_core.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)