[Spice-devel] [PATCH 8/9] worker: make sure we dispatch after releasing items
Frediano Ziglio
fziglio at redhat.com
Wed Dec 16 02:57:32 PST 2015
> 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 at redhat.com>
> > > > >
> > > > >
> > > > > On Wed, 2015-12-09 at 12:17 +0000, Frediano Ziglio wrote:
> > > > > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > > > >
> > > > > > ---
> > > > > > 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
More information about the Spice-devel
mailing list