This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] New condvar implementation that provides stronger ordering guarantees.
- From: Florian Weimer <fweimer at redhat dot com>
- To: Torvald Riegel <triegel at redhat dot com>, GLIBC Devel <libc-alpha at sourceware dot org>
- Cc: "Carlos O'Donell" <carlos at redhat dot com>, David Miller <davem at davemloft dot net>, Darren Hart <dvhart at infradead dot org>
- Date: Tue, 14 Jun 2016 16:44:44 +0200
- Subject: Re: [PATCH] New condvar implementation that provides stronger ordering guarantees.
- Authentication-results: sourceware.org; auth=none
- References: <1464268895 dot 17104 dot 14 dot camel at localhost dot localdomain>
On 05/26/2016 03:21 PM, Torvald Riegel wrote:
+ /* We have to conservatively undo our potential mistake of stealing
+ a signal. We can stop trying to do that when the current G1
+ changes because other spinning waiters will notice this too and
+ __condvar_quiesce_and_switch_g1 has checked that there are no
+ futex waiters anymore before switching G1.
+ Relaxed MO is fine for the __g1_start load because we need to
+ merely be able to observe this fact and not have to observe
+ something else as well.
+ ??? Would it help to spin for a little while to see whether the
+ current G1 gets closed? This might be worthwhile if the group is
+ small or close to being closed. */
+ unsigned int s = atomic_load_relaxed (cond->__data.__g_signals + g);
+ while (__condvar_load_g1_start_relaxed (cond) == g1_start)
+ {
+ /* Try to add a signal. We don't need to acquire the lock
+ because at worst we can cause a spurious wake-up. If the
+ group is in the process of being closed (LSB is true), this
+ has an effect similar to us adding a signal. */
+ if (((s & 1) != 0)
+ || atomic_compare_exchange_weak_relaxed (
+ cond->__data.__g_signals + g, &s, s + 2))
+ {
+ /* If we added a signal, we also need to add a wake-up on
+ the futex. We also need to do that if we skipped adding
+ a signal because the group is being closed because
+ while __condvar_quiesce_and_switch_g1 could have closed
+ the group, it might stil be waiting for futex waiters to
+ leave (and one of those waiters might be the one we stole
+ the signal from, which cause it to block using the
+ futex). */
+ futex_wake (cond->__data.__g_signals + g, 1, private);
+ break;
+ }
+ /* TODO Back off. */
+ }
I don't quite understand the exit condition for this while loop. How do
we know that __g1_start will change (again) in a timely fashion? Or
alternatively, why is the s variable not reloaded?
I haven't really reviewed this patch. One small nit: GNU style does not
use a parenthesis at the end of a line, as in (among others):
+ uint64_t r = __condvar_fetch_add_64_relaxed (
+ return __condvar_load_64_relaxed (
There are some magic numbers 2/4/8, which might better use symbolic
constants.
pthread_cond_common.c should be a header file (pthread_cond_common.h).
Thanks,
Florian