On 28 August 2014 17:52, Pekka Paalanen <ppaala...@gmail.com> wrote: > On Thu, 28 Aug 2014 15:59:37 +0200 > Marek Chalupa <mchqwe...@gmail.com> wrote: > > > On 28 August 2014 12:02, Pekka Paalanen <ppaala...@gmail.com> wrote: > > > > > On Mon, 4 Aug 2014 11:30:46 +0200 > > > Marek Chalupa <mchqwe...@gmail.com> wrote: > > > > > > > From the doc comment I get the feeling, that after successfull call > to > > > > wl_display_prepare_read(), the thread gains exclusive access to the > fd. > > > > That is not true. It only ensures that _one_ of the threads, that > called > > > > this function, will read from the fd and there will be no race. > > > > > > > > Here's slice of the commit message that introduces > > > > wl_display_prepare_read() (3c7e8bfbb4745315b7bcbf69fa746c3d6718c305): > > > > > > > > "wl_display_prepare_read() registers the calling thread as a > potential > > > > reader of events. Once data is available on the fd, all reader > > > > threads must call wl_display_read_events(), at which point > > > > one of the threads will read from the fd and distribute the events > > > > to event queues. When that is done, all threads return from > > > > wl_display_read_events()." > > > > > > > > That is not clear from the doc message. > > > > --- > > > > src/wayland-client.c | 27 +++++++++++++++++++++++---- > > > > 1 file changed, 23 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/wayland-client.c b/src/wayland-client.c > > > > index 19b5694..d7de24d 100644 > > > > --- a/src/wayland-client.c > > > > +++ b/src/wayland-client.c > > > > @@ -1155,6 +1155,12 @@ read_events(struct wl_display *display) > > > > pthread_cond_broadcast(&display->reader_cond); > > > > } else { > > > > serial = display->read_serial; > > > > + /* in the case that the thread is not allowed to read > > > > + * from the fd, make it sleep until reading and > dispatching > > > > + * is done. The condition display->read_serial == > serial) > > > is > > > > + * because it may happen, that some thread will call > this > > > > + * function after the reading is done and without > this, the > > > > + * thread would block until next > pthread_cond_broadcast */ > > > > while (display->read_serial == serial) > > > > pthread_cond_wait(&display->reader_cond, > > > > &display->mutex); > > > > > > I think the condition here is to ensure, that the function call will > > > not return until someone has actually read the fd, not to avoid > > > blocking in some cases. Apparently there could be spurious wake-ups, > > > and need to protect against them. The 'serial' is set to > > > display->read_serial just before waiting, and the mutex is held, so the > > > cond_wait is guaranteed to happen at least once. > > > > > > This branch cannot be entered "after" the reading was done, because > > > it is always the last call here that reads, when reader_count hits > zero. > > > > > > > > Yes, you're right. That's the real purpose of it. My mistake. However, > > AFAIK in the current code this loop > > will run only once, because everywhere is the cond broadcast called along > > with > > increasing the serial. > > The manual I could find: > http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_cond_wait.html > says that spurious wakeups may occur. So better keep the serial > there. > > Oh, didn't know that. Anyway, I wouldn't even think of removing the serial - it's working xD and it is an assurance for future extensions.
> > > Regarding the allowed actions.. do you have something particular in mind? > > Theoretically, > > programmer can safely use everything that doesn't change > > display->readers_count. > > (basically everything except wl_display_dispatch_queue). > > But I don't know why he would do that - the expected action is just to > call > > wl_display_read_events > > (with possibly poll before that) once you're prepared to read. > > Nothing particular in my mind, I was just wondering if there was > any old API that might break this. OTOH, that can be written off as > just not using the new API correctly. > > > > Or better yet, add a chapter to the publican docs about integrating > > > libwayland-client to an event loop. I don't recall there being that > > > yet... > > > > > > > > > Thanks, > > > pq > > > > > > > Thanks for notes, > > I'll fix the patch and eventually try to write up some 'big doc' for > that. > > Very good. I think this is one of the areas that are easy to get > subtly wrong, so a good doc is valuable. :-) > > When I first ran into that, the word 'subtly' was not in place xD > Thanks, > pq > Thanks, Marek
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel