libusb: Fix high CPU usage in callback thread#783
Conversation
…attern Replace broken hidapi_thread_cond_timedwait (which used a fixed absolute timestamp in the past, causing a spin loop) with hidapi_thread_cond_wait using the standard condition variable predicate pattern. - callback_thread: Use cond_wait with proper predicate loop for zero CPU overhead when idle and immediate response on events/shutdown - hid_libusb_hotplug_callback: Protect queue and signal under callback_thread.mutex to prevent lost wakeups - hotplug_thread: Lock mutex before signaling for clean shutdown Agent-Logs-Url: https://github.com/libusb/hidapi/sessions/a47a730b-fbb6-4042-9afb-00e5aa94817d Co-authored-by: Youw <5939659+Youw@users.noreply.github.com>
Agent-Logs-Url: https://github.com/libusb/hidapi/sessions/a47a730b-fbb6-4042-9afb-00e5aa94817d Co-authored-by: Youw <5939659+Youw@users.noreply.github.com>
|
@copilot We used to handle all evens in a queue while holding the lock on a mutex, and now we lock it only when manipulating the queue, but not when handling the event. Is there a benefit in that approach? |
…signal Agent-Logs-Url: https://github.com/libusb/hidapi/sessions/808d88e5-75a9-4fc7-ba46-9cf6042c46d5 Co-authored-by: Youw <5939659+Youw@users.noreply.github.com>
Yes — releasing the lock during event processing avoids two problems:
The tradeoff is that |
|
@copilot I also see that |
…flag Agent-Logs-Url: https://github.com/libusb/hidapi/sessions/4b1a120a-0c8e-45c2-82ce-b7515db6e585 Co-authored-by: Youw <5939659+Youw@users.noreply.github.com>
Done in 3f4721e. |
…ENUMERATE Agent-Logs-Url: https://github.com/libusb/hidapi/sessions/687b4058-4c04-4d5f-8de6-2f38035fd760 Co-authored-by: Youw <5939659+Youw@users.noreply.github.com>
…ess_hotplug_event In process_hotplug_event(), the devs list was modified without holding hid_hotplug_context.mutex. Meanwhile, hid_hotplug_register_callback() iterates devs under that mutex during HID_API_HOTPLUG_ENUMERATE. A device disconnect processed by callback_thread could free a devs entry while the ENUMERATE loop was still traversing it, causing a use-after-free. Fix by wrapping process_hotplug_event() with hid_hotplug_context.mutex. The mutex is recursive, so hid_internal_invoke_callbacks() (which also acquires it) works correctly. This aligns with how the linux backend handles the same scenario. Agent-Logs-Url: https://github.com/libusb/hidapi/sessions/cb1dc652-1f96-457d-8319-55e4a6f6e6f3 Co-authored-by: Youw <5939659+Youw@users.noreply.github.com>
|
@CalcProgrammer1 could you check this change in you environment regarding high CPU usage? it should give even better result. |
The struct comment on hid_hotplug_context.devs claimed it was protected by hid_hotplug_context.mutex, but the final cleanup in callback_thread ran under only callback_thread.mutex — inconsistent with all other accesses (process_hotplug_event, hid_hotplug_register_callback) that hold hid_hotplug_context.mutex when reading or writing devs. Move the free to hid_internal_hotplug_cleanup(), after hidapi_thread_join(libusb_thread) returns, so every access to devs — including the teardown free — happens while holding the context mutex. Also fix a leak on the hid_hotplug_register_callback error path: when libusb_hotplug_register_callback fails, no hotplug thread is created, so hid_internal_hotplug_cleanup() can't be used to unwind the devs list assigned by the preceding hid_enumerate() call. Free it inline on that path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
I think this PR has more than enought changes already. Will merge it like this and going to review/update more of this using Claude - it already spotted potential races and UB in current implementation. |
Callback_thread used hidapi_thread_cond_timedwait with an absolute
timespec already in the past, causing a tight spin loop. Replaced
with cond_wait + predicate pattern so the thread sleeps until an
event or a shutdown signal arrives.
Multithreading fixes folded in along the way:
natural condvar partner), with signal-under-mutex to avoid a
lost-wakeup race at shutdown.
to match hid_hotplug_register_callback()'s HID_API_HOTPLUG_ENUMERATE
traversal of hid_hotplug_context.devs — fixes a use-after-free
where a concurrent disconnect could free a list node mid-traversal.
hidapi_thread_join) so every access to devs happens under
hid_hotplug_context.mutex.
libusb_hotplug_register_callback() error path, which previously
leaked.
Closes: #782
Assisted-by: Copilot:claude-sonnet-4.6
Assisted-by: Copilot:claude-opus-4.6
Assisted-by: Claude:claude-opus-4.7