On Tue, 2015-12-15 at 10:42 -0500, Frediano Ziglio wrote: > > > > > > > > Hi > > > > > > ----- Original Message ----- > > > > Same comment as > > > > http://lists.freedesktop.org/archives/spice-devel/2015-December/024682.h > > > > tml > > > > > > > > "I don't really understand the reason for this patch. Why do we need to > > > > wakeup > > > > the worker immediately after a display pipe item has been released, but > > > > not > > > > a > > > > cursor item? Why can't we wait for the next worker poll timeout to > > > > expire? > > > > > > The question for me is rather, why would you wait for a timeout when you > > > know > > > you can process it now? > > > > > > > Process what actually? If you already deleted the item you already processed > > it. > > If the item is going out of worker you will get a reply, possibly a new > > item. > > If the item was going in it's already processed. > > Or am I missing something? > > > > Oh... now I start getting all these problems! > All came from Glib integration! > Basically before there was a loop were is was clear steps: > 1- select > 2- timers > 3- fd events > 4- guest events > 5- push data to channel. > Now point 5 is just a GSource so you don't know if is the last and > you force an iteration to make sure you don't wait too much. > > Well.. now I have another reason to prove that waiting to merge Glib loop code > was a good choice. > > About how to solve this issue (GSources ordering)... I don't know! > Maybe using priorities. I'll have a look at Glib code. So, let me explain what I think I understand so far. The commit log says "make sure we dispatch". I assume this is referring to the GSource dispatch function (worker_source_dispatch()) rather than something related to the spice-server Dispatcher object. The worker_source_dispatch() function basically just processes the cursor and display commands from the guest, adds them to a queue, and afterwards pushes the queue of pipe items out to the client. The patch under discussion attempts to trigger this dispatch function again after we 'release' a pipe item. Generally the pipe item will be freed from within the worker_source_dispatch() function itself (e.g. after sending the item in red_push()). So this patch appears to be a mechanism to re-dispatch the source as soon as possible after the source was dispatched (but only if we actually sent a item to the client and released it). But why do we need to do this again right away after a pipe item has been released? Is it because the send queue may have been full and so we want to try to send some more after the items in the queue have been sent and released? And we don't want to wait for a timeout to send any future items? It seems we're actually getting close to an 'idle' source by setting the timeout to 0 whenever we release a pipe item. However: the old loop only did display/cursor-handling once in the loop. After it was finished processing the display/cursor, it looped back and did a poll with timeout. So I still don't understand why we would need to try to avoid the poll timeout when we're using the glib loop. I understand that the old loop had a deterministic order and the new loop doesn't. But I still don't understand the problem we're trying to solve here. What am I missing? Jonathon > > Frediano > > > > > > > Updating the worker timeout seems like an implementation detail, so > > > > adding > > > > an > > > > API for other classes to call seems a little weird to me." > > > > > > > > Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> > > > > > > > > > > > > On Wed, 2015-12-09 at 12:17 +0000, Frediano Ziglio wrote: > > > > > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx> > > > > > > > > > > --- > > > > > server/display-channel.c | 2 ++ > > > > > server/red-worker.c | 8 ++++++++ > > > > > server/red-worker.h | 1 + > > > > > 3 files changed, 11 insertions(+) > > > > > > > > > > diff --git a/server/display-channel.c b/server/display-channel.c > > > > > index 32d87af..60170cb 100644 > > > > > --- a/server/display-channel.c > > > > > +++ b/server/display-channel.c > > > > > @@ -1987,9 +1987,11 @@ static void hold_item(RedChannelClient *rcc, > > > > > PipeItem > > > > > *item) > > > > > static void release_item(RedChannelClient *rcc, PipeItem *item, int > > > > > item_pushed) > > > > > { > > > > > DisplayChannelClient *dcc = RCC_TO_DCC(rcc); > > > > > + RedWorker *worker = DCC_TO_WORKER(dcc); > > > > > > > > > > spice_return_if_fail(item != NULL); > > > > > dcc_release_item(dcc, item, item_pushed); > > > > > + red_worker_update_timeout(worker, 0); > > > > > } > > > > > > > > > > static int handle_migrate_flush_mark(RedChannelClient *rcc) > > > > > diff --git a/server/red-worker.c b/server/red-worker.c > > > > > index b59a5a1..ae4ac27 100644 > > > > > --- a/server/red-worker.c > > > > > +++ b/server/red-worker.c > > > > > @@ -112,6 +112,14 @@ RedMemSlotInfo* red_worker_get_memslot(RedWorker > > > > > *worker) > > > > > return &worker->mem_slots; > > > > > } > > > > > > > > > > +void red_worker_update_timeout(RedWorker *worker, gint timeout) > > > > > +{ > > > > > + spice_return_if_fail(worker != NULL); > > > > > + spice_return_if_fail(timeout >= 0); > > > > > + > > > > > + worker->event_timeout = MIN(worker->event_timeout, timeout); > > > > > +} > > > > > + > > > > > static int display_is_connected(RedWorker *worker) > > > > > { > > > > > return (worker->display_channel && red_channel_is_connected( > > > > > diff --git a/server/red-worker.h b/server/red-worker.h > > > > > index b55a45c..3a9dc19 100644 > > > > > --- a/server/red-worker.h > > > > > +++ b/server/red-worker.h > > > > > @@ -94,6 +94,7 @@ static inline void red_pipes_add_verb(RedChannel > > > > > *channel, > > > > > uint16_t verb) > > > > > > > > > > RedWorker* red_worker_new(QXLInstance *qxl, RedDispatcher > > > > > *red_dispatcher); > > > > > bool red_worker_run(RedWorker *worker); > > > > > +void red_worker_update_timeout(RedWorker *worker, gint > > > > > timeout); > > > > > QXLInstance* red_worker_get_qxl(RedWorker *worker); > > > > > RedChannel* red_worker_get_cursor_channel(RedWorker *worker); > > > > > RedChannel* red_worker_get_display_channel(RedWorker *worker); > > > > _______________________________________________ > > Spice-devel mailing list > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/spice-devel > > > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel