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

Uli Schlachter psychon at znc.in
Thu Apr 11 00:53:58 PDT 2013


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.

> +	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).

> +		}
>
> -	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. :-)

Cheers,
Uli


More information about the wayland-devel mailing list