I wrote: > I guess winpthreads' `pthread_spinlock_t` could be a mutex in disguise or > something like that, in which case first implementation could be more > flexible in the sense that we can change underlying structure and logic > anytime we want. I can as well update first implementation to not use loops > at all and instead use some Windows object for synchronization.
So, here's another implementation for spinlocks. This time, it uses auto-reset events; it is mostly original 0004.txt updated to use auto-reset events instead of loops. Unlike second version, no other changes to original patch series needed. I pushed this version to run through CI[1] and it passes. If we are to go with this event-based implementation, we need to keep in mind that winpthreads uses `pthread_spinlock_t` internally: - one global spinlock in barrier.c - one global spinlock in cond.c; used by pthread_cond_destroy and when using statically initialized `pthread_cond_t` object for the first time - two global spinlocks in rwlock.c - one spinlock per thread created with pthread_create What this means is that with event-based implementation winpthreads will use more system resources. I'm not saying that this is a problem, I'm just pointing it out. Another thing to mention is that the way barrier.c and rwlock.c use spinlocks is ridiculous... they lock and unlock global (!) spinlock whenever they increment/decrement reference count in *any* `pthread_rwlock_t`/`pthread_barrier_t` object. I feel like this is an overkill and it could be done better, but this is for another time. While testing this version locally, I noticed that some tests, or to be specific rwlock7 and rwlock8, take significantly more time to complete. Those tests print total amount of time test was running, so I started comparing... (Note that I took numbers from random `make check` runs. Obviously, they will differ from one run to another, so treat them as an approximation.) Tests rwlock7 and rwlock8 are nearly the same; there are two differences: 1. rwlock8 calls `sched_yield` before calling `pthread_rwlock_unlock` 2. rwlock8 does 100,000 iterations per thread, while rwlock7 does 1,000,000 (5 threads total in each test) With current spinlock implementation, they take 1,150ms and 226ms respectively. With implementation I sent last week, they take 1,560ms and 268ms respectively. With new event-based implementation, they take ridiculous 138,142ms and 13,810ms respectively. It does not mean that new event-based implementation is slow; as I mentioned, use of spinlocks in rwlock.c is ridiculous (at least in my opinion). What matters is that those results were when I ran `make check` sequentially, that is, without -j. When I was running `make check -j`, I saw that in some cases rwlock8 (100,000 iterations) would take the same or even more time to complete than rwlock7 (1,000,000 iterations). Meanwhile, results with event-based implementation were fairly consistent; the longest time I saw was about 150,000ms and 15,000ms respectively. We can go with this event-based implementation, in which case no changes to previously sent patches needed. Or, we can go with simplistic implementation I sent last week, in which case I'll need to update some follow up changes in tests. - Kirill Makurin [1] https://github.com/maiddaisuki/mingw-w64/actions/runs/26203279301
From 6b39f3d44174fc1dcf8b2ec906e36f5483c7768b Mon Sep 17 00:00:00 2001 From: Kirill Makurin <[email protected]> Date: Thu, 21 May 2026 08:13:27 +0900 Subject: [PATCH 04/11] winpthreads: new spinlock implementation Previous spinlock implementation did not keep track of lock ownership. It had effect that calling `pthread_spin_unlock` from thread A would always unlock lock held by thread B. The old implementation caused tests spin1.c and spin3.c to fail; they expected that implementation properly keeps track of lock ownership, which was not true. Now spinlocks are implemented using auto-reset event objects: - spinlock is unlocked when event object is in signaled state; this is the initial state - spinlock is locked by calling `WaitForSingleObject`, which releases a thread and automatically puts event object into non-signaled state; the thread that is released owns the lock - spinlock is released when owning thread calls `SetEvent`, which puts event object into signaled state; another thread can acquire the lock The new implementation uses `pthread_spinlock_t` as a handle which points to an internal structure rather than using it as a lock itself. Signed-off-by: Kirill Makurin <[email protected]> --- .../winpthreads/src/spinlock.c | 333 ++++++++++++++++-- .../winpthreads/tests/Makefile.am | 5 - 2 files changed, 313 insertions(+), 25 deletions(-) diff --git a/mingw-w64-libraries/winpthreads/src/spinlock.c b/mingw-w64-libraries/winpthreads/src/spinlock.c index 0fd8d256d..c6648bc82 100644 --- a/mingw-w64-libraries/winpthreads/src/spinlock.c +++ b/mingw-w64-libraries/winpthreads/src/spinlock.c @@ -1,6 +1,5 @@ /* - Copyright (c) 2013 mingw-w64 project - Copyright (c) 2015 Intel Corporation + Copyright (c) 2026 mingw-w64 project Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), @@ -25,6 +24,8 @@ #include "config.h" #endif +#include <stdlib.h> + #define WIN32_LEAN_AND_MEAN #include <windows.h> @@ -33,46 +34,338 @@ /* internal header files */ #include "misc.h" -/* We use the pthread_spinlock_t itself as a lock: - -1 is free, 0 is locked. - (This is dictated by PTHREAD_SPINLOCK_INITIALIZER, which we can't change - without breaking binary compatibility.) */ -typedef intptr_t spinlock_word_t; +/** + * Reference: + * + * pthread_spin_init(), pthread_spin_destroy(): + * <https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/functions/pthread_spin_destroy.html> + * + * pthread_spin_lock(), pthread_spin_trylock(): + * <https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/functions/pthread_spin_lock.html> + * + * pthread_spin_unlock(): + * <https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/functions/pthread_spin_unlock.html> + */ + +/** + * Internal structure pointed to by `pthread_spinlock_t` objects. + */ +typedef struct { + /** + * This value is used to indicate that spin lock has no owner. + */ +#define THREAD_ID_NO_OWNER ((DWORD)-1) + /** + * ID of the thread which holds the lock. + */ + DWORD ThreadId; + /** + * Auto-reset event. + * + * This event is created in signaled state, which means any thread can + * `WaitForSingleObject` on it; only one thread will be released at a time, + * after which this event will be automatically put into non-signaled state. + * + * The released thread owns the lock; it unlocks it by calling `SetEvent` on + * this event, which puts this event into signaled state, allowing system + * release another thread which waits on it. + * + * The cycle repeats until this event is destroyed. + */ + HANDLE Event; +} WinpthreadsSpinlock; + +#define WINPTHREADS_SPINLOCK_INITIALIZER ((WinpthreadsSpinlock *) PTHREAD_SPINLOCK_INITIALIZER) + +/** + * Obtain pointer to `WinpthreadsSpinlock` structure pointed to by `lock`. + * + * If `lock` points to statically initialzied `pthread_spinlock_t` object, + * allocate `WinpthreadsSpinlock` structure and store its address in `*lock`. + * + * On success, stores pointer to `WinpthreadsSpinlock` structure in `*spinlock`. + * + * Returns zero on success and an error-code on failure. + */ +static WINPTHREADS_INLINE int WinpthreadsSpinlockGet (pthread_spinlock_t *lock, WinpthreadsSpinlock **spinlock) +{ + WinpthreadsSpinlock **pSpinlock = (WinpthreadsSpinlock **) lock; + + /** + * POSIX: + * + * If an implementation detects that the value specified by the lock argument + * to pthread_spin_*() does not refer to an initialized spin lock object, + * it is recommended that the function should fail and report an [EINVAL] error. + */ + if (unlikely (pSpinlock == NULL)) { + return EINVAL; + } + + if (*pSpinlock == WINPTHREADS_SPINLOCK_INITIALIZER) { + /** + * We need to avoid race condition when more than one thread calls + * `pthread_spin_[try]lock` on statically initialized `pthread_spinlock_t` + * at the same time. + * + * Store newly initialized spinlock in `spinlock`, which points to local + * variable supplied by the caller, and then store it in `*lock`. + * + * If some other thread was faster then us, destroy newly created spinlock + * and use spinlock pointed to by `lock`. + */ + int error_code = pthread_spin_init ((pthread_spinlock_t *) spinlock, PTHREAD_PROCESS_PRIVATE); + + if (error_code) { + return error_code; + } + + WinpthreadsSpinlock *oldLock = (WinpthreadsSpinlock *) InterlockedCompareExchangePointer ( + (void **) pSpinlock, *spinlock, WINPTHREADS_SPINLOCK_INITIALIZER + ); + + /** + * Some other thread was faster than us. + */ + if (unlikely (oldLock != WINPTHREADS_SPINLOCK_INITIALIZER)) { + pthread_spin_destroy ((pthread_spinlock_t *) spinlock); + *spinlock = oldLock; + } + } else { + *spinlock = *pSpinlock; + } + + if (unlikely (*spinlock == NULL)) { + return EINVAL; + } + + return 0; +} int pthread_spin_init (pthread_spinlock_t *lock, int pshared) { - spinlock_word_t *lk = (spinlock_word_t *)lock; - *lk = -1; + WinpthreadsSpinlock **pSpinlock = (WinpthreadsSpinlock **) lock; + + if (unlikely (pSpinlock == NULL)) { + return EINVAL; + } + + if (unlikely (pshared != PTHREAD_PROCESS_PRIVATE && pshared != PTHREAD_PROCESS_SHARED)) { + return EINVAL; + } + + if (unlikely (pshared == PTHREAD_PROCESS_SHARED)) { + return ENOSYS; + } + + WinpthreadsSpinlock *wSpinlock = calloc (1, sizeof (WinpthreadsSpinlock)); + + /** + * The pthread_spin_init() function shall fail if: + * + * [ENOMEM] + * Insufficient memory exists to initialize the lock. + */ + if (wSpinlock == NULL) { + return ENOMEM; + } + + wSpinlock->ThreadId = THREAD_ID_NO_OWNER; + wSpinlock->Event = CreateEventA (NULL, FALSE, TRUE, NULL); + + /** + * The pthread_spin_init() function shall fail if: + * + * [EAGAIN] + * The system lacks the necessary resources to initialize another spin lock. + */ + if (wSpinlock->Event == NULL) { + free (wSpinlock); + return EAGAIN; + } + + *pSpinlock = wSpinlock; + return 0; } int pthread_spin_destroy (pthread_spinlock_t *lock) { + WinpthreadsSpinlock **pSpinlock = (WinpthreadsSpinlock **) lock; + + /** + * POSIX: + * + * If an implementation detects that the value specified by the lock argument + * to pthread_spin_destroy() does not refer to an initialized spin lock object, + * it is recommended that the function should fail and report an [EINVAL] error. + */ + if (unlikely (pSpinlock == NULL)) { + return EINVAL; + } + + /** + * If `lock` points to statically initialized `pthread_spinlock_t` object, + * this will immediately invalidate it, minimizing the window for + * `pthread_spin_lock` and `pthread_spin_trylock` to attempt using it. + */ + WinpthreadsSpinlock *wSpinlock = InterlockedCompareExchangePointer ( + (void **) pSpinlock, NULL, WINPTHREADS_SPINLOCK_INITIALIZER + ); + + if (unlikely (wSpinlock == NULL)) { + return EINVAL; + } + + if (unlikely (wSpinlock == WINPTHREADS_SPINLOCK_INITIALIZER)) { + return 0; + } + + switch (_pthread_wait_for_single_object (wSpinlock->Event, 0)) { + /** + * `wSpinlock->Event` was in signaled state, which means it was unlocked; + * we are holding the lock now which prevents other threads from locking it. + */ + case WAIT_OBJECT_0: + break; + /** + * `wSpinlock->Event` was in not-signaled state, which means some thread + * holds the lock. + * + * POSIX: + * + * If an implementation detects that the value specified by the lock argument + * to pthread_spin_destroy() or pthread_spin_init() refers to a locked spin + * lock object, or detects that the value specified by the lock argument to + * pthread_spin_init() refers to an already initialized spin lock object, + * it is recommended that the function should fail and report an [EBUSY] error. + */ + case WAIT_TIMEOUT: + return EBUSY; + default: + return EINVAL; + } + + /** + * Invalidate `pthread_spinlock_t` object pointed by `lock`. + */ + InterlockedExchangePointer ((void **) pSpinlock, NULL); + + CloseHandle (wSpinlock->Event); + free (wSpinlock); + return 0; } int pthread_spin_lock (pthread_spinlock_t *lock) { - spinlock_word_t *lk = (spinlock_word_t *)lock; - while (unlikely(InterlockedExchangePointer((PVOID*)lk, 0) == 0)) { - YieldProcessor(); + WinpthreadsSpinlock *wSpinlock = NULL; + + int error_code = WinpthreadsSpinlockGet (lock, &wSpinlock); + + if (error_code) { + return error_code; } + + DWORD threadId = GetCurrentThreadId (); + + /** + * The pthread_spin_lock() function may fail if: + * + * [EDEADLK] + * A deadlock condition was detected. + */ + if (unlikely (wSpinlock->ThreadId == threadId)) { + return EDEADLK; + } + + switch (_pthread_wait_for_single_object (wSpinlock->Event, INFINITE)) { + /** + * We are holding the lock now and `wSpinlock->Event` was reset to + * non-signaled state. + */ + case WAIT_OBJECT_0: + break; + default: + return EINVAL; + } + + wSpinlock->ThreadId = threadId; + return 0; } int pthread_spin_trylock (pthread_spinlock_t *lock) { - spinlock_word_t *lk = (spinlock_word_t *)lock; - return InterlockedExchangePointer((PVOID*)lk, 0) == 0 ? EBUSY : 0; + WinpthreadsSpinlock *wSpinlock = NULL; + + int error_code = WinpthreadsSpinlockGet (lock, &wSpinlock); + + if (error_code) { + return error_code; + } + + /** + * Unlike `pthread_spin_lock`, no deadlock can occur even if calling thread + * owns the lock. + */ + switch (_pthread_wait_for_single_object (wSpinlock->Event, 0)) { + /** + * `wSpinlock->Event` was in signaled state, which means it was unlocked; + * we are holding the lock now and `wSpinlock->Event` was reset to + * non-signaled state. + */ + case WAIT_OBJECT_0: + break; + /** + * `wSpinlock->Event` was in non-signaled state, which means some thread + * holds the lock. + * + * The pthread_spin_trylock() function shall fail if: + * + * [EBUSY] + * A thread currently holds the lock. + */ + case WAIT_TIMEOUT: + return EBUSY; + default: + return EINVAL; + } + + wSpinlock->ThreadId = GetCurrentThreadId (); + + return 0; } int pthread_spin_unlock (pthread_spinlock_t *lock) { - spinlock_word_t *lk = (spinlock_word_t *)lock; -#if defined(__GNUC__) || defined(__clang__) - __atomic_store_n(lk, -1, __ATOMIC_RELEASE); -#else - InterlockedExchangePointer((PVOID*)lk, (void*)-1); -#endif + WinpthreadsSpinlock *wSpinlock = NULL; + + int error_code = WinpthreadsSpinlockGet (lock, &wSpinlock); + + if (error_code) { + return error_code; + } + + DWORD threadId = GetCurrentThreadId (); + + /** + * POSIX: + * + * If an implementation detects that the value specified by the lock argument + * to pthread_spin_unlock() refers to a spin lock object for which the + * current thread does not hold the lock, it is recommended that the function + * should fail and report an [EPERM] error. + */ + if (unlikely (wSpinlock->ThreadId != threadId)) { + return EPERM; + } + + wSpinlock->ThreadId = THREAD_ID_NO_OWNER; + + if (!SetEvent (wSpinlock->Event)) { + return EINVAL; + } + return 0; } diff --git a/mingw-w64-libraries/winpthreads/tests/Makefile.am b/mingw-w64-libraries/winpthreads/tests/Makefile.am index 711b7ff8e..218999d94 100644 --- a/mingw-w64-libraries/winpthreads/tests/Makefile.am +++ b/mingw-w64-libraries/winpthreads/tests/Makefile.am @@ -171,8 +171,3 @@ check_PROGRAMS = \ semaphore/semaphore5 TESTS = $(check_PROGRAMS) - -# TODO: investigate why these tests fail -XFAIL_TESTS = \ - pthread_spinlock/spin1 \ - pthread_spinlock/spin3 -- 2.51.0.windows.1
_______________________________________________ Mingw-w64-public mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
