[Spice-commits] 3 commits - server/red_channel.c server/red_channel.h server/red_worker.c
Yonit Halperin
yhalperi at kemper.freedesktop.org
Fri Sep 27 06:05:26 PDT 2013
server/red_channel.c | 73 +++++++++++++++++++++++++--------------------------
server/red_channel.h | 22 +++++++++------
server/red_worker.c | 73 ++++++++++++++++++++++++++++++++++++++-------------
3 files changed, 105 insertions(+), 63 deletions(-)
New commits:
commit 90a4761249f84421b27d67a85262b1423b24fe04
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Wed Sep 11 15:02:23 2013 -0400
red_worker: disconnect the channel instead of shutdown in case of a blocking method failure
rhbz#1004443
The methods that trigger waitings on the client pipe require that
the waiting will succeed in order to continue, or otherwise, that
all the living pipe items will be released (e.g., when
we must destroy a surface, we need that all its related pipe items will
be released). Shutdown of the socket will eventually trigger
red_channel_client_disconnect (*), which will empty the pipe. However,
if the blocking method failed, we need to empty the pipe synchronously.
It is not safe(**) to call red_channel_client_disconnect from ChannelCbs
, but all the blocking calls in red_worker are done from callbacks that
are triggered from the device.
To summarize, calling red_channel_client_disconnect instead of calling
red_channel_client_shutdown will immediately release all the pipe items that are
held by the channel client (by calling red_channel_client_pipe_clear).
If red_clear_surface_drawables_from_pipe timeouts,
red_channel_client_disconnect will make sure that the surface we wish to
release is not referenced by any pipe-item.
(*) After a shutdown of a socket, we expect that later, when
red_peer_handle_incoming is called, it will encounter a socket
error and will call the channel's on_error callback which calls
red_channel_client_disconnect.
(**) I believe it was not safe before commit 2d2121a17038bc0 (before adding ref
count to ChannelClient). However, I think it might still be unsafe, because
red_channel_client_disconnect sets rcc->stream to NULL, and rcc->stream
may be referred later inside a red_channel_client method unsafely. So instead
of checking if (stream != NULL) after calling callbacks, we try to avoid
calling red_channel_client_disconnect from callbacks.
diff --git a/server/red_worker.c b/server/red_worker.c
index dea7325..1f67212 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10959,10 +10959,10 @@ void handle_dev_destroy_surface_wait(void *opaque, void *payload)
dev_destroy_surface_wait(worker, msg->surface_id);
}
-static void rcc_shutdown_if_pending_send(RedChannelClient *rcc)
+static void rcc_disconnect_if_pending_send(RedChannelClient *rcc)
{
if (red_channel_client_blocked(rcc) || rcc->pipe_size > 0) {
- red_channel_client_shutdown(rcc);
+ red_channel_client_disconnect(rcc);
} else {
spice_assert(red_channel_client_no_item_being_sent(rcc));
}
@@ -10988,7 +10988,7 @@ static inline void red_cursor_reset(RedWorker *worker)
if (!red_channel_wait_all_sent(&worker->cursor_channel->common.base,
DISPLAY_CLIENT_TIMEOUT)) {
red_channel_apply_clients(&worker->cursor_channel->common.base,
- rcc_shutdown_if_pending_send);
+ rcc_disconnect_if_pending_send);
}
}
}
@@ -11275,12 +11275,12 @@ void handle_dev_stop(void *opaque, void *payload)
if (!red_channel_wait_all_sent(&worker->display_channel->common.base,
DISPLAY_CLIENT_TIMEOUT)) {
red_channel_apply_clients(&worker->display_channel->common.base,
- rcc_shutdown_if_pending_send);
+ rcc_disconnect_if_pending_send);
}
if (!red_channel_wait_all_sent(&worker->cursor_channel->common.base,
DISPLAY_CLIENT_TIMEOUT)) {
red_channel_apply_clients(&worker->cursor_channel->common.base,
- rcc_shutdown_if_pending_send);
+ rcc_disconnect_if_pending_send);
}
}
commit bcf9e64f134a6073c1e404efc8892c1cb453bd8a
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Wed Sep 11 13:31:21 2013 -0400
red_channel: cleanup of red_channel_client blocking methods
(1) receive timeout as a parameter.
(2) add a return value and pass the handling
of failures to the calling routine.
diff --git a/server/red_channel.c b/server/red_channel.c
index 961c36c..2cef2be 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -51,11 +51,7 @@ typedef struct EmptyMsgPipeItem {
#define PING_TEST_TIMEOUT_MS 15000
#define PING_TEST_IDLE_NET_TIMEOUT_MS 100
-#define DETACH_TIMEOUT 15000000000ULL //nano
-#define DETACH_SLEEP_DURATION 10000 //micro
-
-#define CHANNEL_PUSH_TIMEOUT 30000000000ULL //nano
-#define CHANNEL_PUSH_SLEEP_DURATION 10000 //micro
+#define CHANNEL_BLOCKED_SLEEP_DURATION 10000 //micro
enum QosPingState {
PING_STATE_NONE,
@@ -2329,43 +2325,49 @@ uint32_t red_channel_sum_pipes_size(RedChannel *channel)
return sum;
}
-void red_wait_outgoing_item(RedChannelClient *rcc)
+int red_channel_client_wait_outgoing_item(RedChannelClient *rcc,
+ int64_t timeout)
{
uint64_t end_time;
int blocked;
if (!red_channel_client_blocked(rcc)) {
- return;
+ return TRUE;
+ }
+ if (timeout != -1) {
+ end_time = red_now() + timeout;
}
- end_time = red_now() + DETACH_TIMEOUT;
spice_info("blocked");
do {
- usleep(DETACH_SLEEP_DURATION);
+ usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
red_channel_client_receive(rcc);
red_channel_client_send(rcc);
- } while ((blocked = red_channel_client_blocked(rcc)) && red_now() < end_time);
+ } while ((blocked = red_channel_client_blocked(rcc)) &&
+ (timeout == -1 || red_now() < end_time));
if (blocked) {
spice_warning("timeout");
- // TODO - shutting down the socket but we still need to trigger
- // disconnection. Right now we wait for main channel to error for that.
- red_channel_client_shutdown(rcc);
+ return FALSE;
} else {
spice_assert(red_channel_client_no_item_being_sent(rcc));
+ return TRUE;
}
}
/* TODO: more evil sync stuff. anything with the word wait in it's name. */
-void red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
- PipeItem *item)
+int red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
+ PipeItem *item,
+ int64_t timeout)
{
uint64_t end_time;
int item_in_pipe;
spice_info(NULL);
- end_time = red_now() + CHANNEL_PUSH_TIMEOUT;
+ if (timeout != -1) {
+ end_time = red_now() + timeout;
+ }
rcc->channel->channel_cbs.hold_item(rcc, item);
@@ -2375,55 +2377,52 @@ void red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
}
red_channel_client_push(rcc);
- while((item_in_pipe = ring_item_is_linked(&item->link)) && (red_now() < end_time)) {
- usleep(CHANNEL_PUSH_SLEEP_DURATION);
+ while((item_in_pipe = ring_item_is_linked(&item->link)) &&
+ (timeout == -1 || red_now() < end_time)) {
+ usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
red_channel_client_receive(rcc);
red_channel_client_send(rcc);
red_channel_client_push(rcc);
}
+ red_channel_client_release_item(rcc, item, TRUE);
if (item_in_pipe) {
spice_warning("timeout");
- red_channel_client_disconnect(rcc);
- } else {
- red_wait_outgoing_item(rcc);
- }
- red_channel_client_release_item(rcc, item, TRUE);
-}
-
-static void rcc_shutdown_if_pending_send(RedChannelClient *rcc)
-{
- if (red_channel_client_blocked(rcc) || rcc->pipe_size > 0) {
- red_channel_client_shutdown(rcc);
+ return FALSE;
} else {
- spice_assert(red_channel_client_no_item_being_sent(rcc));
+ return red_channel_client_wait_outgoing_item(rcc,
+ timeout == -1 ? -1 : end_time - red_now());
}
}
-void red_channel_wait_all_sent(RedChannel *channel)
+int red_channel_wait_all_sent(RedChannel *channel,
+ int64_t timeout)
{
uint64_t end_time;
uint32_t max_pipe_size;
int blocked = FALSE;
- end_time = red_now() + DETACH_TIMEOUT;
+ if (timeout != -1) {
+ end_time = red_now() + timeout;
+ }
red_channel_push(channel);
while (((max_pipe_size = red_channel_max_pipe_size(channel)) ||
(blocked = red_channel_any_blocked(channel))) &&
- red_now() < end_time) {
+ (timeout == -1 || red_now() < end_time)) {
spice_debug("pipe-size %u blocked %d", max_pipe_size, blocked);
- usleep(DETACH_SLEEP_DURATION);
+ usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
red_channel_receive(channel);
red_channel_send(channel);
red_channel_push(channel);
}
if (max_pipe_size || blocked) {
- spice_printerr("timeout: pending out messages exist (pipe-size %u, blocked %d)",
- max_pipe_size, blocked);
- red_channel_apply_clients(channel, rcc_shutdown_if_pending_send);
+ spice_warning("timeout: pending out messages exist (pipe-size %u, blocked %d)",
+ max_pipe_size, blocked);
+ return FALSE;
} else {
spice_assert(red_channel_no_item_being_sent(channel));
+ return TRUE;
}
}
diff --git a/server/red_channel.h b/server/red_channel.h
index 676e1ef..9e54dce 100644
--- a/server/red_channel.h
+++ b/server/red_channel.h
@@ -612,14 +612,20 @@ int red_client_during_migrate_at_target(RedClient *client);
void red_client_migrate(RedClient *client);
-/* blocking function */
-void red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
- PipeItem *item);
-
-/* blocking function */
-void red_wait_outgoing_item(RedChannelClient *rcc);
+/*
+ * blocking functions.
+ *
+ * timeout is in nano sec. -1 for no timeout.
+ *
+ * Return: TRUE if waiting succeeded. FALSE if timeout expired.
+ */
-/* blocking function */
-void red_channel_wait_all_sent(RedChannel *channel);
+int red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
+ PipeItem *item,
+ int64_t timeout);
+int red_channel_client_wait_outgoing_item(RedChannelClient *rcc,
+ int64_t timeout);
+int red_channel_wait_all_sent(RedChannel *channel,
+ int64_t timeout);
#endif
diff --git a/server/red_worker.c b/server/red_worker.c
index 9cfacfa..dea7325 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -102,6 +102,7 @@
#define CMD_RING_POLL_TIMEOUT 10 //milli
#define CMD_RING_POLL_RETRIES 200
+#define DISPLAY_CLIENT_SHORT_TIMEOUT 15000000000ULL //nano
#define DISPLAY_CLIENT_TIMEOUT 30000000000ULL //nano
#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT 10000000000ULL //nano, 10 sec
#define DISPLAY_CLIENT_RETRY_INTERVAL 10000 //micro
@@ -2003,8 +2004,12 @@ static void red_current_clear(RedWorker *worker, int surface_id)
}
}
-static void red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface_id,
- int wait_if_used)
+/*
+ * Return: TRUE if wait_if_used == FALSE, or otherwise, if all of the pipe items that
+ * are related to the surface have been cleared (or sent) from the pipe.
+ */
+static int red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface_id,
+ int wait_if_used)
{
Ring *ring;
PipeItem *item;
@@ -2012,7 +2017,7 @@ static void red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
RedChannelClient *rcc;
if (!dcc) {
- return;
+ return TRUE;
}
/* removing the newest drawables that their destination is surface_id and
@@ -2057,24 +2062,27 @@ static void red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
if (wait_if_used) {
break;
} else {
- return;
+ return TRUE;
}
}
}
if (!wait_if_used) {
- return;
+ return TRUE;
}
if (item) {
- red_channel_client_wait_pipe_item_sent(&dcc->common.base, item);
+ return red_channel_client_wait_pipe_item_sent(&dcc->common.base, item,
+ DISPLAY_CLIENT_TIMEOUT);
} else {
/*
* in case that the pipe didn't contain any item that is dependent on the surface, but
- * there is one during sending.
+ * there is one during sending. Use a shorter timeout, since it is just one item
*/
- red_wait_outgoing_item(&dcc->common.base);
+ return red_channel_client_wait_outgoing_item(&dcc->common.base,
+ DISPLAY_CLIENT_SHORT_TIMEOUT);
}
+ return TRUE;
}
static void red_clear_surface_drawables_from_pipes(RedWorker *worker,
@@ -2085,7 +2093,9 @@ static void red_clear_surface_drawables_from_pipes(RedWorker *worker,
DisplayChannelClient *dcc;
WORKER_FOREACH_DCC_SAFE(worker, item, next, dcc) {
- red_clear_surface_drawables_from_pipe(dcc, surface_id, wait_if_used);
+ if (!red_clear_surface_drawables_from_pipe(dcc, surface_id, wait_if_used)) {
+ red_channel_client_disconnect(&dcc->common.base);
+ }
}
}
@@ -10949,6 +10959,15 @@ void handle_dev_destroy_surface_wait(void *opaque, void *payload)
dev_destroy_surface_wait(worker, msg->surface_id);
}
+static void rcc_shutdown_if_pending_send(RedChannelClient *rcc)
+{
+ if (red_channel_client_blocked(rcc) || rcc->pipe_size > 0) {
+ red_channel_client_shutdown(rcc);
+ } else {
+ spice_assert(red_channel_client_no_item_being_sent(rcc));
+ }
+}
+
static inline void red_cursor_reset(RedWorker *worker)
{
if (worker->cursor) {
@@ -10966,7 +10985,11 @@ static inline void red_cursor_reset(RedWorker *worker)
if (!worker->cursor_channel->common.during_target_migrate) {
red_pipes_add_verb(&worker->cursor_channel->common.base, SPICE_MSG_CURSOR_RESET);
}
- red_channel_wait_all_sent(&worker->cursor_channel->common.base);
+ if (!red_channel_wait_all_sent(&worker->cursor_channel->common.base,
+ DISPLAY_CLIENT_TIMEOUT)) {
+ red_channel_apply_clients(&worker->cursor_channel->common.base,
+ rcc_shutdown_if_pending_send);
+ }
}
}
@@ -11249,8 +11272,16 @@ void handle_dev_stop(void *opaque, void *payload)
* purge the pipe, send destroy_all_surfaces
* to the client (there is no such message right now), and start
* from scratch on the destination side */
- red_channel_wait_all_sent(&worker->display_channel->common.base);
- red_channel_wait_all_sent(&worker->cursor_channel->common.base);
+ if (!red_channel_wait_all_sent(&worker->display_channel->common.base,
+ DISPLAY_CLIENT_TIMEOUT)) {
+ red_channel_apply_clients(&worker->display_channel->common.base,
+ rcc_shutdown_if_pending_send);
+ }
+ if (!red_channel_wait_all_sent(&worker->cursor_channel->common.base,
+ DISPLAY_CLIENT_TIMEOUT)) {
+ red_channel_apply_clients(&worker->cursor_channel->common.base,
+ rcc_shutdown_if_pending_send);
+ }
}
static int display_channel_wait_for_migrate_data(DisplayChannel *display)
commit 6c2ff9864d32199424245b3ca41c201aa2f387b3
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Wed Sep 11 13:39:35 2013 -0400
red_worker: cleanup red_clear_surface_drawables_from_pipes
(1) merge 'force' and 'wait_for_outgoing_item' to one parameter.
'wait_for_outgoing_item' is a derivative of 'force'.
(2) move the call to red_wait_outgoing_item to red_clear_surface_drawables_from_pipe
diff --git a/server/red_worker.c b/server/red_worker.c
index 0c611d0..9cfacfa 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -2004,7 +2004,7 @@ static void red_current_clear(RedWorker *worker, int surface_id)
}
static void red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface_id,
- int force)
+ int wait_if_used)
{
Ring *ring;
PipeItem *item;
@@ -2054,7 +2054,7 @@ static void red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
if (depend_found) {
spice_debug("surface %d dependent item found %p, %p", surface_id, drawable, item);
- if (force) {
+ if (wait_if_used) {
break;
} else {
return;
@@ -2062,24 +2062,30 @@ static void red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
}
}
+ if (!wait_if_used) {
+ return;
+ }
+
if (item) {
red_channel_client_wait_pipe_item_sent(&dcc->common.base, item);
+ } else {
+ /*
+ * in case that the pipe didn't contain any item that is dependent on the surface, but
+ * there is one during sending.
+ */
+ red_wait_outgoing_item(&dcc->common.base);
}
}
-static void red_clear_surface_drawables_from_pipes(RedWorker *worker, int surface_id,
- int force, int wait_for_outgoing_item)
+static void red_clear_surface_drawables_from_pipes(RedWorker *worker,
+ int surface_id,
+ int wait_if_used)
{
RingItem *item, *next;
DisplayChannelClient *dcc;
WORKER_FOREACH_DCC_SAFE(worker, item, next, dcc) {
- red_clear_surface_drawables_from_pipe(dcc, surface_id, force);
- if (wait_for_outgoing_item) {
- // in case that the pipe didn't contain any item that is dependent on the surface, but
- // there is one during sending.
- red_wait_outgoing_item(&dcc->common.base);
- }
+ red_clear_surface_drawables_from_pipe(dcc, surface_id, wait_if_used);
}
}
@@ -4248,7 +4254,7 @@ static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface
otherwise "current" will hold items that other drawables may depend on, and then
red_current_clear will remove them from the pipe. */
red_current_clear(worker, surface_id);
- red_clear_surface_drawables_from_pipes(worker, surface_id, FALSE, FALSE);
+ red_clear_surface_drawables_from_pipes(worker, surface_id, FALSE);
red_destroy_surface(worker, surface_id);
break;
default:
@@ -10921,7 +10927,7 @@ static inline void destroy_surface_wait(RedWorker *worker, int surface_id)
otherwise "current" will hold items that other drawables may depend on, and then
red_current_clear will remove them from the pipe. */
red_current_clear(worker, surface_id);
- red_clear_surface_drawables_from_pipes(worker, surface_id, TRUE, TRUE);
+ red_clear_surface_drawables_from_pipes(worker, surface_id, TRUE);
}
static void dev_destroy_surface_wait(RedWorker *worker, uint32_t surface_id)
More information about the Spice-commits
mailing list