Skip to content

Fix high CPU usage in callback thread due to incorrect timespec#782

Open
k1-801 wants to merge 2 commits intolibusb:connection-callbackfrom
k1-801:patch-1
Open

Fix high CPU usage in callback thread due to incorrect timespec#782
k1-801 wants to merge 2 commits intolibusb:connection-callbackfrom
k1-801:patch-1

Conversation

@k1-801
Copy link
Copy Markdown
Contributor

@k1-801 k1-801 commented Mar 29, 2026

@CalcProgrammer1 noticed high CPU usage (50%) on one of the threads when libusb backend is used for hotplug. It was determined that the load was caused by hidapi_thread_cond_timedwait(), which requires an absolute timespec, while the code passes a relative value. This patch copies how the structure is filled from the other place where hidapi_thread_cond_timedwait() is uses, which should fix the issue.

@mcuee mcuee added the libusb Related to libusb backend label Mar 29, 2026
@Youw Youw requested a review from Copilot March 29, 2026 17:29
libusb/hid.c Outdated
/* This timeout only affects how much time it takes to stop the thread */
hidapi_timespec ts;
hidapi_thread_gettime(&ts);
hidapi_thread_addtime(&ts, 5);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd set it to 1 second - I've been using 1 second in multiple projects, and seems reasonable
5 second to exit the thread (effectively - to quit the application in some cases) seem a bit too much

Copy link
Copy Markdown
Contributor Author

@k1-801 k1-801 Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the call signature, the number is in milliseconds, not whole seconds.
image

When it comes to milliseconds, it doesn't really matter what number is set there, as execution at single milliseconds scale is likely limited by OS scheduler, not the actual number set here, so long as it's long enough to actually go idle.

A whole second here would be too long (at least to my taste), and checking every single millisecond seems a bit excessive. It can be changed later on to 20 or 40 or even 100 milliseconds if needed (i.e. if this thread makes any noticeable CPU load).

Copy link
Copy Markdown
Contributor Author

@k1-801 k1-801 Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, the patch seems to work:

image

("wrapped HID devices" in this context means HID devices that use libusb backend specifically)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While modern processors are fast-enough practically not to notice wait/idle/check every 5ms, my experience tells it is a bit too often, I'd make it 100ms then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increased the timeout to 100

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes high CPU usage in the libusb hotplug callback thread by passing an absolute timeout to hidapi_thread_cond_timedwait() (required by the pthread-backed implementation), instead of a relative timespec.

Changes:

  • Move timeout computation into the callback loop and build it as now + 5ms via hidapi_thread_gettime() + hidapi_thread_addtime().
  • Align the hotplug callback thread’s timed-wait behavior with the existing timed-wait usage elsewhere in libusb/hid.c.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Youw
Copy link
Copy Markdown
Member

Youw commented Mar 30, 2026

Meanwhile I'm looking at a fundamentally different aproach to fix this issue - there should be no timeout at all: #783

@k1-801
Copy link
Copy Markdown
Contributor Author

k1-801 commented Mar 30, 2026

The approach suggested there would also work; however, the proposed change simultaneously breaks the cache of connected devices, which would lead to inability to report disconnect events.

I don't remember exactly why I started with a timed cond_wait, it was either a precaution against infinite wait if, or the exact reason could have been resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libusb Related to libusb backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants