Received-SPF: pass (google.com: domain of linux-kernel+bounces-206938-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sat, 8 Jun 2024 10:00:37 +0200
From: Benjamin Tissoires <bentiss@kernel.org>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
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>
Subject: Re: [PATCH HID v2 03/16] HID: bpf: implement HID-BPF through
 bpf_struct_ops
Message-ID: <m2eq2mdkdjrbumnj2mgvbsstfi4pcigss35gkj365ck2stx2vf@gatvf73z2sd4>
References: <20240607-hid_bpf_struct_ops-v2-0-3f95f4d02292@kernel.org>
 <20240607-hid_bpf_struct_ops-v2-3-3f95f4d02292@kernel.org>
 <CAADnVQJo71xGQKLTW6Z1xsTLjtikN8bfemPUF4zj2c2Uvi5JMw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAADnVQJo71xGQKLTW6Z1xsTLjtikN8bfemPUF4zj2c2Uvi5JMw@mail.gmail.com>

On Jun 07 2024, Alexei Starovoitov wrote:
> On Fri, Jun 7, 2024 at 8:28 AM Benjamin Tissoires <bentiss@kernel.org> 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.

Good point. The only purpose of this was to expose or not the fields in
the doc, so I'll make it clear that this is the reason of
"private/public".

> 
> > +
> > +/* fast path fields are put first to fill one cache line */
> 
> Also misleading. The whole struct fits one cache line.

true :)

> 
> > +
> > +       /**
> > +        * @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 report 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.

ack

> 
> > +
> >  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_list */
> 
> 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 rcU.

Ack

> 
> The rest looks good.

Thanks for looking into it!

Cheers,
Benjamin