On 11/05/2012 11:33 PM, Yonit Halperin wrote:
fix: rhbz#866929
At migration destination side, we need to restore the client's surfaces
state, before sending surfaces related messages.
Before this patch, we stopped the processing of only the cmd ring, till migration data
arrived.
However, some QXL_IOs require reading and rendering the cmd ring (e.g.,
update_area). Moreover, when the device is reset, after destroying all
surfaces, we assert (in qemu) if the cmd ring is not empty (see
rhbz#866929).
This fix makes the red_worker thread wait till the migration data arrives
(or till a timeout), and not process any input from the device after the
vm is started.
Hi Yonit,
This patch makes sense to me.
While red_worker is waiting, potentially, the qxl device (qemu-kvm)
continues to add
requests to the worker queue. Hopefully it will not fill it.
See 2 minor comments/questions below.
---
server/red_worker.c | 45 +++++++++++++++++++++++++++++++++++++--------
1 files changed, 37 insertions(+), 8 deletions(-)
diff --git a/server/red_worker.c b/server/red_worker.c
index 9edd5d4..dd27872 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -102,6 +102,7 @@
#define CHANNEL_PUSH_SLEEP_DURATION 10000 //micro
#define DISPLAY_CLIENT_TIMEOUT 15000000000ULL //nano
+#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT 10000000000ULL //nano
Nitpick, maybe, to make it clear, add after // nano -- 10 seconds
#define DISPLAY_CLIENT_RETRY_INTERVAL 10000 //micro
#define DISPLAY_FREE_LIST_DEFAULT_SIZE 128
@@ -4892,13 +4893,7 @@ static int red_process_commands(RedWorker *worker, uint32_t max_pipe_size, int *
int n = 0;
uint64_t start = red_now();
- /* we don't process the command ring if we are at the migration target and we
- * are expecting migration data. The migration data includes the list
- * of surfaces that are held by the client. We need to have it before
- * processing commands in order not to render and send surfaces unnecessarily
- */
- if (!worker->running || (worker->display_channel&&
- red_channel_waits_for_migrate_data(&worker->display_channel->common.base))) {
+ if (!worker->running) {
*ring_is_empty = TRUE;
return n;
}
@@ -11162,18 +11157,52 @@ void handle_dev_stop(void *opaque, void *payload)
red_wait_outgoing_items(&worker->cursor_channel->common.base);
}
+static int display_channel_wait_for_migrate_data(DisplayChannel *display)
+{
+ uint64_t end_time = red_now() + DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT;
+ RedChannel *channel =&display->common.base;
+ RedChannelClient *rcc;
+
+ spice_debug(NULL);
+ spice_assert(channel->clients_num == 1);
+
+ rcc = SPICE_CONTAINEROF(ring_get_head(&channel->clients), RedChannelClient, channel_link);
+ spice_assert(red_channel_client_waits_for_migrate_data(rcc));
+
+ for (;;) {
+ red_channel_client_receive(rcc);
+ if (!red_channel_client_is_connected(rcc)) {
+ break;
+ }
+
+ if (!red_channel_client_waits_for_migrate_data(rcc)) {
+ return TRUE;
+ }
+ if (red_now()> end_time) {
+ spice_warning("timeout");
+ red_channel_client_disconnect(rcc);
+ break;
+ }
+ usleep(DISPLAY_CLIENT_RETRY_INTERVAL);
+ }
+ return FALSE;
+}
+
void handle_dev_start(void *opaque, void *payload)
{
RedWorker *worker = opaque;
spice_assert(!worker->running);
+ worker->running = TRUE;
Why does worker->running have to be set before the waiting ?
if (worker->cursor_channel) {
worker->cursor_channel->common.during_target_migrate = FALSE;
}
if (worker->display_channel) {
worker->display_channel->common.during_target_migrate = FALSE;
+ if (red_channel_waits_for_migrate_data(&worker->display_channel->common.base)) {
+ display_channel_wait_for_migrate_data(worker->display_channel);
+ }
}
- worker->running = TRUE;
guest_set_client_capabilities(worker);
}
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel