[PATCH] client: Add lock-fd API to avoid requiring a polling main thread
Uli Schlachter
psychon at znc.in
Thu Mar 21 09:13:47 PDT 2013
Hi,
On 21.03.2013 15:20, Kristian Høgsberg wrote:
> The current thread model assumes that the application or toolkit will have
> a thread that either polls the display fd and dispatches events or just
> dispatches in a loop. Only the main thread will read from the fd while
> all other threads will block on a pthread condition and expect the main
> thread to deliver events to them.
>
> This turns out to be too restrictive. Qt QML threaded rendering will
> block the main thread on a condition that's signaled by a rendering
> thread after it finishes rendering. This leads to a deadlock when the
> rendering threads blocks in eglSwapBuffers(), and the main thread is
> waiting on the condition. Another problematic use case is with games
> that has a rendering thread for a splash screen while the main thread
> is busy loading game data.
>
> The solution seems pretty straightforward: just let all threads read
> from the fd. The main-thread restriction was introduced to avoid a
> race, however. Simplified, main loops will do this:
>
> wl_display_dispatch_pending(display);
>
> /* Race here if other thread reads from fd and places events
> * in main eent queue. We go to sleep in poll while sitting on
> * events that may stall the application if not dispatched. */
>
> poll(fds, nfds, -1);
>
> /* Race here if other thread reads and doesn't queue any
> * events for main queue. wl_display_dispatch() below will block
> * trying to read from the fd, while other fds in the mainloop
> * are ignored. */
>
> wl_display_dispatch(display);
>
> The restriction that only the main thread can read from the fd avoids
> these races, but has the problems described above.
>
> This patch introduces new API to solve both problems. We add
>
> int wl_display_lock_fd(struct wl_display *display);
>
> which returns the display fd and blocks any thread trying to read from
> the fd. The client library starts out in the legacy mode, where we assume
> the dedicated event thread. Calling wl_display_lock_fd() will enable
> the new all-threads-can-read semantics.
> ---
> src/wayland-client.c | 236 ++++++++++++++++++++++++++++++++++++++++++++++-----
> src/wayland-client.h | 3 +
> 2 files changed, 216 insertions(+), 23 deletions(-)
>
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index 0873835..8a26600 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -21,6 +21,8 @@
> * OF THIS SOFTWARE.
> */
>
> +#define _GNU_SOURCE
> +
> #include <stdlib.h>
> #include <stdint.h>
> #include <stddef.h>
> @@ -73,6 +75,15 @@ struct wl_event_queue {
> pthread_cond_t cond;
> };
>
> +struct waiter {
> + pthread_cond_t cond;
> + struct wl_list link;
> +};
> +
> +enum {
> + NO_READER, VOLUNTEER_READER, LOCKED_READER, LEGACY_READER
> +};
> +
> struct wl_display {
> struct wl_proxy proxy;
> struct wl_connection *connection;
> @@ -83,6 +94,10 @@ struct wl_display {
> struct wl_event_queue queue;
> struct wl_list event_queue_list;
> pthread_mutex_t mutex;
> + pthread_t reader;
> + struct wl_list read_waiter_list;
> + int reader_pipe[2];
> + int reader_state;
> };
>
> /** \endcond */
> @@ -518,6 +533,8 @@ 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);
> + wl_list_init(&display->read_waiter_list);
> + display->reader_state = LEGACY_READER;
Should this initialize ->reader? What makes sure that there is no random garbage
in that variable which just happens to match some thread's id? (Sorry if this is
allocated via calloc() or something like that)
> wl_map_insert_new(&display->objects, WL_MAP_CLIENT_SIDE, NULL);
>
> @@ -540,9 +557,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);
> + return NULL;
> + }
Where does this pipe get closed again? Is the fd leaked when the wl connection
is disconnected?
> return display;
> }
>
> +/** Ensure exclusive access to display file descriptor
> + *
> + * \param display The display context object
> + * \return Display object file descriptor or -1 if another thread
> + * locked the file descriptor.
> + *
> + * This locks the file descriptor for the display, ensuring no other
> + * threads will read from it between returning from this function and
> + * calling wl_display_dispatch().
> + *
> + * Use this function to integrate the display fd into a toolkit event
> + * loop in a race-free way. Typically, a toolkit will call
> + * wl_display_dispatch_pending() before sleeping, to make sure it
> + * doesn't block with unhandled events. Upon waking up, it will
> + * assume the file descriptor is readable and read events from the fd
> + * by calling wl_display_dispatch(). Simplified, we have:
> + *
> + * wl_display_dispatch_pending(display);
> + * poll(fds, nfds, -1);
> + * wl_display_dispatch(display);
> + *
> + * There are two races here: first, before blocking in poll(), the fd
> + * could become readable and another thread reads the events. Some of
> + * these events may be for the main queue and the other thread will
> + * queue them there and then the main thread will go to sleep in
> + * poll(). This will stall the application, which could be waiting
> + * for a event to kick of the next animation frame, for example.
> + *
> + * 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.
> + *
> + * Calling wl_display_lock_fd() ensures that no other thread will read
> + * the display fd. This means that no events will be queued between
> + * dispatching pending events and going to sleep in the event loop and
> + * that no other thread will read from the fd when data becomes
> + * available. Calling wl_display_dispatch() will read events, unlock
> + * the fd and then dispatch new main queue events, if any.
I wonder if it really is a good idea to have wl_display_dispatch() automatically
unlock the fd. From the implementation, it looks like it doesn't make things
more complicated if users always have to match wl_display_lock_fd() with a
suitable call to wl_display_release_fd()?
This would mean that just a single implementation of wl_display_release_fd() is
needed and the duplicate in read_events() below can be removed.
Can anyone come up with a valid reason for this besides my "feels better to me"?
> + * If another thread has locked the file descriptor, this function
> + * returns -1 and errno is set to EBUSY.
How would EBUSY be handled? This shouldn't make anything fail and thus things
get complicated...
> + * \memberof wl_display
> + */
> +WL_EXPORT int
> +wl_display_lock_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;
> + }
> +
> + if (display->reader_state == VOLUNTEER_READER)
> + write(display->reader_pipe[1], &c, 1);
> +
> + display->reader_state = LOCKED_READER;
> + display->reader = pthread_self();
> +
> + pthread_mutex_unlock(&display->mutex);
> +
> + return display->fd;
> +}
> +
> +/** Release exclusive access to display file descriptor
> + *
> + * \param display The display context object
> + *
> + * This releases the exclusive access. Useful for canceling the lock
> + * when a timed out poll returns fd not readable and we're not going
> + * to read from the fd anytime soon.
> + *
> + * \memberof wl_display
> + */
> +WL_EXPORT void
> +wl_display_release_fd(struct wl_display *display)
> +{
> + struct waiter *w;
> +
> + pthread_mutex_lock(&display->mutex);
> +
> + if (display->reader_state != LOCKED_READER ||
> + !pthread_equal(display->reader, pthread_self())) {
> + pthread_mutex_unlock(&display->mutex);
> + return;
> + }
> +
> + display->reader_state = NO_READER;
> + wl_list_for_each(w, &display->read_waiter_list, link)
> + pthread_cond_signal(&w->cond);
> + wl_list_init(&display->read_waiter_list);
> +
> + pthread_mutex_unlock(&display->mutex);
> +}
> +
> /** Connect to a Wayland display
> *
> * \param name Name of the Wayland display to connect to
> @@ -847,45 +973,107 @@ 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;
> -
> - ret = wl_connection_flush(display->connection);
> - if (ret < 0 && errno != EAGAIN) {
> - display_fatal_error(display, errno);
> - goto err_unlock;
> + struct pollfd pfd[2];
> + int len, size, ret;
> + struct waiter waiter, *w;
> + 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
> + * VOLUNTEER_READER and poll on the fd and the
> + * reader_pipe, so that a locked reader can preempt if
> + * it needs to. */
> + display->reader = pthread_self();
> + display->reader_state = VOLUNTEER_READER;
> +
> + pthread_mutex_unlock(&display->mutex);
> +
> + pfd[0].fd = display->fd;
> + pfd[0].events = POLLIN;
> + pfd[1].fd = display->reader_pipe[0];
> + pfd[1].events = POLLIN;
> + ret = poll(pfd, 2, -1);
> + if (pfd[1].revents & POLLIN)
> + read(display->reader_pipe[0], &c, 1);
> +
> + pthread_mutex_lock(&display->mutex);
> +
> + if (ret == -1)
> + return -1;
> +
> + /* If we come back from poll in NO_READER state,
> + * another thread locked and then released the fd.
> + * Nobody is going to read anything in this state
> + * so return instead of blocking without a reader. */
> + if (display->reader_state == NO_READER)
> + return 0;
Let's get creative:
- Thread 1 becomes a VOLUNTEER_READER and sits in poll().
[reader = 1, reader_state = VOLUNTEER]
- Thread 2 locks the fd and writes to the pipe.
[reader = 2, reader_state = LOCKED]
- Thread 2 does its thing and unlocks the fd again.
[reader = 2, reader_state = NO_READER]
- Thread 3 becomes a VOLUNTEER_READER, calls poll(), sees that the reader_pipe
is readable, reads from the pipe, proceeds to read from the FD
[reader = 3, reader_state = NO_READER]
- Finally, Thread 1 gets scheduled again and is blocked in read()ing from the pipe.
The above had a thread other than the VOLUNTEER_READER read from the pipe and
thus a "wakeup byte" got lost.
After thinking about this for 0.5 seconds, I don't have any good suggestions on
how to handle this. And I don't feel like thinking right now... :-)
> }
>
> - if (block && wl_list_empty(&queue->event_list) &&
> - pthread_equal(display->display_thread, pthread_self())) {
> + if (pthread_equal(display->reader, pthread_self())) {
> + /* At this point we have a reader. Either a
> + * VOLUNTEER_READER and the fd is readable or a
> + * LOCKED_READER which is allowed to block in
> + * wl_connection_read(). */
> len = wl_connection_read(display->connection);
> if (len == -1) {
> display_fatal_error(display, errno);
> - goto err_unlock;
> + return -1;
> } else if (len == 0) {
> display_fatal_error(display, EPIPE);
> - goto err_unlock;
> + return -1;
> }
> while (len >= 8) {
> size = queue_event(display, len);
> if (size == -1) {
> display_fatal_error(display, errno);
> - goto err_unlock;
> + return -1;
> } else if (size == 0) {
> break;
> }
> len -= size;
> }
> - } else if (block && wl_list_empty(&queue->event_list)) {
> - pthread_cond_wait(&queue->cond, &display->mutex);
> - if (display->last_error)
> +
> + /* Indicate we're done by setting NO_READER and wake
> + * up other threads waiting to read. */
> + if (display->reader_state != LEGACY_READER)
> + display->reader_state = NO_READER;
> + wl_list_for_each(w, &display->read_waiter_list, link)
> + pthread_cond_signal(&w->cond);
This is the only (real) use of "read_waiter_list" and this one can trivially be
replaced with pthread_cond_broadcast(). So I'd propose to remove the whole list
and replace it with a single condition variable. Or did I miss something?
> + wl_list_init(&display->read_waiter_list);
> + } else {
> + /* Some other thread signed up to read from the fd, so
> + * just add ourselves to the waiter list and wait for
> + * the reader to do the work. */
> + pthread_cond_init(&waiter.cond, NULL);
> + wl_list_insert(display->read_waiter_list.prev, &waiter.link);
> + pthread_cond_wait(&waiter.cond, &display->mutex);
I think this needs to remove the new entry from the list again. Otherwise the
list contains pointers to random addresses on the stack and Bad Things(tm) will
happen.
> + }
> +
> + return 0;
> +}
> +
> +static int
> +dispatch_queue(struct wl_display *display,
> + struct wl_event_queue *queue, int block)
> +{
> + int ret, count;
> +
> + pthread_mutex_lock(&display->mutex);
> +
> + if (display->last_error)
> + goto err_unlock;
> +
> + ret = wl_connection_flush(display->connection);
> + if (ret < 0 && errno != EAGAIN) {
> + display_fatal_error(display, errno);
> + goto err_unlock;
> + }
> +
> + if (block && wl_list_empty(&queue->event_list)) {
> + if (read_events(display, queue) == -1)
> goto err_unlock;
> }
>
> @@ -974,7 +1162,8 @@ wl_display_dispatch_queue_pending(struct wl_display *display,
> WL_EXPORT int
> wl_display_dispatch(struct wl_display *display)
> {
> - display->display_thread = pthread_self();
> + if (display->reader_state == LEGACY_READER)
> + display->reader = pthread_self();
>
> return dispatch_queue(display, &display->queue, 1);
> }
> @@ -1020,7 +1209,8 @@ wl_display_dispatch(struct wl_display *display)
> WL_EXPORT int
> wl_display_dispatch_pending(struct wl_display *display)
> {
> - display->display_thread = pthread_self();
> + if (display->reader_state == LEGACY_READER)
> + display->reader = pthread_self();
>
> return dispatch_queue(display, &display->queue, 0);
> }
> diff --git a/src/wayland-client.h b/src/wayland-client.h
> index 578fa7e..7d8e421 100644
> --- a/src/wayland-client.h
> +++ b/src/wayland-client.h
> @@ -152,6 +152,9 @@ int wl_display_flush(struct wl_display *display);
> int wl_display_roundtrip(struct wl_display *display);
> struct wl_event_queue *wl_display_create_queue(struct wl_display *display);
>
> +int wl_display_lock_fd(struct wl_display *display);
> +void wl_display_release_fd(struct wl_display *display);
As mentioned by jadahl on IRC: The opposite of "lock" is "unlock" while
"release" goes with "acquire".
> void wl_log_set_handler_client(wl_log_func_t handler);
>
> #ifdef __cplusplus
Cheers,
Uli
--
"In the beginning the Universe was created. This has made a lot of
people very angry and has been widely regarded as a bad move."
More information about the wayland-devel
mailing list