From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
Jiong Wang <jiong.wang@netronome.com>
Cc: daniel@iogearbox.net, bpf@vger.kernel.org,
netdev@vger.kernel.org, oss-drivers@netronome.com
Subject: Re: [PATCH v4 bpf-next 02/15] bpf: mark lo32 writes that should be zero extended into hi32
Date: Fri, 19 Apr 2019 13:40:51 -0700 [thread overview]
Message-ID: <20190419134051.71eeea08@cakuba.netronome.com> (raw)
In-Reply-To: <20190418235747.jv4yocuf6fgwahli@ast-mbp.dhcp.thefacebook.com>
On Thu, 18 Apr 2019 16:57:50 -0700, Alexei Starovoitov wrote:
> > @@ -6371,8 +6406,10 @@ static int propagate_liveness(struct bpf_verifier_env *env,
> > for (i = frame < vstate->curframe ? BPF_REG_6 : 0; i < BPF_REG_FP; i++) {
> > err = propagate_liveness_reg(env, &state_reg[i],
> > &parent_reg[i]);
> > - if (err)
> > + if (err < 0)
> > return err;
> > + if (err & REG_LIVE_READ64)
> > + mark_insn_zext(env, &parent_reg[i]);
>
> I'm not quite following why it's parent_reg here instead of state_reg.
Perhaps we should rename the parameters to something else than parent
here? I always have to do some mental gymnastics looking at this code..
"explored" and "current"?
The current state is parent, the "next" state that pruned the search
is "state". So we check if the reads under state X need 64bit, if so
have to propagate back to writes on current (which is called parent
here, even though it won't become state's parent, ugh.)
> If I understood the code the liveness can have all three states:
> REG_LIVE_READ64 | REG_LIVE_READ32
> REG_LIVE_READ64
> REG_LIVE_READ32
> whereas 2 is a superset of 3, so 1 should never be seen.
>
> If so, why in propagate_liveness we have this dance:
> + u8 parent_bits = parent_reg->live & REG_LIVE_READ;
> + u8 bits = reg->live & REG_LIVE_READ;
> + u8 bits_diff = parent_bits ^ bits;
> + u8 bits_prop = bits_diff & bits;
> int err;
>
> - if (parent_reg->live & REG_LIVE_READ || !(reg->live & REG_LIVE_READ))
> + /* No diff bit comes from "reg". */
> + if (!bits_prop)
>
> I'm struggling to see through all 3 combinations in respect to above diff.
> Shouldn't propagation of REG_LIVE_READ64 remove REG_LIVE_READ32 bit
> and clear subreg_def during mark_reg_read() instead of
> once in propagate_liveness() ?
This reminds me, I'm not entirely clear on the need to propagate the
zext through stack slots... Pointers are guaranteed to be 64bit, we
don't save parentage on scalars (AFAICT), why not pass REG_LIVE_READ
or READ64 to mark_reg_read() from stack_read?
next prev parent reply other threads:[~2019-04-19 20:40 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-15 17:26 [PATCH v4 bpf-next 00/15] bpf: eliminate zero extensions for sub-register writes Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 01/15] bpf: split read liveness into REG_LIVE_READ64 and REG_LIVE_READ32 Jiong Wang
2019-04-15 23:03 ` Jakub Kicinski
2019-04-16 1:26 ` Alexei Starovoitov
2019-04-16 7:39 ` Jiong Wang
2019-04-16 16:20 ` Alexei Starovoitov
2019-04-16 20:19 ` Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 02/15] bpf: mark lo32 writes that should be zero extended into hi32 Jiong Wang
2019-04-18 23:57 ` Alexei Starovoitov
2019-04-19 20:40 ` Jakub Kicinski [this message]
2019-04-19 21:14 ` Alexei Starovoitov
2019-04-19 21:33 ` Jakub Kicinski
2019-04-19 21:41 ` Alexei Starovoitov
2019-04-19 23:27 ` Jiong Wang
2019-04-19 23:28 ` Alexei Starovoitov
2019-04-15 17:26 ` [PATCH v4 bpf-next 03/15] bpf: reduce false alarm by refining helper call arg types Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 04/15] bpf: insert explicit zero extension insn when hardware doesn't do it implicitly Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 05/15] bpf: introduce new bpf prog load flags "BPF_F_TEST_RND_HI32" Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 06/15] bpf: randomize high 32-bit when BPF_F_TEST_RND_HI32 is set Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 07/15] libbpf: add "prog_flags" to bpf_program/bpf_prog_load_attr/bpf_load_program_attr Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 08/15] selftests: enable hi32 randomization for all tests Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 09/15] arm: bpf: eliminate zero extension code-gen Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 10/15] powerpc: " Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 11/15] s390: " Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 12/15] sparc: " Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 13/15] x32: " Jiong Wang
2019-04-15 17:26 ` [PATCH v4 bpf-next 14/15] riscv: " Jiong Wang
2019-04-17 7:55 ` Björn Töpel
2019-04-15 17:26 ` [PATCH v4 bpf-next 15/15] nfp: " Jiong Wang
2019-04-24 16:31 ` kbuild test robot
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=20190419134051.71eeea08@cakuba.netronome.com \
--to=jakub.kicinski@netronome.com \
--cc=alexei.starovoitov@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=jiong.wang@netronome.com \
--cc=netdev@vger.kernel.org \
--cc=oss-drivers@netronome.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).