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.
+/* 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 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.
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.
The rest looks good.