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

[Kristian Høgsberg]

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.

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

Kristian