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
.
