On Wed, 2015-12-16 at 05:57 -0500, Frediano Ziglio wrote: > > 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/0246 > > > > > > 82.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); > > > > > > I wrote this patch (o top of Marc patch for release_item) to try understand > where release_item is called during the loop. > > > > diff --git a/server/dispatcher.c b/server/dispatcher.c > index 974fa75..b91ed90 100644 > --- a/server/dispatcher.c > +++ b/server/dispatcher.c > @@ -32,6 +32,7 @@ > #include "common/mem.h" > #include "common/spice_common.h" > #include "dispatcher.h" > +#include "red-worker.h" > > //#define DEBUG_DISPATCHER > > @@ -296,7 +297,9 @@ static gboolean dispatch_cb(GIOChannel *source, > GIOCondition condition, > Dispatcher *dispatcher = data; > > spice_debug(NULL); > + loop_phase = "dispatch"; > dispatcher_handle_recv_read(dispatcher); > + loop_phase = NULL; > > /* FIXME: remove source cb if error */ > return TRUE; > diff --git a/server/display-channel.c b/server/display-channel.c > index ae2aacf..45be7a7 100644 > --- a/server/display-channel.c > +++ b/server/display-channel.c > @@ -1982,6 +1982,9 @@ static void release_item(RedChannelClient *rcc, PipeItem > *item, int item_pushed) > spice_return_if_fail(item != NULL); > dcc_release_item(dcc, item, item_pushed); > red_worker_update_timeout(worker, 0); > + > + spice_assert(loop_phase); > + fprintf(stderr, "release on %s\n", loop_phase); > } > > static int handle_migrate_flush_mark(RedChannelClient *rcc) > diff --git a/server/red-worker.c b/server/red-worker.c > index 039339e..af5614a 100644 > --- a/server/red-worker.c > +++ b/server/red-worker.c > @@ -526,6 +526,8 @@ static int common_channel_config_socket(RedChannelClient > *rcc) > return TRUE; > } > > +const char *loop_phase = NULL; > + > typedef struct SpiceTimer { > SpiceTimerFunc func; > void *opaque; > @@ -546,7 +548,9 @@ static gboolean worker_timer_func(gpointer user_data) > { > SpiceTimer *timer = user_data; > > + loop_phase = "timer"; > timer->func(timer->opaque); > + loop_phase = NULL; > /* timer might be free after func(), don't touch */ > > return FALSE; > @@ -625,7 +629,9 @@ static gboolean watch_func(GIOChannel *source, > GIOCondition condition, > > spice_return_val_if_fail(!g_source_is_destroyed(watch->source), FALSE); > > + loop_phase = "watch"; > watch->func(fd, giocondition_to_spice_event(condition), watch->rcc); > + loop_phase = NULL; > > return TRUE; > } > @@ -1675,6 +1681,7 @@ static gboolean worker_source_dispatch(GSource *source, > GSourceFunc callback, > DisplayChannel *display = worker->display_channel; > int ring_is_empty; > > + loop_phase = "free glz"; > /* during migration, in the dest, the display channel can be initialized > while the global lz data not since migrate data msg hasn't been > received yet */ > @@ -1682,15 +1689,21 @@ static gboolean worker_source_dispatch(GSource > *source, GSourceFunc callback, > display_channel_free_glz_drawables_to_free(display); > > /* FIXME: could use its own source */ > + loop_phase = "stream timeouts"; > stream_timeout(display); > > worker->event_timeout = INF_EVENT_WAIT; > + loop_phase = "cursor"; > red_process_cursor(worker, MAX_PIPE_SIZE, &ring_is_empty); > + loop_phase = "display"; > red_process_display(worker, MAX_PIPE_SIZE, &ring_is_empty); > > /* FIXME: remove me? that should be handled by watch out condition */ > + loop_phase = "push"; > red_push(worker); > > + loop_phase = NULL; > + > return TRUE; > } > > diff --git a/server/red-worker.h b/server/red-worker.h > index 951f356..f2b7437 100644 > --- a/server/red-worker.h > +++ b/server/red-worker.h > @@ -102,6 +102,8 @@ GMainContext* red_worker_get_main_context(RedWorker > *worker); > clockid_t red_worker_get_clockid(RedWorker *worker); > RedMemSlotInfo* red_worker_get_memslot(RedWorker *worker); > > +extern const char *loop_phase; > + > void red_drawable_unref(RedWorker *worker, RedDrawable *red_drawable, > uint32_t group_id); > > > > Then with a > > $ cat stderr_from_replay | grep 'release on ' | sort | uniq -c > 3 release on dispatch > 502 release on display > 3249 release on push > 1843 release on watch > > So mostly of the times items are released pushing (which is expected) but > also on watches (so data from file descriptors) and processing display data. > I think this patch address the items during watches which before happened > always before the push. > > I think a better solution is to move red_push inside worker_source_prepare. > > Frediano It seems that "watch" is related to watches on non-Dispatcher fds, and "dispatch" is related to the watch on the Dispatcher fd. Items can be released in the "display" stage when we try to queue pipe items when the client is not connected (see red_channel_client_pipe_add(), which calls validate_pipe_add() which releases the pipe item if the client is not connected). They can also be released immediately after connection as part of dcc_start() -> dcc_push_surface_image(). In either of these cases, I don't think it's useful to trigger another source dispatch. Those released in the "push" stage have just been sent to the client, so the only reason I can think to trigger another source dispatch here is perhaps to re -run the display/cursor handler immediately in the case that the client pipe item queue was full. But I have no idea whether this is a regular occurrence or not. I suspect this was not really the goal of this patch. The "dispatch" stage happens when we receive a message from the Dispatcher object. These messages from the Dispatcher can cause us to send new pipe items to the client, and they can also queue new items without sending them. For example, handle_dev_destroy_primary_surface() will flush all queued display and cursor commands (which results in a red_channel_push() to send those to the client, which will in turn release those pipe items), and then it does a bunch of stuff with dependent drawables (which may result in pipe items being released without being sent) and then it queues a DESTROY_SURFACE pipe item. It doesn't seem to me that the act of releasing any of those pipe items that I described above should result in an immediate source dispatch. The "watch" stage is perhaps more interesting since it's the second highest total in your results. The watch handler is generally triggered when we receive data on the client socket. Different channels have different handlers, and different behaviors related to Pipe Items. Some handlers don't result in any messages sent back to the client (and thus, no pipe items are created or released). Some handlers result in a new pipe item being queued to be sent to the client (e.g. INPUTS_MOUSE_POSITION queues a MOUSE_MOTION_ACK item to send to the client), but do not send it directly. And some handlers send a message to the client immediately (SPICE_MSGC_ACK calls red_channel_client_push() and thus results in all of the sent items being released). Of these three different situations, only the third one (immediately sending a message to the client) would cause us to call release_item() and thus set the timeout to 0 and trigger another source dispatch. But I still don't understand why that would be useful. So I'm left with a big question: What problem are we trying to solve here? I can't think of a problem that would be solved by this patch. Jonathon _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel