From: Jiong Wang <jiong.wang@netronome.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Jiong Wang <jiong.wang@netronome.com>,
daniel@iogearbox.net, bpf@vger.kernel.org,
netdev@vger.kernel.org, oss-drivers@netronome.com
Subject: Re: [PATCH v6 bpf-next 01/17] bpf: verifier: offer more accurate helper function arg and return type
Date: Fri, 10 May 2019 09:30:28 +0100 [thread overview]
Message-ID: <87sgtmk8yj.fsf@netronome.com> (raw)
In-Reply-To: <20190510015352.6w6fghcthtjj74pl@ast-mbp>
Alexei Starovoitov writes:
> On Thu, May 09, 2019 at 01:32:30PM +0100, Jiong Wang wrote:
>>
>> Alexei Starovoitov writes:
>>
>> > On Wed, May 08, 2019 at 03:45:12PM +0100, Jiong Wang wrote:
>> >>
>> >> I might be misunderstanding your points, please just shout if I am wrong.
>> >>
>> >> Suppose the following BPF code:
>> >>
>> >> unsigned helper(unsigned long long, unsigned long long);
>> >> unsigned long long test(unsigned *a, unsigned int c)
>> >> {
>> >> unsigned int b = *a;
>> >> c += 10;
>> >> return helper(b, c);
>> >> }
>> >>
>> >> We get the following instruction sequence by latest llvm
>> >> (-O2 -mattr=+alu32 -mcpu=v3)
>> >>
>> >> test:
>> >> 1: w1 = *(u32 *)(r1 + 0)
>> >> 2: w2 += 10
>> >> 3: call helper
>> >> 4: exit
>> >>
>> >> Argument Types
>> >> ===
>> >> Now instruction 1 and 2 are sub-register defines, and instruction 3, the
>> >> call, use them implicitly.
>> >>
>> >> Without the introduction of the new ARG_CONST_SIZE32 and
>> >> ARG_CONST_SIZE32_OR_ZERO, we don't know what should be done with w1 and
>> >> w2, zero-extend them should be fine for all cases, but could resulting in a
>> >> few unnecessary zero-extension inserted.
>> >
>> > I don't think we're on the same page.
>> > The argument type is _const_.
>> > In the example above they are not _const_.
>>
>> Right, have read check_func_arg + check_helper_mem_access again.
>>
>> Looks like ARG_CONST_SIZE* are designed for describing memory access size
>> for things like bounds checking. It must be a constant for stack access,
>> otherwise prog will be rejected, but it looks to me variables are allowed
>> for pkt/map access.
>>
>> But pkt/map has extra range info. So, indeed, ARG_CONST_SIZE32* are
>> unnecessary, the width could be figured out through the range.
>>
>> Will just drop this patch in next version.
>>
>> And sorry for repeating it again, I am still concerned on the issue
>> described at https://www.spinics.net/lists/netdev/msg568678.html.
>>
>> To be simple, zext insertion is based on eBPF ISA and assumes all
>> sub-register defines from alu32 or narrow loads need it if the underlying
>
> It's not an assumption. It's a requirement. If JIT is not zeroing
> upper 32-bits after 32-bit alu or narrow load it's a bug.
>
>> hardware arches don't do it. However, some arches support hardware zext
>> partially. For example, PowerPC, SPARC etc are 64-bit arches, while they
>> don't do hardware zext on alu32, they do it for narrow loads. And RISCV is
>> even more special, some alu32 has hardware zext, some don't.
>>
>> At the moment we have single backend hook "bpf_jit_hardware_zext", once a
>> backend enable it, verifier just insert zero extension for all identified
>> alu32 and narrow loads.
>>
>> Given verifier analysis info is not pushed down to JIT back-ends, verifier
>> needs more back-end info pushed up from back-ends. Do you think make sense
>> to introduce another hook "bpf_jit_hardware_zext_narrow_load" to at least
>> prevent unnecessary zext inserted for narrowed loads for arches like
>> PowerPC, SPARC?
>>
>> The hooks to control verifier zext insertion then becomes two:
>>
>> bpf_jit_hardware_zext_alu32
>> bpf_jit_hardware_zext_narrow_load
>
> and what to do with other combinations?
> Like in some cases narrow load on particular arch will be zero extended
> by hw and if it's misaligned or some other condition then it will not be?
> It doesn't feel that we can enumerate all such combinations.
Yes, and above narrow_load is just an example. As mentioned, behaviour on
alu32 also diverse on some arches.
> It feels 'bpf_jit_hardware_zext' backend hook isn't quite working.
It is still useful for x86_64 and aarch64 to disable verifier insertion
pass completely. But then perhaps should be renamed into
"bpf_jit_verifier_zext". Returning false means verifier should disable the
insertion completely.
> It optimizes out some zext, but may be adding unnecessary extra zexts.
This is exactly my concern.
> May be it should be a global flag from the verifier unidirectional to JITs
> that will say "the verifier inserted MOV32 where necessary. JIT doesn't
> need to do zext manually".
> And then JITs will remove MOV32 when hw does it automatically.
> Removal should be easy, since such insn will be right after corresponding
> alu32 or narrow load.
OK, so you mean do a simple peephole to combine insns. JIT looks forward
the next insn, if it is mov32 with dst_src == src_reg, then skip it. And
only do this when jitting a sub-register write eBPF insn and there is
hardware zext support.
I guess such special mov32 won't be generated by compiler that it won't be
jump destination hence skip it is safe.
For zero extension insertion part of this set, I am going to do the
following changes in next version:
1. verifier inserts special "mov32" (dst_reg == src_reg) as "zext".
JIT could still save zext for the other "mov32", but should always do
zext for this special "mov32".
2. rename 'bpf_jit_hardware_zext' to 'bpf_jit_verifier_zext' which
returns false at default to disable zext insertion.
3. JITs want verifier zext override bpf_jit_verifier_zext to return
true and should skip unnecessary mov32 as described above.
Looks good?
Regards,
Jiong
next prev parent reply other threads:[~2019-05-10 8:30 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-03 10:42 [PATCH v6 bpf-next 00/17] bpf: eliminate zero extensions for sub-register writes Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 01/17] bpf: verifier: offer more accurate helper function arg and return type Jiong Wang
2019-05-06 13:57 ` Daniel Borkmann
2019-05-06 22:25 ` Jiong Wang
2019-05-08 11:12 ` Jiong Wang
2019-05-06 15:50 ` Alexei Starovoitov
2019-05-08 14:45 ` Jiong Wang
2019-05-08 17:51 ` Alexei Starovoitov
2019-05-09 12:32 ` Jiong Wang
2019-05-09 17:31 ` Jiong Wang
2019-05-10 1:53 ` Alexei Starovoitov
2019-05-10 8:30 ` Jiong Wang [this message]
2019-05-10 20:10 ` Alexei Starovoitov
2019-05-10 21:59 ` Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 02/17] bpf: verifier: mark verified-insn with sub-register zext flag Jiong Wang
2019-05-06 13:49 ` Daniel Borkmann
2019-05-06 14:49 ` Daniel Borkmann
2019-05-06 22:14 ` Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 03/17] bpf: verifier: mark patched-insn " Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 04/17] bpf: introduce new alu insn BPF_ZEXT for explicit zero extension Jiong Wang
2019-05-06 15:57 ` Alexei Starovoitov
2019-05-06 23:19 ` Jiong Wang
2019-05-07 4:29 ` Jiong Wang
2019-05-07 4:40 ` Alexei Starovoitov
2019-05-03 10:42 ` [PATCH v6 bpf-next 05/17] bpf: verifier: insert BPF_ZEXT according to zext analysis result Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 06/17] bpf: introduce new bpf prog load flags "BPF_F_TEST_RND_HI32" Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 07/17] bpf: verifier: randomize high 32-bit when BPF_F_TEST_RND_HI32 is set Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 08/17] libbpf: add "prog_flags" to bpf_program/bpf_prog_load_attr/bpf_load_program_attr Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 09/17] selftests: bpf: adjust several test_verifier helpers for insn insertion Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 10/17] selftests: bpf: enable hi32 randomization for all tests Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 11/17] arm: bpf: eliminate zero extension code-gen Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 12/17] powerpc: " Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 13/17] s390: " Jiong Wang
2019-05-03 13:41 ` Heiko Carstens
2019-05-03 13:50 ` Eric Dumazet
2019-05-03 14:09 ` Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 14/17] sparc: " Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 15/17] x32: " Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 16/17] riscv: " Jiong Wang
2019-05-03 10:42 ` [PATCH v6 bpf-next 17/17] nfp: " Jiong Wang
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=87sgtmk8yj.fsf@netronome.com \
--to=jiong.wang@netronome.com \
--cc=alexei.starovoitov@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--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).