Received-SPF: pass (google.com: domain of linux-kernel+bounces-206512-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240607-hid_bpf_struct_ops-v2-0-3f95f4d02292@kernel.org> <20240607-hid_bpf_struct_ops-v2-3-3f95f4d02292@kernel.org>
In-Reply-To: <20240607-hid_bpf_struct_ops-v2-3-3f95f4d02292@kernel.org>
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Fri, 7 Jun 2024 09:51:34 -0700
Message-ID: <CAADnVQJo71xGQKLTW6Z1xsTLjtikN8bfemPUF4zj2c2Uvi5JMw@mail.gmail.com>
Subject: Re: [PATCH HID v2 03/16] HID: bpf: implement HID-BPF through bpf_struct_ops
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>, Jiri Kosina <jikos@kernel.org>, Jonathan Corbet <corbet@lwn.net>, 
	Alexei Starovoitov <ast@kernel.org>, 
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
	bpf <bpf@vger.kernel.org>, 
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>, 
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jun 7, 2024 at 8:28=E2=80=AFAM Benjamin Tissoires <bentiss@kernel.o=
rg> wrote:
> +struct hid_bpf_ops {
> +       /* hid_id needs to stay first so we can easily change it
> +        * from userspace.
> +        */
> +       int                     hid_id;
> +       u32                     flags;
> +
> +       /* private: internal use only */
> +       struct list_head        list;
> +
> +       /* public: rest is public */

Didn't notice it before, but the above comments are misleading.
The whole struct is private to the kernel and bpf prog, while
registering, can only touch a handful.
I'd drop "internal use" and "is public". It's not an uapi.

> +
> +/* fast path fields are put first to fill one cache line */

Also misleading. The whole struct fits one cache line.

> +
> +       /**
> +        * @hid_device_event: called whenever an event is coming in from =
the device
> +        *
> +        * It has the following arguments:
> +        *
> +        * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
> +        *
> +        * Return: %0 on success and keep processing; a positive
> +        * value to change the incoming size buffer; a negative
> +        * error code to interrupt the processing of this event
> +        *
> +        * Context: Interrupt context.
> +        */
> +       int (*hid_device_event)(struct hid_bpf_ctx *ctx, enum hid_report_=
type report_type);
> +
> +/* control/slow paths put last */
> +
> +       /**
> +        * @hid_rdesc_fixup: called when the probe function parses the re=
port descriptor
> +        * of the HID device
> +        *
> +        * It has the following arguments:
> +        *
> +        * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
> +        *
> +        * Return: %0 on success and keep processing; a positive
> +        * value to change the incoming size buffer; a negative
> +        * error code to interrupt the processing of this device
> +        */
> +       int (*hid_rdesc_fixup)(struct hid_bpf_ctx *ctx);
> +
> +       /* private: internal use only */
> +       struct hid_device *hdev;
> +} ____cacheline_aligned_in_smp;

Such alignment is an overkill.
I don't think you can measure the difference.

> +
>  struct hid_bpf_prog_list {
>         u16 prog_idx[HID_BPF_MAX_PROGS_PER_DEV];
>         u8 prog_cnt;
> @@ -129,6 +188,10 @@ struct hid_bpf {
>         bool destroyed;                 /* prevents the assignment of any=
 progs */
>
>         spinlock_t progs_lock;          /* protects RCU update of progs *=
/
> +
> +       struct hid_bpf_ops *rdesc_ops;
> +       struct list_head prog_list;
> +       struct mutex prog_list_lock;    /* protects RCU update of prog_li=
st */

mutex protects rcu update... sounds very odd.
Just say that mutex protects prog_list update, because "RCU update"
has a different meaning. RCU logic itself is what protects Update part of r=
cU.

The rest looks good.