[Xcb] [PATCH 3/3] fix pthread_cond_wait and get_socket_back dependency

[Christian König]

Hey Jamey & Uli,

this is a ping, can we commit this or do you have further comments?

No idea how I can further convince you that the code is right, but I've 
done quite some testing and it seems to work perfectly fine.

If it really is an interface change then please point me if and where I 
should further document that, but I still think it is just a bugfix.

Cheers,
Christian.

Am 16.05.2013 11:35, schrieb Christian König:
> Am 16.05.2013 02:02, schrieb Jamey Sharp:
>> Thanks for splitting the original patch into these independent parts--it
>> helped a lot for review!
>>
>> After reviewing the code this patch touches, I think there are two
>> possibilities.
>>
>> - Either this patch does not completely address race conditions around
>>    socket ownership;
>>
>> - Or there are existing, interlocking, invariants that make this safe,
>>    and they just need to be better documented.
>>
>> Consider, for example: What happens if xcb_take_socket is called from
>> two threads at once?
>
> Not much, changing socket ownership is protected by the iolock. So 
> when two threads call into xcb_take_socket the thread who can acquire 
> the iolock wins and gets the socket. The other thread gets the iolock 
> after the first returns from xcb_take_socket and so calls 
> return_socket to get the socket back to xcb before giving it out again.
>
> But that's exactly the argument why the current implementation doesn't 
> work correctly, any caller of xcb_take_socket must make sure that the 
> socket isn't stolen by another thread. The conventional way to do so 
> is to use a locking primitive, which is acquired by the caller of 
> xcb_take_socket and inside return_socket.
>
> So calling return_socket with the iolock held (and yes using 
> socket_moving has the same effect of holding the iolock) is just a 
> classic case of acquiring locking primitives in different order.
>
> Christian.
>
>> I'm not excited to merge this patch as-is, because it makes the
>> invariants more obscure than they already were. I would rather see a
>> patch that makes the code obviously correct. As it stands, I can't
>> convince myself that the original code is wrong, and I can't convince
>> myself that the replacement code is right.
>>
>> Thanks,
>> Jamey
>>
>> On Wed, May 15, 2013 at 11:21:37AM +0200, Christian König wrote:
>>> From: Christian König <christian.koenig at amd.com>
>>>
>>> Both functions may drop the iolock, so it is possible that somebody 
>>> else has
>>> acquired the socket or started writing data.
>>>
>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>> ---
>>>   src/xcb_out.c |   16 +++++++++++-----
>>>   1 file changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/xcb_out.c b/src/xcb_out.c
>>> index 429fa99..b9901ca 100644
>>> --- a/src/xcb_out.c
>>> +++ b/src/xcb_out.c
>>> @@ -239,10 +239,12 @@ unsigned int xcb_send_request(xcb_connection_t 
>>> *c, int flags, struct iovec *vect
>>>         /* get a sequence number and arrange for delivery. */
>>>       pthread_mutex_lock(&c->iolock);
>>> +    get_socket_back(c);
>>>       /* wait for other writing threads to get out of my way. */
>>> -    while(c->out.writing)
>>> +    while(c->out.writing) {
>>>           pthread_cond_wait(&c->out.cond, &c->iolock);
>>> -    get_socket_back(c);
>>> +        get_socket_back(c);
>>> +    }
>>>         /* send GetInputFocus (sync_req) when 64k-2 requests have 
>>> been sent without
>>>        * a reply. */
>>> @@ -357,10 +359,12 @@ int _xcb_out_send(xcb_connection_t *c, struct 
>>> iovec *vector, int count)
>>>     void _xcb_out_send_sync(xcb_connection_t *c)
>>>   {
>>> +    get_socket_back(c);
>>>       /* wait for other writing threads to get out of my way. */
>>> -    while(c->out.writing)
>>> +    while(c->out.writing) {
>>>           pthread_cond_wait(&c->out.cond, &c->iolock);
>>> -    get_socket_back(c);
>>> +        get_socket_back(c);
>>> +    }
>>>       send_sync(c);
>>>   }
>>>   @@ -377,8 +381,10 @@ int _xcb_out_flush_to(xcb_connection_t *c, 
>>> uint64_t request)
>>>           c->out.queue_len = 0;
>>>           return _xcb_out_send(c, &vec, 1);
>>>       }
>>> -    while(c->out.writing)
>>> +    while(c->out.writing) {
>>>           pthread_cond_wait(&c->out.cond, &c->iolock);
>>> +        get_socket_back(c);
>>> +    }
>>>       assert(XCB_SEQUENCE_COMPARE(c->out.request_written, >=, 
>>> request));
>>>       return 1;
>>>   }
>>> -- 
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> Xcb mailing list
>>> Xcb at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/xcb
>
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb
>