[PATCH] client: Add acquire-fd API to avoid requiring a polling main thread

[Uli Schlachter]

Hi,

On 12.04.2013 22:27, Kristian Høgsberg wrote:
> On Thu, Apr 11, 2013 at 09:53:58AM +0200, Uli Schlachter wrote:
>> On 10.04.2013 23:55, Kristian Høgsberg wrote:
>> [...]
>>> +WL_EXPORT int
>>> +wl_display_acquire_fd(struct wl_display *display)
>>> +{
>>> +	char c = 0;
>>> +
>>> +	pthread_mutex_lock(&display->mutex);
>>> +
>>> +	if (display->reader_state == LOCKED_READER &&
>>> +	    !pthread_equal(display->reader, pthread_self())) {
>>> +		errno = EBUSY;
>>> +		pthread_mutex_unlock(&display->mutex);
>>> +		return -1;
>>> +	}
>>> +
>>> +	pthread_cond_signal(&display->pipe_cond);
>>
>> I feel light I am missing something, so: Why does this call
>> cond_signal() here? The only threads which might be waiting on this
>> condition are waiting for the volunteer reader to exit and they will
>> not be able to progress until that one unsets pipe_busy. So the
>> signal() here won't do anything bad, but also none of the threads
>> waking up will be able to do anything.
> 
> Oh, I guess the code is ordered a little unintuitively there... we set
> reader_state = LOCKED_READER further down, which should also wake up a
> thread waiting for the pipe.  When a locked reader turns up, there's
> no need for a volunteer and the waiting thread can go straight to the
> pthread_cond_wait(reader_cond) case instead of waiting for the pipe.

Ah, ok. I was thinking that the volunteer reader would do this when it exits,
but it works here, too. But then this should be a broadcast instead of a signal,
too. (Just as below)

>>> +	if (display->reader_state == VOLUNTEER_READER)
>>> +		write(display->reader_pipe[1], &c, 1);
>>> +
>>> +	display->reader = pthread_self();
>>> +	display->reader_state = LOCKED_READER;
>>> +
>>> +	pthread_mutex_unlock(&display->mutex);
>>> +
>>> +	return display->fd;
>>> +}
>> [...]
>>> +		pthread_mutex_lock(&display->mutex);
>>> +
>>> +		display->pipe_busy = 0;
>>> +		if (display->reader_state != VOLUNTEER_READER) {
>>> +			read(display->reader_pipe[0], &c, 1);
>>> +			pthread_cond_signal(&display->pipe_cond);
>>
>> Why is this a cond_signal()? If more than one thread is waiting on
>> pipe_cond, all of them should be waken up. One of them will/might
>> become a volunteer reader, the others should end up waiting on
>> reader_cond.
>>
>> (Otherwise the volunteer reader might read something useful for one
>> of the threads, but this thread is still waiting on pipe_cond).
> 
> Yup, should be broadcast there, thanks.
> 
>>> +		}
>>>
>>> -	ret = wl_connection_flush(display->connection);
>>> -	if (ret < 0 && errno != EAGAIN) {
>>> -		display_fatal_error(display, errno);
>>> -		goto err_unlock;
>>> +		if (ret == -1) {
>>> +			display_fatal_error(display, errno);
>>> +			return -1;
>>> +		}
>>>  	}
>> [...]
>>
>> The synchronization in this patch looks solid to me. I will think
>> about it a little more, but I didn't yet manage to really break it.
>> Yay. :-)
> 
> Thanks again for reviewing.

Thanks for standing my review. :-)

Uli
-- 
Sent from my Game Boy.