mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Eduard Zingerman <eddyz87@gmail.com>
To: Daniel Xu <dxu@dxuuu.xyz>,
	daniel@iogearbox.net, ast@kernel.org,  andrii@kernel.org
Cc: martin.lau@linux.dev, song@kernel.org, yonghong.song@linux.dev,
	 john.fastabend@gmail.com, kpsingh@kernel.org, sdf@fomichev.me,
	haoluo@google.com, 	jolsa@kernel.org, bpf@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next 3/3] bpf: arraymap: Skip boundscheck during inlining when possible
Date: Wed, 22 Jan 2025 16:15:53 -0800	[thread overview]
Message-ID: <da5e61f8bd7e3e8a7c9317c023cec93219069e59.camel@gmail.com> (raw)
In-Reply-To: <7bfb3b6b1d3400d03fd9b7a7e15586c826449c71.1737433945.git.dxu@dxuuu.xyz>

On Mon, 2025-01-20 at 21:35 -0700, Daniel Xu wrote:

[...]

Hi Daniel,

> @@ -221,11 +221,13 @@ static int array_map_gen_lookup(struct bpf_map *map,
>  
>  	*insn++ = BPF_ALU64_IMM(BPF_ADD, map_ptr, offsetof(struct bpf_array, value));
>  	*insn++ = BPF_LDX_MEM(BPF_W, ret, index, 0);
> -	if (!map->bypass_spec_v1) {
> -		*insn++ = BPF_JMP_IMM(BPF_JGE, ret, map->max_entries, 4);
> -		*insn++ = BPF_ALU32_IMM(BPF_AND, ret, array->index_mask);
> -	} else {
> -		*insn++ = BPF_JMP_IMM(BPF_JGE, ret, map->max_entries, 3);
> +	if (!inbounds) {
> +		if (!map->bypass_spec_v1) {
> +			*insn++ = BPF_JMP_IMM(BPF_JGE, ret, map->max_entries, 4);
> +			*insn++ = BPF_ALU32_IMM(BPF_AND, ret, array->index_mask);
> +		} else {
> +			*insn++ = BPF_JMP_IMM(BPF_JGE, ret, map->max_entries, 3);
> +		}
>  	}
>  
>  	if (is_power_of_2(elem_size)) {

Note that below this hunk there is the following code:

	*insn++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
	*insn++ = BPF_MOV64_IMM(ret, 0);
	return insn - insn_buf;

This part becomes redundant after your change. E.g. here is jit
listing for an_array_with_a_32bit_constant_0_no_nullness selftest:

JITED:
=============
func #0:
0:	f3 0f 1e fa                         	endbr64
4:	0f 1f 44 00 00                      	nopl	(%rax,%rax)
9:	0f 1f 00                            	nopl	(%rax)
c:	55                                  	pushq	%rbp
d:	48 89 e5                            	movq	%rsp, %rbp
10:	f3 0f 1e fa                         	endbr64
14:	48 81 ec 08 00 00 00                	subq	$0x8, %rsp
1b:	31 ff                               	xorl	%edi, %edi
1d:	89 7d fc                            	movl	%edi, -0x4(%rbp)
20:	48 89 ee                            	movq	%rbp, %rsi
23:	48 83 c6 fc                         	addq	$-0x4, %rsi
27:	48 bf 00 70 58 06 81 88 ff ff       	movabsq	$-0x777ef9a79000, %rdi
31:	48 81 c7 d8 01 00 00                	addq	$0x1d8, %rdi
38:	8b 46 00                            	movl	(%rsi), %eax
3b:	48 6b c0 30                         	imulq	$0x30, %rax, %rax
3f:	48 01 f8                            	addq	%rdi, %rax
42:	eb 02                               	jmp	L0             //
44:	31 c0                               	xorl	%eax, %eax     // never executed
46:	bf 04 00 00 00                      L0:	movl	$0x4, %edi     //
4b:	89 78 00                            	movl	%edi, (%rax)
4e:	b8 04 00 00 00                      	movl	$0x4, %eax
53:	c9                                  	leave
54:	e9 22 38 50 c3                      	jmp	0xffffffffc350387b

Also note that there are __arch_x86_64 and __jited tags for selftests.
These allow to match against disassembly of the generated binary code.
(See verifier_tailcall_jit.c for an example).
I think it would be good to add a test matching jited code for this feature.

[...]


  reply	other threads:[~2025-01-23  0:16 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-01-21  4:35 [PATCH bpf-next 0/3] bpf: Omit inlined bounds checks for null elided map lookups Daniel Xu
2025-01-21  4:35 ` [PATCH bpf-next 1/3] bpf: verifier: Store null elision decision in insn_aux_data Daniel Xu
2025-01-23  0:23   ` Alexei Starovoitov
2025-01-21  4:35 ` [PATCH bpf-next 2/3] bpf: map: Thread null elision metadata to map_gen_lookup Daniel Xu
2025-01-23  0:29   ` Alexei Starovoitov
2025-01-21  4:35 ` [PATCH bpf-next 3/3] bpf: arraymap: Skip boundscheck during inlining when possible Daniel Xu
2025-01-23  0:15   ` Eduard Zingerman [this message]
2025-01-23  0:33   ` Alexei Starovoitov

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=da5e61f8bd7e3e8a7c9317c023cec93219069e59.camel@gmail.com \
    --to=eddyz87@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=dxu@dxuuu.xyz \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=martin.lau@linux.dev \
    --cc=sdf@fomichev.me \
    --cc=song@kernel.org \
    --cc=yonghong.song@linux.dev \
    /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

all inboxes | Powered by JetHome®