From: Puranjay Mohan <puranjay12@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: "Ilya Leoshkevich" <iii@linux.ibm.com>,
"Alexei Starovoitov" <ast@kernel.org>,
"Daniel Borkmann" <daniel@iogearbox.net>,
"Andrii Nakryiko" <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
"Heiko Carstens" <hca@linux.ibm.com>,
"Vasily Gorbik" <gor@linux.ibm.com>,
"Alexander Gordeev" <agordeev@linux.ibm.com>,
"Kumar Kartikeya Dwivedi" <memxor@gmail.com>,
"Björn Töpel" <bjorn@kernel.org>,
"Johan Almbladh" <johan.almbladh@anyfinetworks.com>,
"Christophe Leroy" <christophe.leroy@csgroup.eu>
Subject: Re: [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX
Date: Wed, 13 Sep 2023 00:49:38 +0200 [thread overview]
Message-ID: <CANk7y0hK9sQJ-kRx3nQpVJSxpP=NzzFaLitOYq8=Pb6Dvk9fpg@mail.gmail.com> (raw)
In-Reply-To: <CAADnVQ+ccoQrTcOZW_BZXMv2A+uYEYdHqx0tSVgXK31vGS=+gA@mail.gmail.com>
Hi Alexei,
[...]
> I guess we never clearly defined what 'needs_zext' is supposed to be,
> so it wouldn't be fair to call 32-bit JITs buggy.
> But we better address this issue now.
> This 32-bit zeroing after LDX hurts mips64, s390, ppc64, riscv64.
> I believe all 4 JITs emit proper zero extension into 64-bit register
> by using single cpu instruction,
> but they also define bpf_jit_needs_zext() as true,
> so extra BPF_ZEXT_REG() is added by the verifier
> and it is a pure run-time overhead.
I just realised that these zext instructions will not be a runtime
overhead because the JITs ignore them.
Like
s390 does:
case BPF_LDX | BPF_MEM | BPF_B: /* dst = *(u8 *)(ul) (src + off) */
case BPF_LDX | BPF_PROBE_MEM | BPF_B:
/* llgc %dst,0(off,%src) */
EMIT6_DISP_LH(0xe3000000, 0x0090, dst_reg, src_reg, REG_0, off);
jit->seen |= SEEN_MEM;
if (insn_is_zext(&insn[1]))
insn_count = 2; /* this will skip the next zext instruction */
break;
powerpc does after LDX:
if (size != BPF_DW && insn_is_zext(&insn[i + 1]))
addrs[++i] = ctx->idx * 4;
> It's better to remove
> if (t != SRC_OP)
> return BPF_SIZE(code) == BPF_DW;
> from is_reg64() to avoid adding BPF_ZEXT_REG() insn
> and fix 32-bit JITs at the same time.
> RISCV32, PowerPC32, x86-32 JITs fixed in the first 3 patches
> to always zero upper 32-bit after LDX and
> then 4th patch to remove these two lines.
I have sent the patches for above, although I think this optimization
is useful because
zero extension after LDX is only required when the loaded value is
later being used as
a 64-bit value. If it is not the case then the verifier will not emit
the zext and 32-bit JITs will emit
1 less instruction because they expect the verifier to do the zext for
them where required.
Link to patch series:
https://lore.kernel.org/bpf/20230912224654.6556-1-puranjay12@gmail.com/T/#t
Thanks,
Puranjay
next prev parent reply other threads:[~2023-09-12 22:49 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-30 1:07 [PATCH bpf-next 00/11] Implement cpuv4 support for s390x Ilya Leoshkevich
2023-08-30 1:07 ` [PATCH bpf-next 01/11] bpf: Disable zero-extension for BPF_MEMSX Ilya Leoshkevich
2023-09-01 10:40 ` Yonghong Song
2023-09-01 14:19 ` Puranjay Mohan
2023-09-01 14:56 ` Puranjay Mohan
2023-09-07 0:39 ` Alexei Starovoitov
2023-09-07 7:33 ` Puranjay Mohan
2023-09-07 15:36 ` Alexei Starovoitov
2023-09-07 16:39 ` Puranjay Mohan
2023-09-07 22:45 ` Alexei Starovoitov
2023-09-07 22:57 ` Puranjay Mohan
2023-09-12 22:49 ` Puranjay Mohan [this message]
2023-09-13 0:09 ` Alexei Starovoitov
2023-09-13 0:22 ` Puranjay Mohan
2023-09-13 1:49 ` Alexei Starovoitov
2023-09-13 6:10 ` Christophe Leroy
2023-09-03 8:16 ` Ilya Leoshkevich
2023-08-30 1:07 ` [PATCH bpf-next 02/11] net: netfilter: Adjust timeouts of non-confirmed CTs in bpf_ct_insert_entry() Ilya Leoshkevich
2023-08-31 15:30 ` Daniel Borkmann
2023-09-03 8:23 ` Ilya Leoshkevich
2023-08-30 1:07 ` [PATCH bpf-next 03/11] selftests/bpf: Unmount the cgroup2 work directory Ilya Leoshkevich
2023-08-30 1:07 ` [PATCH bpf-next 04/11] selftests/bpf: Add big-endian support to the ldsx test Ilya Leoshkevich
2023-08-30 1:07 ` [PATCH bpf-next 05/11] s390/bpf: Implement BPF_MOV | BPF_X with sign-extension Ilya Leoshkevich
2023-08-30 1:07 ` [PATCH bpf-next 06/11] s390/bpf: Implement BPF_MEMSX Ilya Leoshkevich
2023-08-30 1:07 ` [PATCH bpf-next 07/11] s390/bpf: Implement unconditional byte swap Ilya Leoshkevich
2023-08-30 1:07 ` [PATCH bpf-next 08/11] s390/bpf: Implement unconditional jump with 32-bit offset Ilya Leoshkevich
2023-08-30 1:07 ` [PATCH bpf-next 09/11] s390/bpf: Implement signed division Ilya Leoshkevich
2023-08-30 1:07 ` [PATCH bpf-next 10/11] selftests/bpf: Enable the cpuv4 tests for s390x Ilya Leoshkevich
2023-09-01 10:41 ` Yonghong Song
2023-08-30 1:07 ` [PATCH bpf-next 11/11] selftests/bpf: Trim DENYLIST.s390x Ilya Leoshkevich
2023-09-14 13:00 ` [PATCH bpf-next 00/11] Implement cpuv4 support for s390x patchwork-bot+netdevbpf
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='CANk7y0hK9sQJ-kRx3nQpVJSxpP=NzzFaLitOYq8=Pb6Dvk9fpg@mail.gmail.com' \
--to=puranjay12@gmail.com \
--cc=agordeev@linux.ibm.com \
--cc=alexei.starovoitov@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bjorn@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=christophe.leroy@csgroup.eu \
--cc=daniel@iogearbox.net \
--cc=gor@linux.ibm.com \
--cc=hca@linux.ibm.com \
--cc=iii@linux.ibm.com \
--cc=johan.almbladh@anyfinetworks.com \
--cc=memxor@gmail.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).