Received-SPF: pass (google.com: domain of linux-kernel+bounces-172617-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240422-sleepable_array_progs-v1-1-7c46ccbaa6e2@kernel.org>
 <7344022a-6f59-7cbf-ee45-6b7d59114be6@iogearbox.net> <un4jw2ef45vu3vwojpjca3wezso7fdp5gih7np73f4pmsmhmaj@csm3ix2ygd5i>
 <35nbgxc7hqyef3iobfvhbftxtbxb3dfz574gbba4kwvbo6os4v@sya7ul5i6mmd>
 <CAADnVQJaG8kDaJr5LV29ces+gVpgARLAWiUvE9Ee5huuiW5X=Q@mail.gmail.com>
 <mhkzkf4e23uvljtmwizwcxyuyat2tmfxn33xb4t7waafgmsa66@mcrzpj3b6ssx>
 <CAADnVQLJ=nxp3bZYYMJd0yrUtMNx2DcvYXXmbGKBQAiG85kSLQ@mail.gmail.com> <xt2zckipzs24eur4ozdo64uoxfed6jm3qixxgnp3o2gogjmosc@723s2u7jbsaz>
In-Reply-To: <xt2zckipzs24eur4ozdo64uoxfed6jm3qixxgnp3o2gogjmosc@723s2u7jbsaz>
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Tue, 7 May 2024 19:03:10 -0700
Message-ID: <CAADnVQK9qeMmzxE-aivmue-CF_hn1EFUTUAZyaMRqy2cW6j73A@mail.gmail.com>
Subject: Re: [PATCH] bpf: verifier: allow arrays of progs to be used in
 sleepable context
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>, Alexei Starovoitov <ast@kernel.org>, 
	John Fastabend <john.fastabend@gmail.com>, Andrii Nakryiko <andrii@kernel.org>, 
	Martin KaFai Lau <martin.lau@linux.dev>, Eduard Zingerman <eddyz87@gmail.com>, Song Liu <song@kernel.org>, 
	Yonghong Song <yonghong.song@linux.dev>, KP Singh <kpsingh@kernel.org>, 
	Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>, 
	bpf <bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, May 7, 2024 at 6:32=E2=80=AFAM Benjamin Tissoires <bentiss@kernel.o=
rg> wrote:
>
> Yes, exactly that. See [0] for my current WIP. I've just sent it, not
> for reviews, but so you see what I meant here.

The patches helped to understand, for sure, and on surface
they kind of make sense, but without seeing what is that
hid specific kfunc that will use it
it's hard to make a call.
The (u64)(long) casting concerns and prog lifetime are
difficult to get right. The verifier won't help and it all
will fall on code reviews.
So I'd rather not go this route.
Let's explore first what exactly the goal here.
We've talked about sleepable tail_calls, this async callbacks
from hid kfuncs, and struct-ops.
Maybe none of them fit well and we need something else.
Could you please explain (maybe once again) what is the end goal?

> Last time I checked, I thought struct_ops were only for defining one set
> of operations. And you could overwrite them exactly once.
> But after reading more carefully how it was used in tcp_cong.c, it seems
> we can have multiple programs which define the same struct_ops, and then
> it's the kernel which will choose which one needs to be run.

struct-ops is pretty much a mechanism for kernel to define
a set of callbacks and bpf prog to provide implementation for
these callbacks. The kernel choses when to call them.
tcp-bpf is one such user. sched_ext is another and more advanced.
Currently struct-ops bpf prog loading/attaching mechanism
only specifies the struct-ops. There is no device-id argument,
but that can be extended and kernel can keep per-device a set
of bpf progs.
struct-ops is a bit of overkill if you have only one callback.
It's typically for a set of callbacks.

> Last, I'm not entirely sure how I can specify which struct_ops needs to b=
e
> attached to which device, but it's worth a shot. I've already realized
> that I would probably have to drop the current way of HID-BPF is running,
> so now it's just technical bits to assemble :)

You need to call different bpf progs per device, right?
If indirect call is fine from performance pov,
then tailcall or struct_ops+device_argument might fit.

If you want max perf with direct calls then
we'd need to generalize xdp dispatcher.

So far it sounds that tailcalls might be the best actually,
since prog lifetime is handled by prog array map.
Maybe instead of bpf_tail_call helper we should add a kfunc that
will operate on prog array differently?
(if current bpf_tail_call semantics don't fit).