Message-ID: <CAB-e3NRn9VgdWfakom6Cbx-3btakEzvpNVmiQw7k-h_-EtOMng@mail.gmail.com>
Date:   Fri, 6 May 2022 14:44:34 -0700
From:   Luke Nelson <luke.r.nels@...il.com>
To:     Pu Lehui <pulehui@...wei.com>
Cc:     bpf <bpf@...r.kernel.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Networking <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Björn Töpel <bjorn@...nel.org>,
        Xi Wang <xi.wang@...il.com>, Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>
Subject: Re: [PATCH bpf-next v2 2/2] riscv, bpf: Support riscv jit to provide bpf_line_info

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

Powered by blists - more mailing lists