<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On 28 August 2014 12:02, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On Mon, 4 Aug 2014 11:30:46 +0200<br>
Marek Chalupa <<a href="mailto:mchqwerty@gmail.com" target="_blank">mchqwerty@gmail.com</a>> wrote:<br>
<br>
> From the doc comment I get the feeling, that after successfull call to<br>
> wl_display_prepare_read(), the thread gains exclusive access to the fd.<br>
> That is not true. It only ensures that _one_ of the threads, that called<br>
> this function, will read from the fd and there will be no race.<br>
><br>
> Here's slice of the commit message that introduces<br>
> wl_display_prepare_read() (3c7e8bfbb4745315b7bcbf69fa746c3d6718c305):<br>
><br>
> "wl_display_prepare_read() registers the calling thread as a potential<br>
> reader of events. Once data is available on the fd, all reader<br>
> threads must call wl_display_read_events(), at which point<br>
> one of the threads will read from the fd and distribute the events<br>
> to event queues. When that is done, all threads return from<br>
> wl_display_read_events()."<br>
><br>
> That is not clear from the doc message.<br>
> ---<br>
> src/wayland-client.c | 27 +++++++++++++++++++++++----<br>
> 1 file changed, 23 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/src/wayland-client.c b/src/wayland-client.c<br>
> index 19b5694..d7de24d 100644<br>
> --- a/src/wayland-client.c<br>
> +++ b/src/wayland-client.c<br>
> @@ -1155,6 +1155,12 @@ read_events(struct wl_display *display)<br>
> pthread_cond_broadcast(&display->reader_cond);<br>
> } else {<br>
> serial = display->read_serial;<br>
> + /* in the case that the thread is not allowed to read<br>
> + * from the fd, make it sleep until reading and dispatching<br>
> + * is done. The condition display->read_serial == serial) is<br>
> + * because it may happen, that some thread will call this<br>
> + * function after the reading is done and without this, the<br>
> + * thread would block until next pthread_cond_broadcast */<br>
> while (display->read_serial == serial)<br>
> pthread_cond_wait(&display->reader_cond,<br>
> &display->mutex);<br>
<br>
</div></div>I think the condition here is to ensure, that the function call will<br>
not return until someone has actually read the fd, not to avoid<br>
blocking in some cases. Apparently there could be spurious wake-ups,<br>
and need to protect against them. The 'serial' is set to<br>
display->read_serial just before waiting, and the mutex is held, so the<br>
cond_wait is guaranteed to happen at least once.<br>
<br>
This branch cannot be entered "after" the reading was done, because<br>
it is always the last call here that reads, when reader_count hits zero.<br>
<div><br></div></blockquote><div> </div><div>Yes, you're right. That's the real purpose of it. My mistake. However, AFAIK in the current code this loop<br></div><div>will run only once, because everywhere is the cond broadcast called along with<br>
</div><div>increasing the serial.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
> @@ -1180,6 +1186,13 @@ read_events(struct wl_display *display)<br>
> * Before calling this function, wl_display_prepare_read() must be<br>
> * called first.<br>
> *<br>
> + * When more threads call wl_display_prepare_read() to announce the<br>
> + * intention to read from the display's fd, all must call this function<br>
> + * (or wl_display_cancel_read()). Only one thread (at random) will succeed<br>
> + * in reading and the others will block in this function<br>
> + * until the reader is done to avoid races. If this function is called<br>
> + * after the reading is done, it returns immediately.<br>
<br>
</div>Continuing here, I do not think there is a "after the reading is done".<br>
<div><br>
> + *<br>
> * \memberof wl_display<br>
> */<br>
> WL_EXPORT int<br>
> @@ -1256,15 +1269,21 @@ wl_display_prepare_read_queue(struct wl_display *display,<br>
> * This function must be called before reading from the file<br>
> * descriptor using wl_display_read_events(). Calling<br>
> * wl_display_prepare_read() announces the calling threads intention<br>
> - * to read and ensures that until the thread is ready to read and<br>
> - * calls wl_display_read_events(), no other thread will read from the<br>
> - * file descriptor. This only succeeds if the event queue is empty<br>
> + * to read from the display's fd. This only succeeds if the event queue is empty<br>
> * though, and if there are undispatched events in the queue, -1 is<br>
> * returned and errno set to EAGAIN.<br>
> *<br>
> * If a thread successfully calls wl_display_prepare_read(), it must<br>
> * either call wl_display_read_events() when it's ready or cancel the<br>
> - * read intention by calling wl_display_cancel_read().<br>
> + * read intention by calling wl_display_cancel_read(). After successfull<br>
> + * polling on fd, all threads must call wl_display_read_events() (or cancel).<br>
> + * Only one of them will succeed in reading and once the data are read<br>
> + * and dispatched, all threads return from the function.<br>
<br>
</div>The wording is a little strange to me here. When you say only one thread<br>
succeeds, I would think all others fail and get an error. I think it is<br>
also not mandatory to poll, is it? You could just read() and cancel()<br>
right away, no?<br></blockquote><div><br></div><div>It's not mandatory. I took the sentence about polling from the original doc.<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
> + *<br>
> + * After calling wl_display_prepare_read(), the thread doesn't gain exclusive<br>
> + * access to the fd. It only ensures, that only one thread (at random) will read<br>
> + * from the fd. The non-readers will sleep until the reading is done to avoid races<br>
> + * (see below).<br>
> *<br>
> * Use this function before polling on the display fd or to integrate<br>
> * the fd into a toolkit event loop in a race-free way. Typically, a<br>
<br>
</div>Does it really matter that it might not be exactly this thread that<br>
will actually read the fd? All threads must be coded the same way in any<br>
case.<br>
<br>
Are you perhaps thinking of someone doing something else with the fd<br>
than calling wl_display_read_events()? If so, maybe we should just<br>
document the allowed/expected actions?<br>
<br></blockquote><div><br></div><div>Well, this piece of doc is not my work, here I took the feeling, that we must poll before<br></div><div>wl_display_read_events(). However, nowadays I (hopefully) understand how it works and<br>
</div><div>will reword it.<br><br></div><div>Regarding the allowed actions.. do you have something particular in mind? Theoretically,<br></div><div>programmer can safely use everything that doesn't change display->readers_count.<br>
</div><div>(basically everything except wl_display_dispatch_queue).<br>But I don't know why he would do that - the expected action is just to call wl_display_read_events<br></div><div>(with possibly poll before that) once you're prepared to read.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Or better yet, add a chapter to the publican docs about integrating<br>
libwayland-client to an event loop. I don't recall there being that<br>
yet...<br>
<br>
<br>
Thanks,<br>
pq<br></blockquote><div><br></div><div>Thanks for notes,<br></div><div>I'll fix the patch and eventually try to write up some 'big doc' for that.<br><br></div><div>Cheers,<br>Marek <br></div></div><br></div></div>