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

[Thiago Macieira]

On segunda-feira, 25 de março de 2013 13.42.31, Kristian Høgsberg wrote:
> which returns the display fd and blocks any thread trying to read from
> the fd.  Calling wl_display_dispatch() will read events, release the fd
> and the dispatch events.  Alternatively,  if after acquiring and polling
> the fd, a thread decides to not call wl_display_dispatch() after all,
> the fd can be released by calling wl_display_release_fd().  This is
> typically useful when poll returns without data on the fd, eg in case of
> timeout.

Wouldn't it be better to require calling display_release in all cases? That 
is, avoid the magic of releasing the file descriptor inside 
wl_display_dispatch?

This would add support for dispatching more than once while in a consistent 
state.

>  /** \endcond */
> @@ -518,6 +529,10 @@ wl_display_connect_to_fd(int fd)
>  	wl_event_queue_init(&display->queue, display);
>  	wl_list_init(&display->event_queue_list);
>  	pthread_mutex_init(&display->mutex, NULL);
> +	pthread_cond_init(&display->reader_cond, NULL);
> +	pthread_cond_init(&display->pipe_cond, NULL);
> +	display->reader_state = LEGACY_READER;
> +	display->reader = pthread_self();
> 
>  	wl_map_insert_new(&display->objects, WL_MAP_CLIENT_SIDE, NULL);
> 
> @@ -540,9 +555,118 @@ wl_display_connect_to_fd(int fd)
>  		return NULL;
>  	}
> 
> +	if (pipe2(display->reader_pipe, O_CLOEXEC) == -1) {
> +		wl_connection_destroy(display->connection);
> +		wl_map_release(&display->objects);
> +		close(display->fd);
> +		free(display);

Need to pthread_cond_destroy the two condition variables here.

> + * by calling wl_display_dispatch().  Simplified, we have:
> + *
> + *   wl_display_dispatch_pending(display);
> + *   poll(fds, nfds, -1);
> + *   wl_display_dispatch(display);
> + *

> + * The other race is immediately after poll(), where another thread
> + * could preempt and read events before the main thread calls
> + * wl_display_dispatch().  This call now blocks and starves the other
> + * fds in the event loop.

This one could be solved by calling wl_dispatch_pending in all cases, couldn't 
it?

> +WL_EXPORT int
> +wl_display_acquire_fd(struct wl_display *display)
> +{
> +	char c = 0;
> +	int old_state;
> +
> +	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;

Do we need the pthread_equal check? Just let it return EBUSY if someone tries 
to acquire twice in the same thread. You're not counting the number of locks, 
so this could lead to an unexpected releasing if someone is nesting acquires.

> +WL_EXPORT void
> +wl_display_release_fd(struct wl_display *display)
> +{
> +	pthread_mutex_lock(&display->mutex);
> +
> +	if (display->reader_state != LOCKED_READER ||
> +	    !pthread_equal(display->reader, pthread_self())) {
> +		pthread_mutex_unlock(&display->mutex);
> +		return;
> +	}

Ditto. If someone releases without acquiring (or releases from the wrong 
thread), their code is wrong.

> @@ -597,6 +721,8 @@ wl_display_disconnect(struct wl_display *display)
>  	pthread_mutex_destroy(&display->mutex);
>  	if (display->fd > 0)
>  		close(display->fd);
> +	close(display->reader_pipe[0]);
> +	close(display->reader_pipe[1]);

Missing pthread_cond_destroy here too.

> @@ -847,45 +973,105 @@ dispatch_event(struct wl_display *display, struct
> wl_event_queue *queue) }
> 
>  static int
> -dispatch_queue(struct wl_display *display,
> -	       struct wl_event_queue *queue, int block)
> +read_events(struct wl_display *display, struct wl_event_queue *queue)
>  {
> -	int len, size, count, ret;
> -
> -	pthread_mutex_lock(&display->mutex);
> -
> -	if (display->last_error)
> -		goto err_unlock;
> +	struct pollfd pfd[2];
> +	int len, size, ret;
> +	char c;
> +
> +	if (display->reader_state == NO_READER) {
> +		/* A thread gets here when it's the first reader to
> +		 * try to read and there's LOCKED_READER.  We set

"and there's [no] LOCKED_READER", I guess?

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20130325/d4c99b59/attachment-0001.pgp>