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: Torvald Riegel <triegel at redhat dot com>
- To: Florian Weimer <fweimer at redhat dot com>
- Cc: GLIBC Devel <libc-alpha at sourceware dot org>, "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 20:15:07 +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> <9ba4528e-6c48-f832-825a-bdc68c37eb47 at redhat dot com>
On Tue, 2016-06-14 at 16:44 +0200, Florian Weimer wrote:
> 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?
The loop above is just the retry loop should the CAS fail. If
cond->__data.__g1_start changes, we're done because somebody closed the
group. If it doesn't change, then we'll keep trying to add a signal
until the group gets closed. This will eventually succeed (or the group
will be closed, in which case we do wake up and break out of the loop).
> Or
> alternatively, why is the s variable not reloaded?
It is. C11's compare_exchange updates the expected value if the it the
comparison failed (that's why we pass in the address of s).
> I haven't really reviewed this patch.
Thanks for taking a look nonetheless.
> 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 (
What's the expected solution, parenthesis on the next line?
> There are some magic numbers 2/4/8, which might better use symbolic
> constants.
I considered this, but kind of preferred the short numbers. If we
prefer constants with names, I'll change that.
> pthread_cond_common.c should be a header file (pthread_cond_common.h).
There are many cases of C files being included across all of glibc. Are
there precise rules for when to pick .h vs. .c? pthread_cond_common.c
doesn't have just declarations or small helper functions, so .c seemd to
be the right choice.