[Bug nptl/25847] pthread_cond_signal failed to wake up pthread_cond_wait due to a bug in undoing stealing

[triegel at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25847

--- Comment #18 from Torvald Riegel <triegel at redhat dot com> ---
(In reply to Qin Li from comment #0)
> As the G1 might still have several remaining waiters, when new signals come,
> waiters from this damaged G1 will still be woke up. Until the last signal is
> delivered on this G1, we would observe what was shown in the dump above:
> that we posted a signal to a G1, no futex waiter woke up, as __g_refs[G1]
> was already 0 before __g_size[G1] did, and the signal remains not taken. But
> in the meantime there are one or more waiters in G2. Signal is lost, when
> indeed we could have wake up a waiter in G2.

Qin Li, thank you for investigating, isolating the issue, and providing the
reproducer.  I agree that there is a bug in that just incrementing a group's
number of available signals isn't sufficient because it doesn't adjust the
group size (ie, the number of waiters that signalers think are still in this
group) accordingly.

The result is that signalers can put a correct signal in a group that is
already effectively empty even though it's size doesn't show that, which leads
to this signal being "lost".  __condvar_cancel_waiting also updates group size,
for example. 

I also think that a solution would be to handle potential stealing in such a
way that it takes all the steps a pthread_cond_signal would, so including
updating the group's size.  I believe we don't need a full broadcast for that,
which would reset everything basically.

Malte Skarupke's proposed solution of "locking" groups through __g_refs more
broadly could also work in principle.  I'll respond to the patch directly on
libc-alpha.  I'm wondering about the performance implications of this approach,
even though a full pthread_cond_signal could also be costly because it adds
contention to signalers.

(In reply to Torvald Riegel from comment #17)
> (I'm aware of the first
> reproducer posted, but I'm currently looking at it and am not yet convinced
> that it is correct; it sends out more signals than the number of wake-ups it
> allows through the wait condition, AFAICT, which I find surprising.)

I haven't fully wrapped my head around how the critical section implementation
covered in the reproducer works, but I also don't see any concrete red flags
anymore.  What had me surprised is that it really depends on blocking through
the condvar, which I'd say is different from how many concurrent algorithms
treat blocking through OS facilities like futexes as "optional" because the
real functional blocking happens through shared-memory synchronization. 
Second, the AWAKENED_WAITER flag seems to be intended to leak into the waiter
ref count (ie, you can't have the waiter ref count start at a higher bit, the
"overflow" of the flag seems to be intended).

-- 
You are receiving this mail because:
You are on the CC list for the bug.