-
Notifications
You must be signed in to change notification settings - Fork 148
Fix CPU spinning when curl_easy_send returns 0 bytes on SSL socket writes #6890
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for your contribution @harshitkr12! We will review the pull request and get back to you soon. |
a3d2cbc to
ac65016
Compare
|
Thank you! I am approving the pull request, it looks good to me. I'd like @LarryOsterman to review before merging. @LarryOsterman, if you will be doing the next release (it is probably January 8, unless @ronniegeraghty says it is a week after), please make sure that the changelog has an entry for this fix, as well as the acknowledgements section + credit after the fix line, similar to the other fixes that we've taken. OR, I think we can wait for the week of Jan 12th - it is still going to be a release week, and then I can make the release myself. I suggest we do that. But just in case, here is the hange that we'll need for the CHANGELOG.md in core: I am going to follow up with Rubrik in the issue letting them know how they can start consuming it right away, if they need it. |
|
/azp run cpp - core |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Live tests are green. |
|
/azp run cpp - storage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // When curl_easy_send returns CURLE_OK but sends 0 bytes (which can happen on SSL | ||
| // sockets), add a small delay to prevent rapid retries that keep the CPU busy | ||
| // without making progress. This allows the scheduler to run other threads. | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::this_thread::yield will give up the time slice to other like or higher priority threads. Does curl actually need the thread to suspend (sleep) in case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation of this_thread::yield: https://en.cppreference.com/w/cpp/thread/yield.html
Provides a hint to the implementation to reschedule the execution of threads, allowing other threads to run.
Note that it is only a hint.
Also, note Linux CFS scheduler behavior https://www.kernel.org/doc/html/latest/scheduler/sched-design-CFS.html
CFS’s task picking logic is based on this p->se.vruntime value and it is thus very simple: it always tries to run the task with the smallest p->se.vruntime value (i.e., the task which executed least so far). CFS always tries to split up CPU time between runnable tasks as close to “ideal multitasking hardware” as possible.
This means - since Linux CFS (Completely Fair Scheduler) tries to be "fair," if your thread has yielded a lot recently, yield might effectively be a "no-op" (do nothing) if the scheduler decides you still haven't used your "fair share" of time compared to others, or if no other thread of the same priority is waiting.
this_thread::yieldwill maybe yield the cpu but keep the thread in Running state and let OS sched decide what to dothis_thread::sleep_forwill move it to Blocked state which is much better in this scenario and implicitly includes yielding the cpu.
Conclusion
- Using
this_thread::yieldin this context is risky and might not solve the cpu spin problem. Standard sleep is safer and better.
-- Abhinav
Summary:
Why ?
When uploading blobs to Azure Storage over HTTPS (SSL/TLS),
curl_easy_send()can returnCURLE_OKwith 0 bytes sent. This would lead to a retry without delay immediately, causing a tight CPU-spinning loop.What ?
Added a 10ms sleep when
curl_easy_send()returnsCURLE_OKwith 0 bytes sent to prevent rapid retries and allow the OS scheduler to run other threads.Test Plan:
Fixes #6817