From 7edc1353bd8774cf284b5b07e99d8c3f6e5ea47c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Apr 2025 16:51:14 +0200 Subject: [PATCH 1/5] bitlock: improve doctext for g_pointer_bit_unlock_and_set() --- glib/gbitlock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/glib/gbitlock.c b/glib/gbitlock.c index 900897517..f72f64cc1 100644 --- a/glib/gbitlock.c +++ b/glib/gbitlock.c @@ -653,8 +653,8 @@ g_pointer_bit_lock_mask_ptr (gpointer ptr, guint lock_bit, gboolean set, guintpt * @ptr: the new pointer value to set * @preserve_mask: if non-zero, those bits of the current pointer in @address * are preserved. - * Note that the @lock_bit bit will be always set according to @set, - * regardless of @preserve_mask and the currently set value in @address. + * Note that the @lock_bit bit will be always unset regardless of + * @ptr, @preserve_mask and the currently set value in @address. * * This is equivalent to g_pointer_bit_unlock() and atomically setting * the pointer value. From b8f0117b956358d603e7be6139b18e4f130632a8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Apr 2025 16:54:47 +0200 Subject: [PATCH 2/5] bitlock: add g_bit_lock_and_get() and g_bit_unlock_and_set() API These are similar to g_pointer_bit_lock_and_get() and g_pointer_bit_unlock_and_set(). The bitlock API is pretty amazing, as it allows to use a single bit of an integer for locking while using the remaining 31 bits for other purposes. But those other bits always need to be accessed atomically too. There is a use in being able to lock_and_get(), to combine the setting of the lock bit and fetching the new value at once. For one, that can safe additional atomic operations to fetch the value afterwards. But more importantly, it allows to do this change in one atomic operation. Likewise, unlock_and_set() allows to atomically clear the lock bit and set a new value (while also preserving unrelated bits, by using the @preserve_mask parameter). --- glib/gbitlock.c | 182 ++++++++++++++++++++++++++++++---------- glib/gbitlock.h | 11 +++ glib/tests/1bit-mutex.c | 2 + 3 files changed, 149 insertions(+), 46 deletions(-) diff --git a/glib/gbitlock.c b/glib/gbitlock.c index f72f64cc1..ad36a13f3 100644 --- a/glib/gbitlock.c +++ b/glib/gbitlock.c @@ -31,6 +31,7 @@ #include #include +#include "gtestutils.h" #include "gthreadprivate.h" #ifdef G_BIT_LOCK_FORCE_FUTEX_EMULATION @@ -186,6 +187,81 @@ bit_lock_contended_class (gpointer address) #endif #endif +/** + * g_bit_lock_and_get: + * @address: a pointer to an integer + * @lock_bit: a bit value between 0 and 31 + * @out_val: (out) (optional): return location for the new value of the integer + * + * Sets the indicated @lock_bit in @address and atomically returns the new value. + * + * This is like [func@GLib.bit_lock], except it can atomically return the new value at + * @address (right after obtaining the lock). Thus the value returned in @out_val + * always has the @lock_bit set. + * + * Since: 2.86 + **/ +void +g_bit_lock_and_get (gint *address, + guint lock_bit, + gint *out_val) +{ + const guint MASK = 1u << lock_bit; + guint v; + +#ifdef G_ENABLE_DEBUG + g_assert (lock_bit < 32u); +#endif + +#ifdef USE_ASM_GOTO + if (G_LIKELY (!out_val)) + { + while (TRUE) + { + __asm__ volatile goto("lock bts %1, (%0)\n" + "jc %l[contended]" + : /* no output */ + : "r"(address), "r"(lock_bit) + : "cc", "memory" + : contended); + return; + + contended: + { + guint v; + + v = (guint) g_atomic_int_get (address); + if (v & MASK) + { + guint class = bit_lock_contended_class (address); + + g_atomic_int_add (&g_bit_lock_contended[class], +1); + g_futex_wait (address, v); + g_atomic_int_add (&g_bit_lock_contended[class], -1); + } + } + } + } +#endif + +retry: + v = g_atomic_int_or ((guint *) address, MASK); + if (v & MASK) + /* already locked */ + { + guint class = bit_lock_contended_class (address); + + g_atomic_int_add (&g_bit_lock_contended[class], +1); + g_futex_wait (address, v); + g_atomic_int_add (&g_bit_lock_contended[class], -1); + + goto retry; + } + + if (out_val) + *out_val = (gint) (v | MASK); +} + /** * g_bit_lock: * @address: a pointer to an integer @@ -212,52 +288,7 @@ void g_bit_lock (volatile gint *address, gint lock_bit) { - gint *address_nonvolatile = (gint *) address; - -#ifdef USE_ASM_GOTO - retry: - __asm__ volatile goto ("lock bts %1, (%0)\n" - "jc %l[contended]" - : /* no output */ - : "r" (address), "r" (lock_bit) - : "cc", "memory" - : contended); - return; - - contended: - { - guint mask = 1u << lock_bit; - guint v; - - v = (guint) g_atomic_int_get (address_nonvolatile); - if (v & mask) - { - guint class = bit_lock_contended_class (address_nonvolatile); - - g_atomic_int_add (&g_bit_lock_contended[class], +1); - g_futex_wait (address_nonvolatile, v); - g_atomic_int_add (&g_bit_lock_contended[class], -1); - } - } - goto retry; -#else - guint mask = 1u << lock_bit; - guint v; - - retry: - v = g_atomic_int_or (address_nonvolatile, mask); - if (v & mask) - /* already locked */ - { - guint class = bit_lock_contended_class (address_nonvolatile); - - g_atomic_int_add (&g_bit_lock_contended[class], +1); - g_futex_wait (address_nonvolatile, v); - g_atomic_int_add (&g_bit_lock_contended[class], -1); - - goto retry; - } -#endif + g_bit_lock_and_get ((gint *) address, (guint) lock_bit, NULL); } /** @@ -354,6 +385,65 @@ g_bit_unlock (volatile gint *address, } } +/** + * g_bit_unlock_and_set: + * @address: a pointer to an integer + * @lock_bit: a bit value between 0 and 31 + * @val: the new value to set + * @preserve_mask: mask for bits from @address to preserve + * + * This is like [func@GLib.bit_unlock]() but also atomically sets @address to + * @val. + * + * If @preserve_mask is not zero, then the @preserve_mask bits will be + * preserved in @address and are not set to @val. + * + * Note that the @lock_bit bit will be always unset regardless of + * @val, @preserve_mask and the currently set value in @address. + * + * Since: 2.86 + **/ +void +g_bit_unlock_and_set (gint *address, + guint lock_bit, + gint val, + gint preserve_mask) + +{ + const guint MASK = 1u << lock_bit; + +#ifdef G_ENABLE_DEBUG + g_assert (lock_bit < 32u); +#endif + + if (G_UNLIKELY (preserve_mask != 0)) + { + guint old_val; + guint new_val; + + old_val = (guint) g_atomic_int_get (address); + + again: + new_val = ((old_val & ((guint) preserve_mask)) | (((guint) val) & ~((guint) preserve_mask))) & ~MASK; + if (!g_atomic_int_compare_and_exchange_full (address, (gint) old_val, (gint) new_val, (gint *) &old_val)) + goto again; + } + else + { + g_atomic_int_set (address, (gint) (((guint) val) & ~MASK)); + } + + /* Warning: unlocking may allow another thread to proceed and destroy the + * memory that @address points to. We thus must not dereference it anymore. + */ + + { + guint class = bit_lock_contended_class (address); + + if (g_atomic_int_get (&g_bit_lock_contended[class])) + g_futex_wake (address); + } +} /* We emulate pointer-sized futex(2) because the kernel API only * supports integers. diff --git a/glib/gbitlock.h b/glib/gbitlock.h index f44a52c37..1411157d4 100644 --- a/glib/gbitlock.h +++ b/glib/gbitlock.h @@ -34,6 +34,11 @@ G_BEGIN_DECLS GLIB_AVAILABLE_IN_ALL void g_bit_lock (volatile gint *address, gint lock_bit); +GLIB_AVAILABLE_IN_2_86 +void g_bit_lock_and_get (gint *address, + guint lock_bit, + gint *out_val); + GLIB_AVAILABLE_IN_ALL gboolean g_bit_trylock (volatile gint *address, gint lock_bit); @@ -41,6 +46,12 @@ GLIB_AVAILABLE_IN_ALL void g_bit_unlock (volatile gint *address, gint lock_bit); +GLIB_AVAILABLE_IN_2_86 +void g_bit_unlock_and_set (gint *address, + guint lock_bit, + gint new_val, + gint preserve_mask); + GLIB_AVAILABLE_IN_ALL void g_pointer_bit_lock (volatile void *address, gint lock_bit); diff --git a/glib/tests/1bit-mutex.c b/glib/tests/1bit-mutex.c index f6a90f0bd..2904753c7 100644 --- a/glib/tests/1bit-mutex.c +++ b/glib/tests/1bit-mutex.c @@ -47,8 +47,10 @@ #undef g_pointer_bit_unlock_and_set #define g_bit_lock _emufutex_g_bit_lock + #define g_bit_lock_and_get _emufutex_g_bit_lock_and_get #define g_bit_trylock _emufutex_g_bit_trylock #define g_bit_unlock _emufutex_g_bit_unlock + #define g_bit_unlock_and_set _emufutex_g_bit_unlock_and_set #define g_pointer_bit_lock _emufutex_g_pointer_bit_lock #define g_pointer_bit_lock_and_get _emufutex_g_pointer_bit_lock_and_get #define g_pointer_bit_trylock _emufutex_g_pointer_bit_trylock From 893e89d116126ea0bb85a658b92cfeb0fb2d9f4c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Apr 2025 08:24:36 +0200 Subject: [PATCH 3/5] bitlock/tests: add test for using bit lock API in parallel Add a test that runs a few threads, which take a bit lock and collectively increment a counter. --- glib/tests/bitlock.c | 137 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+) diff --git a/glib/tests/bitlock.c b/glib/tests/bitlock.c index a21672099..df30cdfdb 100644 --- a/glib/tests/bitlock.c +++ b/glib/tests/bitlock.c @@ -1,5 +1,7 @@ #include +#include "gtestutils.h" + static void test_bitlocks (void) { @@ -28,12 +30,147 @@ test_bitlocks (void) } } +#define PARALLEL_N_THREADS 5 +#define PARALLEL_LOCKBIT 31 +#define PARALLEL_TOGGLEBIT 30 +#define PARALLEL_SETBIT 29 +#define PARALLEL_LOCKMASK (1 << PARALLEL_LOCKBIT) +#define PARALLEL_TOGGLEMASK (1 << PARALLEL_TOGGLEBIT) +#define PARALLEL_SETMASK (1 << PARALLEL_SETBIT) +#define PARALLEL_MAX_COUNT_SELF 500 +#define PARALLEL_MAX_COUNT_ALL (10 * PARALLEL_MAX_COUNT_SELF) +static int parallel_thread_ready = 0; +static int parallel_atomic = 0; + +static void +_test_parallel_randomly_toggle (void) +{ + if (g_random_boolean ()) + g_atomic_int_or (¶llel_atomic, PARALLEL_TOGGLEMASK); + else + g_atomic_int_and (¶llel_atomic, ~PARALLEL_TOGGLEMASK); +} + +static gpointer +_test_parallel_run (gpointer thread_arg) +{ + const int IDX = GPOINTER_TO_INT (thread_arg); + int count_self = 0; + + (void) IDX; + + g_atomic_int_inc (¶llel_thread_ready); + while (g_atomic_int_get (¶llel_thread_ready) != PARALLEL_N_THREADS) + g_usleep (10); + + while (TRUE) + { + gint val; + gint val2; + gint new_val; + gint count_all; + + _test_parallel_randomly_toggle (); + + /* take a lock. */ + if (g_random_boolean ()) + { + g_bit_lock (¶llel_atomic, PARALLEL_LOCKBIT); + val = g_atomic_int_get (¶llel_atomic); + } + else + { + g_bit_lock_and_get (¶llel_atomic, PARALLEL_LOCKBIT, &val); + } + + _test_parallel_randomly_toggle (); + + /* the toggle bit is random. Clear it. */ + val &= ~PARALLEL_TOGGLEMASK; + + /* these bits must be set. */ + g_assert_true (val & PARALLEL_LOCKMASK); + g_assert_true (val & PARALLEL_SETMASK); + + /* If we fetch again, we must get the same value. Nobody changes the + * value while we hold the lock, except for the toggle bit. */ + val2 = g_atomic_int_get (¶llel_atomic); + val2 &= ~PARALLEL_TOGGLEMASK; + g_assert_cmpint (val, ==, val2); + + count_all = (val & ~(PARALLEL_LOCKMASK | PARALLEL_SETMASK)); + + if ((g_random_int () % 5) == 0) + { + /* regular unlock without any changes. */ + g_bit_unlock (¶llel_atomic, PARALLEL_LOCKBIT); + continue; + } + + /* unlock-and-set with an increased counter. */ + new_val = MIN (count_all + 1, PARALLEL_MAX_COUNT_ALL); + if (g_random_boolean ()) + new_val |= PARALLEL_SETMASK; + if (g_random_boolean ()) + new_val |= PARALLEL_TOGGLEMASK; + + g_bit_unlock_and_set (¶llel_atomic, PARALLEL_LOCKBIT, new_val, ((new_val & PARALLEL_SETMASK) && g_random_boolean ()) ? 0 : PARALLEL_SETMASK); + + count_self++; + + if (count_self < PARALLEL_MAX_COUNT_SELF) + continue; + if (count_all < PARALLEL_MAX_COUNT_ALL) + continue; + + break; + } + + /* To indicate success, we return a pointer to ¶llel_atomic. */ + return ¶llel_atomic; +} + +static void +test_parallel (void) +{ + GThread *threads[PARALLEL_N_THREADS]; + gpointer ptr; + int i; + gint val; + + g_atomic_int_or (¶llel_atomic, PARALLEL_SETMASK); + + for (i = 0; i < PARALLEL_N_THREADS; i++) + { + threads[i] = g_thread_new ("bitlock-parallel", _test_parallel_run, GINT_TO_POINTER (i)); + } + + for (i = 0; i < PARALLEL_N_THREADS; i++) + { + ptr = g_thread_join (threads[i]); + g_assert_true (ptr == ¶llel_atomic); + + /* After we join the first thread, we already expect that the resulting + * atomic's counter is set to PARALLEL_MAX_COUNT_ALL. This stays until + * the end. */ + val = g_atomic_int_get (¶llel_atomic); + if (i == PARALLEL_N_THREADS - 1) + { + /* at last, the atomic must be unlocked. */ + g_assert_true (!(val & PARALLEL_LOCKMASK)); + } + val &= ~(PARALLEL_LOCKMASK | PARALLEL_TOGGLEMASK | PARALLEL_SETMASK); + g_assert_cmpint (val, ==, PARALLEL_MAX_COUNT_ALL); + } +} + int main (int argc, char **argv) { g_test_init (&argc, &argv, NULL); g_test_add_func ("/bitlock/performance/uncontended", test_bitlocks); + g_test_add_func ("/bitlock/performance/parallel", test_parallel); return g_test_run (); } From 493b9c49afccd233c171f33ea28ea2fb4ed599ba Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Apr 2025 17:05:02 +0200 Subject: [PATCH 4/5] bitlock: mark g_futex_int_address() as G_ALWAYS_INLINE It probably would get inlined anyway(?). But add G_ALWAYS_INLINE to be more sure about this. We don't want this mostly trivial function to be a separate call. --- glib/gbitlock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib/gbitlock.c b/glib/gbitlock.c index ad36a13f3..9838144f6 100644 --- a/glib/gbitlock.c +++ b/glib/gbitlock.c @@ -469,7 +469,7 @@ g_bit_unlock_and_set (gint *address, * * g_futex_wake (g_futex_int_address (int_address)); */ -static const gint * +G_ALWAYS_INLINE static inline const gint * g_futex_int_address (const void *address) { const gint *int_address = address; From 90e6e2b52886305fa64c6d7a9c148ad20128971d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Apr 2025 10:25:04 +0200 Subject: [PATCH 5/5] bitlock: factor out duplicated futex wait/wake functions --- glib/gbitlock.c | 89 ++++++++++++++++++++++--------------------------- 1 file changed, 40 insertions(+), 49 deletions(-) diff --git a/glib/gbitlock.c b/glib/gbitlock.c index 9838144f6..a7a53ddd9 100644 --- a/glib/gbitlock.c +++ b/glib/gbitlock.c @@ -176,7 +176,7 @@ g_futex_wake (const gint *address) static gint g_bit_lock_contended[CONTENTION_CLASSES]; /* (atomic) */ G_ALWAYS_INLINE static inline guint -bit_lock_contended_class (gpointer address) +bit_lock_contended_class (gconstpointer address) { return ((gsize) address) % G_N_ELEMENTS (g_bit_lock_contended); } @@ -187,6 +187,37 @@ bit_lock_contended_class (gpointer address) #endif #endif +static const gint *g_futex_int_address (const void *address); + +G_ALWAYS_INLINE static inline void +bit_lock_futex_wait (gconstpointer address, gboolean is_pointer_pointer, gint value) +{ + const guint CLASS = bit_lock_contended_class (address); + + g_atomic_int_add (&g_bit_lock_contended[CLASS], +1); + if (is_pointer_pointer) + address = g_futex_int_address (address); + g_futex_wait (address, value); + g_atomic_int_add (&g_bit_lock_contended[CLASS], -1); +} + +G_ALWAYS_INLINE static inline void +bit_lock_futex_maybe_wake (gconstpointer address, gboolean is_pointer_pointer) +{ + const guint CLASS = bit_lock_contended_class (address); + + /* Warning: unlocking may allow another thread to proceed and destroy the + * memory that @address points to. We thus must not dereference it anymore. + */ + + if (g_atomic_int_get (&g_bit_lock_contended[CLASS])) + { + if (is_pointer_pointer) + address = g_futex_int_address (address); + g_futex_wake (address); + } +} + /** * g_bit_lock_and_get: * @address: a pointer to an integer @@ -232,13 +263,7 @@ g_bit_lock_and_get (gint *address, v = (guint) g_atomic_int_get (address); if (v & MASK) - { - guint class = bit_lock_contended_class (address); - - g_atomic_int_add (&g_bit_lock_contended[class], +1); - g_futex_wait (address, v); - g_atomic_int_add (&g_bit_lock_contended[class], -1); - } + bit_lock_futex_wait (address, FALSE, (gint) v); } } } @@ -247,14 +272,8 @@ g_bit_lock_and_get (gint *address, retry: v = g_atomic_int_or ((guint *) address, MASK); if (v & MASK) - /* already locked */ { - guint class = bit_lock_contended_class (address); - - g_atomic_int_add (&g_bit_lock_contended[class], +1); - g_futex_wait (address, v); - g_atomic_int_add (&g_bit_lock_contended[class], -1); - + bit_lock_futex_wait (address, FALSE, (gint) v); goto retry; } @@ -376,13 +395,7 @@ g_bit_unlock (volatile gint *address, /* Warning: unlocking may allow another thread to proceed and destroy the * memory that @address points to. We thus must not dereference it anymore. */ - - { - guint class = bit_lock_contended_class (address_nonvolatile); - - if (g_atomic_int_get (&g_bit_lock_contended[class])) - g_futex_wake (address_nonvolatile); - } + bit_lock_futex_maybe_wake (address_nonvolatile, FALSE); } /** @@ -436,13 +449,7 @@ g_bit_unlock_and_set (gint *address, /* Warning: unlocking may allow another thread to proceed and destroy the * memory that @address points to. We thus must not dereference it anymore. */ - - { - guint class = bit_lock_contended_class (address); - - if (g_atomic_int_get (&g_bit_lock_contended[class])) - g_futex_wake (address); - } + bit_lock_futex_maybe_wake (address, FALSE); } /* We emulate pointer-sized futex(2) because the kernel API only @@ -534,7 +541,6 @@ void guint lock_bit, guintptr *out_ptr) { - guint class = bit_lock_contended_class (address); guintptr mask; guintptr v; @@ -558,11 +564,7 @@ void contended: v = (guintptr) g_atomic_pointer_get ((gpointer *) address); if (v & mask) - { - g_atomic_int_add (&g_bit_lock_contended[class], +1); - g_futex_wait (g_futex_int_address (address), v); - g_atomic_int_add (&g_bit_lock_contended[class], -1); - } + bit_lock_futex_wait (address, TRUE, (gint) v); } } #endif @@ -570,11 +572,8 @@ void retry: v = g_atomic_pointer_or ((gpointer *) address, mask); if (v & mask) - /* already locked */ { - g_atomic_int_add (&g_bit_lock_contended[class], +1); - g_futex_wait (g_futex_int_address (address), (guint) v); - g_atomic_int_add (&g_bit_lock_contended[class], -1); + bit_lock_futex_wait (address, TRUE, (gint) v); goto retry; } @@ -696,13 +695,7 @@ void /* Warning: unlocking may allow another thread to proceed and destroy the * memory that @address points to. We thus must not dereference it anymore. */ - - { - guint class = bit_lock_contended_class (address_nonvolatile); - - if (g_atomic_int_get (&g_bit_lock_contended[class])) - g_futex_wake (g_futex_int_address (address_nonvolatile)); - } + bit_lock_futex_maybe_wake (address_nonvolatile, TRUE); } } @@ -762,7 +755,6 @@ void (g_pointer_bit_unlock_and_set) (void *address, guintptr preserve_mask) { gpointer *pointer_address = address; - guint class = bit_lock_contended_class (address); gpointer ptr2; g_return_if_fail (lock_bit < 32u); @@ -782,8 +774,7 @@ void (g_pointer_bit_unlock_and_set) (void *address, g_atomic_pointer_set (pointer_address, ptr2); } - if (g_atomic_int_get (&g_bit_lock_contended[class]) > 0) - g_futex_wake (g_futex_int_address (address)); + bit_lock_futex_maybe_wake (address, TRUE); /* It makes no sense, if unlocking mangles the pointer. Assert against * that.