lkml.org 
[lkml]   [2022]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH bpf-next v2 2/2] riscv, bpf: Support riscv jit to provide bpf_line_info
From
Date
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
> .
>

\
 
 \ /
  Last update: 2022-09-17 16:24    [W:0.054 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site