Wayland client library thread safety

Mon Apr 30 13:07:45 PDT 2012

On Mon, Apr 30, 2012 at 3:58 PM, Kristian Høgsberg <hoegsberg at gmail.com> wrote:
> On Fri, Apr 27, 2012 at 11:09:55PM +0200, Arnaud Vrac wrote:
>> On Fri, Apr 20, 2012 at 10:06 PM, Kristian Hoegsberg
>> <hoegsberg at gmail.com> wrote:
>> > On Thu, Apr 19, 2012 at 03:38:39PM +0200, Arnaud Vrac wrote:
>> >> Hello everyone,
>> >>
>> >> I am hitting a bug when using Qt5 and wayland on an embedded platform,
>> >> for which I have written a custom EGL backend. The problem is that Qt5
>> >> (QtQuick2 actually) renders in a separate thread, when the wayland
>> >> display has been created in the main thread. I know there is a patch
>> >> for "thread affinity" in qtwayland to solve this, but it is wrong and
>> >> breaks other clients.
>> >>
>> >> Since it is required from the EGL spec to be able to render in
>> >> multiple threads, how can this be solved ? Does the wayland backend
>> >> for mesa support this properly ? I don't see how it can work since
>> >> even writes in the connection queue are not protected.
>> >
>> > Hi Arnaud,
>> >
>> > It's a problem that's been around for a while and we're aware of it.
>> > I've been working with Jørgen Lind from Nokias Qt team on it and we're
>> > looking at two different approaches for solving this.
>> >
>> > Initial assumption for wl_display and related objects was that it is
>> > not going to be thread safe, and you have to lock access to the wl API
>> > yourself if you're going to use it from multiple threads.  It's a
>> > valid assumption and it avoids fine grained API level locking, and in
>> > many cases, wl would be integrated into an existing mainloop that
>> > solves locking.  Of course, EGL makes that kind of design impossible.
>> >
>> > So the two options we're looking at are
>> >
>> > 1) Toughen up and add locking around all access to wl_display.  It's
>> >   not as much code as it sounds, there are actually only a few entry
>> >   points that need locking.  Another problem that we need to solve is
>> >   how the wl_display_iterate() inside EGL may dispatch events, from a
>> >   rendering thread and (in case of the mesa implementation) with the
>> >   EGL lock held.  The solution there is to add thread affinity to
>> >   objects such that event callbacks from an object are always handled
>> >   in the thread that owns the object.
>> >
>> >   In case of single-threaded EGL applications we also need to make
>> >   sure we don't call out to random application callbacks while
>> >   blocking inside eglSwapBuffer (or other blocking EGL calls).  The
>> >   idea here is to introduce a wl_display_iterate_for_object() entry
>> >   point that will only call out for events addressed to that object
>> >   and queue up the rest.  This makes eventloop integration more
>> >   complicated, since now the question of "does this wl_display have
>> >   events?" isn't just a matter or whether or not the fd polls
>> >   readable, there may also be events queued up in the wl_display.
>> >   That's a part of Xlib API I've tried hard to avoid inflicting on
>> >   wayland users, but thanks to Xlib working this way, it's something
>> >   most event loops support.  We could also do some trickery with
>> >   epoll and eventfd to the "events pending" iff "fd readable"
>> >   semantics again.
>> >
>> >   Jørgen has been working on this approach, his patch is attached
>> >   (still work in progress).
>> >
>> > 2) Add a new wl_display for the other thread.  This gives EGL its own
>> >   wl_display, complete with a separate socket to the server.  This is
>> >   a more radical idea perhaps, but much less invasive.  We don't have
>> >   to change the assumption that wl_display is only usable in one
>> >   thread and we don't have to add locking.  We do need a mechanism to
>> >   share an object between the threads (typically the wl_surface), but
>> >   that's doable.  The bigger problem is how to synchronize the two
>> >   command streams.  Right now we can say eglSwapBuffer and then know
>> >   that that attaches a new wl_buffer to the surface and do something
>> >   that relies on that having happend, but if it happens in different
>> >   connections we need a mechanism to synchronize the two protocol
>> >   streams.  Of course, if you want to do something like that, you
>> >   still need to synchronize between the two threads to make sure the
>> >   eglSwapBuffer happened in one thread before you do something that
>> >   depends on that in anoteher thread.
>> >
>> >   The biggest drawback with this approach is that even single
>> >   threaded EGL applications now need to connections to the server.
>> >
>> > Anyway, that sums up the work we've done in this area.  I'd say that
>> > option 1) is the preferred solution at the moment.
>>
>> I have tried to make the library thread safe without changing the API.
>> The result is the attached patch, which differs from Jorgen's patch in
>> the following ways:
>>
>>  * no need for an added wl_display_get_notification_fd function.
>>  * TLS key is stored in the display structure, instead of using a global.
>>  * a thread is created on connect, it is dedicated to handle read and
>> writes to the display socket. The input events are queued in the
>> thread in which their proxy was created.
>>  * wl_display_flush is now unnecessary, since sending is done
>> asynchronously in the IO thread.
>>  * wl_display_iterate_for_objects is unnecessary (if you do not agree,
>> can you tell me what it would bring ?).
>>  * Added missing locks around the objects map accesses.
>>
>> This is a proof of concept, but the patch should work out of the box
>> with any current wayland client. I would just like to know if this
>> approach could be considered for merging in master. Some optimisations
>> could be done, for example we can allow any thread to dispatch output
>> events. We could also implement finer locking around the display
>> connection, but that would mean changing the connection
>> implementation. The wl_display_connect function also needs some added
>> code in case of failure.
>
> I want to add an IO thread in all cases unconditionally.  I don't want

Argh, *don't* want...

> to make single threaded programs multi threaded behind their backs.
> But more importantly, I don't want an IO thread automatically flushing
> output whenever it happens to wake up.  One of th key features of the
> protocol is that we can build up multiple requests in a buffer and
> only flush it once when then client goes to sleep (goes back into the
> main loop).  That way, there's only one sendmsg call at the end of an
> "activity period" for a client, and the server only does one recvmsg
> call to read all the requests.  One the server side it works the same
> way, and thus, when the client wakes up, it only does one recvmsg to
> receive all the events.  When everything works right, this gives us
> just one client wakeup and one server wakeup per frame.  Adding an IO
> thread breaks that predictability and adds the overhead of an extra
> thread, so I don't want to go that route.
>
> I do think we can address most of the points you made above without a
> thread, fortunately.  The idea I've discussed with Joergen is to use
> epoll on the client side too.  This lets us listen on the write
> notification fd and the socket fd using epoll, while still only
> exposing one fd outside the library (the epoll fd).  That way we don't
> have to add mandatory new API (like wl_display_get_notification_fd).
> We do need wl_display_iterate_for_object() for single threaded
> applications where we end up having to block in eglSwapBuffers().  In
> that case, all objects belong in the same thread, but we don't want to
> call out to any other objects except the one we're waiting on.
>
> Kristian
>
>> You can also check the code here:
>> https://github.com/fbx/wayland/commits/threaded-io
>>
>> Please tell me what you think of this approach.
>>
>> Thanks,
>>
>> --
>> rawoul
>
>