From: "Björn Töpel" <bjorn.topel@gmail.com>
To: Luke Nelson <lukenels@cs.washington.edu>
Cc: bpf <bpf@vger.kernel.org>, Luke Nelson <luke.r.nels@gmail.com>,
Jonathan Corbet <corbet@lwn.net>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Albert Ou <aou@eecs.berkeley.edu>, Xi Wang <xi.wang@gmail.com>,
Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
Stephen Hemminger <stephen@networkplumber.org>,
Rob Herring <robh@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jonathan Cameron <Jonathan.Cameron@huawei.com>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
linux-doc@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
Netdev <netdev@vger.kernel.org>,
linux-riscv@lists.infradead.org
Subject: Re: [PATCH bpf-next v4 2/4] riscv, bpf: add RV32G eBPF JIT
Date: Wed, 4 Mar 2020 06:59:17 +0100 [thread overview]
Message-ID: <CAJ+HfNhOp_Rbcqer0K=mZ8h+uswYSv4hSa3wCTdjjxH26HUTCw@mail.gmail.com> (raw)
In-Reply-To: <CADasFoBODSbgHHXU+iA-32=oKNs6n0Ff_UDU3063uiyGjx1xXg@mail.gmail.com>
On Wed, 4 Mar 2020 at 03:32, Luke Nelson <lukenels@cs.washington.edu> wrote:
>
[...]
>
> > > + case BPF_LSH:
> > > + if (imm >= 32) {
> > > + emit(rv_slli(hi(rd), lo(rd), imm - 32), ctx);
> > > + emit(rv_addi(lo(rd), RV_REG_ZERO, 0), ctx);
> > > + } else if (imm == 0) {
> > > + /* nop */
> >
> > Can we get rid of this, and just do if/else if?
>
> imm == 0 has been a tricky case for 32-bit JITs; see 6fa632e719ee
> ("bpf, x32: Fix bug with ALU64 {LSH, RSH, ARSH} BPF_K shift by 0").
> We wanted to make the imm == 0 case explicit and help future readers
> see that this case is handled correctly here.
>
> We could do the following if we really wanted to get rid of the
> check:
>
> if (imm >= 32) {
> ...
> } else if (imm != 0) {
> ...
> }
> /* Do nothing for imm == 0. */
>
> Though it's unclear if this is easier to read.
>
Thanks for clearing that up. *I* prefer the latter, but that's really
up to you! Keep the current one, if you prefer that! :-)
> > > + case BPF_ARSH:
> > > + if (is_12b_int(imm)) {
> > > + emit(rv_srai(lo(rd), lo(rd), imm), ctx);
> > > + } else {
> > > + emit_imm(RV_REG_T0, imm, ctx);
> > > + emit(rv_sra(lo(rd), lo(rd), RV_REG_T0), ctx);
> > > + }
> > > + break;
> >
> > Again nit; I like "early exit" code if possible. Instead of:
> >
> > if (bleh) {
> > foo();
> > } else {
> > bar();
> > }
> >
> > do:
> >
> > if (bleh) {
> > foo()
> > return/break;
> > }
> > bar();
> >
> > I find the latter easier to read -- but really a nit, and a matter of
> > style. There are number of places where that could be applied in the
> > file.
>
> I like "early exit" code, too, and agree that it's easier to read
> in general, especially when handling error conditions.
>
> But here we wanted to make it explicit that both branches are
> emitting equivalent instruction sequences (under different paths).
> Structured control flow seems a better fit for this particular
> context.
>
Ok!
> > At this point of the series, let's introduce the shared code .c-file
> > containing implementation for bpf_int_jit_compile() (with build_body
> > part of that)and bpf_jit_needs_zext(). That will make it easier to
> > catch bugs in both JITs and to avoid code duplication! Also, when
> > adding the stronger invariant suggested by Palmer [1], we only need to
> > do it in one place.
> >
> > The pull out refactoring can be a separate commit.
>
> I think the idea of deduplicating bpf_int_jit_compile is good and
> will lead to more maintainable JITs. How does the following proposal
> for v5 sound?
>
> In patch 1 of this series:
>
> - Factor structs and common helpers to bpf_jit.h (just like v4).
>
> - Factor out bpf_int_jit_compile(), bpf_jit_needs_zext(), and
> build_body() to a new file bpf_jit_core.c and tweak the code as in v4.
>
> - Rename emit_insn() and build_{prologue,epilogue}() to bpf_jit_emit_insn()
> and bpf_jit_build_{prologue,epilogue}, since these functions are
> now extern rather than static.
>
> - Rename bpf_jit_comp.c to bpf_jit_comp64.c to be more explicit
> about its contents (as the next patch will add bpf_jit_comp32.c).
>
> Then patch 2 can reuse the new header and won't need to define its
> own bpf_int_jit_compile() etc.
>
I like that, but keep the first patch as a refactoring patch only, and
then in a *new* patch 2 you add the rv32 specific code (sltu and
pseudo instructions + the xlen preprocessor check + copyright-things
;-)). Patch 3 will be the old patch 2. Wdyt?
Thanks for working on this!
Björn
> Thanks!
>
> Luke
next prev parent reply other threads:[~2020-03-04 5:59 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-03 0:50 [PATCH bpf-next v4 0/4] eBPF JIT for RV32G Luke Nelson
2020-03-03 0:50 ` [PATCH bpf-next v4 1/4] riscv, bpf: move common riscv JIT code to header Luke Nelson
2020-03-03 7:50 ` Björn Töpel
2020-03-04 2:31 ` Luke Nelson
2020-03-04 5:44 ` Björn Töpel
2020-03-03 0:50 ` [PATCH bpf-next v4 2/4] riscv, bpf: add RV32G eBPF JIT Luke Nelson
2020-03-03 7:48 ` Björn Töpel
2020-03-04 2:32 ` Luke Nelson
2020-03-04 5:59 ` Björn Töpel [this message]
2020-03-04 7:24 ` Luke Nelson
2020-03-04 7:31 ` Björn Töpel
2020-03-03 0:50 ` [PATCH bpf-next v4 3/4] bpf, doc: add BPF JIT for RV32G to BPF documentation Luke Nelson
2020-03-03 7:27 ` Björn Töpel
2020-03-03 0:50 ` [PATCH bpf-next v4 4/4] MAINTAINERS: Add entry for RV32G BPF JIT Luke Nelson
2020-03-03 5:51 ` Björn Töpel
2020-03-03 10:02 ` Andy Shevchenko
2020-03-04 2:33 ` Luke Nelson
2020-03-04 10:21 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJ+HfNhOp_Rbcqer0K=mZ8h+uswYSv4hSa3wCTdjjxH26HUTCw@mail.gmail.com' \
--to=bjorn.topel@gmail.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=andriin@fb.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=aou@eecs.berkeley.edu \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=corbet@lwn.net \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=kafai@fb.com \
--cc=kuba@kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=luke.r.nels@gmail.com \
--cc=lukenels@cs.washington.edu \
--cc=mchehab+samsung@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=robh@kernel.org \
--cc=songliubraving@fb.com \
--cc=stephen@networkplumber.org \
--cc=xi.wang@gmail.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).