Skip to content
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

build: stop detecting sched_yield() on Windows #16037

Closed
wants to merge 5 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jan 17, 2025

On Windows a successful sched_yield() detection requires mingw-w64
built with POSIX threads (not Win32 threads) and GCC (not llvm/clang).
(linking to winpthread via custom options may also work.)

In CMake builds, it was pre-cached as unavailable before this patch.

When detected (via autotools), it got only used for Windows XP or older
targets combined with a non-GCC, non-clang compiler that doesn't support
__builtin_ia32_pause(), or with the Intel C compiler. According to
lib/easy_lock.h.

mingw-w64 only supports GCC and clang, leaving a very narrow chance when
sched_yield() gets called on Windows. Even then, sched_yield() is
implemented in winpthread as Sleep(0), which may or not be a useful.
It's also trivial to implement locally if it is, and such rare build
combination is also deemed useful.

Thus, this patch marks sched_yield() permanently unavailable on the
Windows platform also with autotools, and instead of pre-caching, skip
this feature check with CMake.

This syncs HAVE_SCHED_YIELD between build methods on Windows.

Follow-up to 9b517c8 #11973
Follow-up to 23af112 #8680

Closes #16037


mingw, AM x86_64 c-ares U                     checking for sched_yield... yes           POSIX threads + gcc
mingw, AM x86_64 default                      checking for sched_yield... yes           POSIX threads + gcc
mingw, CM clang-x86_64 openssl                Looking for sched_yield - not found       POSIX threads + clang
mingw, CM i686 schannel R                     Looking for sched_yield - found           POSIX threads + gcc
mingw, CM ucrt-x86_64 schannel R TrackMemory  Looking for sched_yield - found           POSIX threads + gcc
mingw, CM ucrt-x86_64 schannel uwp            Looking for sched_yield - found           POSIX threads + gcc
mingw, CM x86_64 schannel c-ares U            Looking for sched_yield - found           POSIX threads + gcc
mingw, CM x86_64 schannel dev debug           Looking for sched_yield - found           POSIX threads + gcc
dl-mingw, CM 9.5.0-x86_64 schannel            Looking for sched_yield - found           POSIX threads + gcc
dl-mingw, CM 7.3.0-x86_64 schannel U          Looking for sched_yield - not found       win32 threads + gcc
dl-mingw, CM 6.4.0-i686 schannel !unity       Looking for sched_yield - not found       win32 threads + gcc
linux-mingw, CM gcc                           Looking for sched_yield - not found       win32 threads + gcc
msvc, CM x64-uwp openssl                      Looking for sched_yield - not found       no winpthread
msvc, CM x64-windows boringssl                Looking for sched_yield - not found       no winpthread
msvc, CM x64-windows libressl                 Looking for sched_yield - not found       no winpthread
msvc, CM x64-windows mbedtls                  Looking for sched_yield - not found       no winpthread
msvc, CM x64-windows openssl                  Looking for sched_yield - not found       no winpthread
msvc, CM x64-windows schannel MultiSSL U      Looking for sched_yield - not found       no winpthread
msvc, CM x64-windows wolfssl                  Looking for sched_yield - not found       no winpthread

On Windows this function is exported from winpthread, but winpthread
is never used in Windows builds by curl itself.

In cmake it was marked as never available, but autotools did check
for this function. It found it sometimes (presumable when a dependency
linked to pthread), resulting in a different `curl_config.h`.

Sync the build systems by skipping this check in both, for Windows.

Follow-up to 9b517c8 curl#11973
Follow-up to 23af112 curl#8680
@vszakats vszakats added build Windows Windows-specific tidy-up labels Jan 17, 2025
This reverts commit 34db1e3.
This reverts commit d5d9113.
@vszakats vszakats changed the title [WIP] build: stop detecting sched_yield() on Windows build: stop detecting sched_yield() on Windows Jan 17, 2025
@vszakats vszakats closed this in 2c4bfef Jan 17, 2025
@vszakats vszakats deleted the w-sched-yield branch January 17, 2025 22:32
@jay
Copy link
Member

jay commented Jan 18, 2025

If targeting Windows Vista+ the lock is srwlock style and if targeting pre-Vista I don't think there's a stdatomic so there never was sched_yield use on Windows. I think for XP target what may happen is there is no thread safe global init. That is true of Visual Studio legacy stuff in projects/ as well.

Also, Sleep(0) does not always work as one might expect on Windows. It would seem in this specific case that the contentious threads are surely all of the same priority so it should work as expected, however if one of the libcurl init threads is waiting on some library which is waiting on some other thing you can see it quickly becomes uncertain.

@vszakats
Copy link
Member Author

vszakats commented Jan 19, 2025

If targeting Windows Vista+ the lock is srwlock style and if targeting pre-Vista I don't think there's a stdatomic so there never was sched_yield use on Windows. I think for XP target what may happen is there is no thread safe global init. That is true of Visual Studio legacy stuff in projects/ as well.

stdatomic is offered by mingw-w64 when combined with gcc 4.9+ or clang 3.6+.
Recent versions still support targeting XP. That, combined with the other conditions
(POSIX threads + no __builtin_ia32_pause() + not ARM64) is unlikely, but not
entirely impossible.

Also, Sleep(0) does not always work as one might expect on Windows. It would seem in this specific case that the contentious threads are surely all of the same priority so it should work as expected, however if one of the libcurl init threads is waiting on some library which is waiting on some other thing you can see it quickly becomes uncertain.

Would it be useful to put add an #error for that build combination?

--- a/lib/easy_lock.h
+++ b/lib/easy_lock.h
@@ -44,7 +44,7 @@
 
 #elif defined(HAVE_ATOMIC) && defined(HAVE_STDATOMIC_H)
 #include <stdatomic.h>
-#if defined(HAVE_SCHED_YIELD)
+#elif !defined(_WIN32) && defined(HAVE_SCHED_YIELD)
 #include <sched.h>
 #endif
 
@@ -81,6 +81,8 @@ static CURL_INLINE void curl_simple_lock_lock(curl_simple_lock *lock)
       __builtin_ia32_pause();
 #elif defined(__aarch64__)
       __asm__ volatile("yield" ::: "memory");
+#elif defined(_WIN32)
+#error "...."
 #elif defined(HAVE_SCHED_YIELD)
       sched_yield();
 #endif

jay added a commit to jay/curl that referenced this pull request Jan 19, 2025
- Prefer Sleep(1) over sched_yield() for pre-Vista thread yield.

On Windows sched_yield is often implemented as Sleep(0) which only
yields to threads of highest priority to current priority. However,
during libcurl initialization if there is thread contention then it's
possible that there is a wait for a different library or OS thread of
a lesser priority and then the yield is not effective during that time.
On the other hand Sleep(1) will wait the minimum time slice which is
usually like 15ms or more.

Commit 2c4bfef removed sched_yield detection on Windows. Regardless
though Sleep(1) should be preferred over sched_yield.

For Windows Vista and later we use SRW locks and do not have this issue.

Ref: curl#16037 (comment)
Ref: https://devblogs.microsoft.com/oldnewthing/20051004-09/?p=33923

Closes #xxxx
jay added a commit to jay/curl that referenced this pull request Jan 19, 2025
- Prefer Sleep(1) over sched_yield() for pre-Vista thread yield.

On Windows sched_yield is often implemented as Sleep(0) which only
yields to threads of highest priority to current priority. However,
during libcurl initialization if there is thread contention then it's
possible that there is a wait for a different library or OS thread of
a lesser priority and then the yield is not effective during that time.
On the other hand Sleep(1) will wait the minimum time slice which is
usually like 15ms or more.

Commit 2c4bfef recently removed sched_yield detection on Windows.
Regardless though Sleep(1) should be preferred over sched_yield.

For Windows Vista and later we use SRW locks and do not have this issue.

Ref: curl#16037 (comment)
Ref: https://devblogs.microsoft.com/oldnewthing/20051004-09/?p=33923

Closes #xxxx
jay added a commit to jay/curl that referenced this pull request Jan 19, 2025
- Prefer Sleep(1) over sched_yield() for pre-Vista thread yield.

On Windows sched_yield is often implemented as Sleep(0) which only
yields to threads of highest priority to current priority. However,
during libcurl initialization if there is thread contention then it's
possible that there is a wait for a different library or OS thread of
a lesser priority and then the yield is not effective during that time.
On the other hand Sleep(1) will wait the minimum time slice which is
usually like 15ms or more.

Note 2c4bfef recently removed sched_yield detection on Windows.
Regardless though Sleep(1) should be preferred over sched_yield.

For Windows Vista and later we use SRW locks and do not have this issue.

Ref: curl#16037 (comment)
Ref: https://devblogs.microsoft.com/oldnewthing/20051004-09/?p=33923

Closes #xxxx
jay added a commit to jay/curl that referenced this pull request Jan 19, 2025
- Prefer Sleep(1) over sched_yield() for pre-Vista thread yield.

On Windows sched_yield is often implemented as Sleep(0) which only
yields to threads of highest priority to current priority. However,
during libcurl initialization if there is thread contention then it's
possible that there is a wait for a different library or OS thread of
a lesser priority and then the yield is not effective during that time.
On the other hand Sleep(1) will wait the minimum time slice which is
usually like 15ms or more.

Prior to this change 2c4bfef removed sched_yield detection on Windows,
which effectively removed the yield in the spin lock, and therefore this
change restores it.

For Windows Vista and later we use SRW locks and do not have this issue.

Ref: curl#16037 (comment)
Ref: https://devblogs.microsoft.com/oldnewthing/20051004-09/?p=33923

Closes #xxxx
jay added a commit to jay/curl that referenced this pull request Jan 19, 2025
- Prefer Sleep(1) over sched_yield() for pre-Vista thread yield.

On Windows sched_yield is often implemented as Sleep(0) which only
yields to threads of highest priority to current priority. However,
during libcurl initialization if there is thread contention then it's
possible that there is a wait for a different library or OS thread of
a lesser priority and then the yield is not effective during that time.
On the other hand Sleep(1) will wait the minimum time slice which is
usually like 15ms or more.

Prior to this change 2c4bfef removed sched_yield detection on Windows,
which effectively removed the yield in the spin lock, and therefore this
change restores the yield but in a different way.

For Windows Vista and later we use SRW locks and do not have this issue.

Ref: curl#16037 (comment)
Ref: https://devblogs.microsoft.com/oldnewthing/20051004-09/?p=33923

Closes #xxxx
@jay
Copy link
Member

jay commented Jan 19, 2025

Recent versions still support targeting XP.

Oh ok.

Would it be useful to put add an #error for that build combination?

No we don't need to error it can still work. #16048

jay added a commit that referenced this pull request Jan 21, 2025
- Prefer Sleep(1) over sched_yield() for pre-Vista thread yield.

On Windows sched_yield is often implemented as Sleep(0) which only
yields to threads of highest priority to current priority. However,
during libcurl initialization if there is thread contention then it's
possible that there is a wait for a different library or OS thread of
a lesser priority and then the yield is not effective during that time.
On the other hand Sleep(1) will wait the minimum time slice which is
usually like 15ms or more.

Prior to this change 2c4bfef removed sched_yield detection on Windows,
which effectively removed the yield in the spin lock, and therefore this
change restores the yield but in a different way.

For Windows Vista and later we use SRW locks and do not have this issue.

Ref: #16037 (comment)
Ref: https://devblogs.microsoft.com/oldnewthing/20051004-09/?p=33923

Closes #16048
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants