[Xcb] [PATCH v2 2/2] Also signal special event waiters in _xcb_in_wake_up_next_reader

[Uli Schlachter]

Am 01.06.2015 um 04:09 schrieb Michel Dänzer:
> On 30.05.2015 21:22, Uli Schlachter wrote:
>> Am 25.05.2015 um 10:52 schrieb Michel Dänzer:
>>> From: Michel Dänzer <michel.daenzer at amd.com>
>>>
>>> Fixes occasional deadlock when quitting the Mesa demo glthreads with
>>> DRI3/Present.
>>>
>>> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
>>> ---
>>>
>>> v2: Fix indentation of closing curly brace.
>>>
>>>  src/xcb_in.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/src/xcb_in.c b/src/xcb_in.c
>>> index 322bed8..9a8cae1 100644
>>> --- a/src/xcb_in.c
>>> +++ b/src/xcb_in.c
>>> @@ -890,7 +890,14 @@ void _xcb_in_wake_up_next_reader(xcb_connection_t *c)
>>>      if(c->in.readers)
>>>          pthreadret = pthread_cond_signal(c->in.readers->data);
>>>      else
>>> +    {
>>> +        xcb_special_event_t *se;
>>> +
>>>          pthreadret = pthread_cond_signal(&c->in.event_cond);
>>> +        for (se = c->in.special_events; se; se = se->next) {
>>> +             pthreadret = pthread_cond_signal(&se->special_event_cond);

btw: This should be

   pthreadret |= ...

>>> +        }
>>> +    }
>>>      assert(pthreadret == 0);
>>>  }
>>
>> Hrm. This is basically a pthread_cond_broadcast() waking up all threads, even
>> those for which we do not have a special event.
>>
>> Can you explain why this is necessary at all? The function event_special()
>> should be handling this. This is called on all special events. It appends the
>> event to the xcb_special_event_t's event queue and then calls
>> pthread_cond_signal() on exactly this condition variable.
>>
>> In my understanding, that should be enough. Why would we need a "wake up all
>> waiting threads and let them figure out if they have to do something themselves"?
> 
> I can't explain why, but what you're saying applies to "normal" events
> as well, doesn't it? _xcb_in_wake_up_next_reader() signals
> c->in.event_cond, even though read_packet() already does the same. This
> change is mirroring that for special events.

Can you try changing your above pthread_cond_signal() into a
pthread_cond_broadcast? Alternatively, change the pthread_cond_signal() in
event_special() to pthread_cond_broadcast() and see if that helps.

(The idea being: There are multiple threads in xcb_wait_for_special_event()
concurrently.)

I tried the reproducing program from [1] and while I can also reproduce the hang
that your first patch fixes (and I can confirm that the patch fixes the
problem), I cannot reproduce any hang that this patch would have to fix.

(But this program has only two threads where one thread doesn't call any of
xcb's special_event code. Perhaps if that program where extended to a greater
level of parallelism where multiple threads wait for special events at the same
time?)

Since this is threading-related, something like "I can't explain why" doesn't
really satisfy for fixing a missed wakeup, sorry.

Cheers,
Uli

[1]: https://bugs.freedesktop.org/show_bug.cgi?id=84252
-- 
“I’m Olaf and I like warm hugs.”