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