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

[Kristian Høgsberg]

On Wed, Apr 10, 2013 at 2:29 PM, Uli Schlachter <psychon at znc.in> wrote:
> On 10.04.2013 17:16, Kristian Høgsberg wrote:
> [...]
>> +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;
>> +     }
>> +
>> +     old_state = display->reader_state;
>> +     display->reader_state = LOCKED_READER;
>> +     display->reader = pthread_self();
>> +
>> +     if (old_state == VOLUNTEER_READER) {
>> +             write(display->reader_pipe[1], &c, 1);
>> +             pthread_cond_wait(&display->pipe_cond, &display->mutex);
>> +     }
>> +
>> +     pthread_mutex_unlock(&display->mutex);
>> +
>> +     return display->fd;
>> +}
> [...]
>> +     if (display->reader_state == NO_READER) {
>> +             /* A thread gets here when it's the first reader to
>> +              * try to read and there's no 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);
>> +
>> +             pthread_cond_signal(&display->pipe_cond);
>> +
>> +             if (ret == -1) {
>> +                     display_fatal_error(display, errno);
>> +                     return -1;
>> +             }
> [...]
>
> This still has the "pending byte in the pipe"-problem from earlier, doesn't it?
>
> I mean this one:

Yes, thanks, I knew there was a loose end here.  I sat down and looked
over your VOLUNTEER_READER_EXITING suggestion again and ended up
implementing that.  I think it should work, but thinking about it, we
don't need to force the synchronization between
wl_display_display_acquire_fd() and the exiting volunteer reader.  It
only becomes an issue if there's another volunteer reader trying to
use the pipe, so we can move the synchronization to that point.

To that end I've introduced a pipe_busy flag, and a thread trying to
become volunteer_reader has to wait for that flag to clear. This way,
we don't enforce synchronization unless we have actual contention.
I'll send out the patch now, let me know what you think.  As for the
pthread_cond_wait() early exit case, one of them now is in a while
loop, and the other is can exit early, since that turns into an
wl_display_dispatch_queue() early exit, which is allowed.

Kristian

> Uli Schlachter wrote:
>> Ok, I think I found another problem:
>>
>> - Thread 1 is a volunteer reader, sitting in poll()
>> - The server sends some data and poll() returns because display->fd is readable
>> - Thread 2 calls wl_display_acquire_fd() and reaches the cond_wait(pipe_cond)
>> - Thread 1 reaches the above cond_wait(reader_cond)
>> - Thread 2 normally reads from the connection and wakes up all readers.
>>
>> At this point, everything is OK. No thread has any problems. However, there is
>> still a single byte sitting in the pipe that was not read. So what happens next?
>>
>> - Thread X wants to read from the display and becomes a volunteer reader. Since
>> the pipe is readable, it immediately continues, reads the byte and ends up in
>> the above cond_wait(reader_cond). At this point we have a thread waiting on
>> reader_cond, but no reader which will wake it up (=exactly the situation that
>> inspired this patch, just a lot more unlikely).
>
> Cheers,
> Uli
>
> P.S.: Urgh, I really don't like pthread's spurious wakeups. Thanks for
> remembering them, Jiergir.
> --
> Q: Because it reverses the logical flow of conversation.
> A: Why is putting a reply at the top of the message frowned upon?