Re: [PATCH v2 6/9] server: move three functions to red_channel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

On 08/12/2013 12:32 PM, Alon Levy wrote:

Hi,

I think red_channel_client_wait_pipe_item_sent should ref and unref the
pipe item by itself (using the hold/release-item callbacks). Then, you
don't need dcc_wait_pipe_item_sent.


This is a bit scarier then my mechanical change. Since there are two versions of the release function, for item_pushed=1 and item_pushed=0, which is the correct one to use? if we timeout and disconnect the channel, does that mean the item hasn't been pushed? for the current case it doesn't matter, but if something comes along and reuses this later it may be a problem.
I understand what you mean, but it is an error to call ring_item_is_linked(&item->link) after the item was released, and It is dangerous that the calling routine should increment the ref-count by itself. We use item_pushed=0, in order to release information that is needed only as long the item is inside the pipe. release_item with item_pushed=0 should be called only once, and in case of a timeout, red_channel_client_disconnect will call it in this manner. So item_pushed=1 should be called in any case. But I agree it is not clear. 





Also, I'm not sure why PUSH_TIMEOUT is bigger than DETACH timeout. It
would have been logical if the bigger timeout was used for
red_wait_all_sent, but it isn't. But we can clean it up in another patch
series :).


ok.



Can you also s/red_wait_all_sent/red_channel_wait_all_sent?



sure.


Cheers,
Yonit.

On 08/12/2013 11:32 AM, Alon Levy wrote:

Three blocking functions, one was split to leave the display channel
specific referencing of the DrawablePipeItem being sent inside
red_worker, but the rest (most) of the timeout logic was moved to
red_channel, including the associated constants.

Moved functions:
red_channel_client_wait_pipe_item_sent
red_wait_outgoing_item
red_wait_all_sent
---
   server/red_channel.c | 104 ++++++++++++++++++++++++++++++++++++++++
   server/red_channel.h |  10 ++++
   server/red_worker.c  | 131
   +++++----------------------------------------------
   3 files changed, 126 insertions(+), 119 deletions(-)

diff --git a/server/red_channel.c b/server/red_channel.c
index d565634..29d64a9 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -41,6 +41,7 @@
   #include "red_channel.h"
   #include "reds.h"
   #include "main_dispatcher.h"
+#include "red_time.h"

   typedef struct EmptyMsgPipeItem {
       PipeItem base;
@@ -50,6 +51,12 @@ 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
+
   enum QosPingState {
       PING_STATE_NONE,
       PING_STATE_TIMER,
@@ -2195,3 +2202,100 @@ uint32_t red_channel_sum_pipes_size(RedChannel
*channel)
       }
       return sum;
   }
+
+void red_wait_outgoing_item(RedChannelClient *rcc)
+{
+    uint64_t end_time;
+    int blocked;
+
+    if (!red_channel_client_blocked(rcc)) {
+        return;
+    }
+    end_time = red_now() + DETACH_TIMEOUT;
+    spice_info("blocked");
+
+    do {
+        usleep(DETACH_SLEEP_DURATION);
+        red_channel_client_receive(rcc);
+        red_channel_client_send(rcc);
+    } while ((blocked = red_channel_client_blocked(rcc)) && 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);
+    } else {
+        spice_assert(red_channel_client_no_item_being_sent(rcc));
+    }
+}
+
+
+/* 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)
+{
+    uint64_t end_time;
+    int item_in_pipe;
+
+    spice_info(NULL);
+
+    end_time = red_now() + CHANNEL_PUSH_TIMEOUT;
+
+    if (red_channel_client_blocked(rcc)) {
+        red_channel_client_receive(rcc);
+        red_channel_client_send(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);
+        red_channel_client_receive(rcc);
+        red_channel_client_send(rcc);
+        red_channel_client_push(rcc);
+    }
+
+    if (item_in_pipe) {
+        spice_warning("timeout");
+        red_channel_client_disconnect(rcc);
+    } else {
+        red_wait_outgoing_item(rcc);
+    }
+}
+
+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));
+    }
+}
+
+void red_wait_all_sent(RedChannel *channel)
+{
+    uint64_t end_time;
+    uint32_t max_pipe_size;
+    int blocked = FALSE;
+
+    end_time = red_now() + DETACH_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) {
+        spice_debug("pipe-size %u blocked %d", max_pipe_size, blocked);
+        usleep(DETACH_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);
+    } else {
+        spice_assert(red_channel_no_item_being_sent(channel));
+    }
+}
diff --git a/server/red_channel.h b/server/red_channel.h
index 0dd73ea..b2a3a6a 100644
--- a/server/red_channel.h
+++ b/server/red_channel.h
@@ -596,4 +596,14 @@ 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 function */
+void red_wait_all_sent(RedChannel *channel);
+
   #endif
diff --git a/server/red_worker.c b/server/red_worker.c
index 3b9c5b0..334a709 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -83,6 +83,7 @@
   #include "spice_timer_queue.h"
   #include "main_dispatcher.h"
   #include "spice_server_utils.h"
+#include "red_time.h"

   //#define COMPRESS_STAT
   //#define DUMP_BITMAP
@@ -103,12 +104,6 @@
   #define CMD_RING_POLL_TIMEOUT 10 //milli
   #define CMD_RING_POLL_RETRIES 200

-#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 DISPLAY_CLIENT_TIMEOUT 30000000000ULL //nano
   #define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT 10000000000ULL //nano, 10 sec
   #define DISPLAY_CLIENT_RETRY_INTERVAL 10000 //micro
@@ -1104,10 +1099,8 @@ static void
cursor_channel_client_release_item_before_push(CursorChannelClient *
                                                              PipeItem
                                                              *item);
   static void
   cursor_channel_client_release_item_after_push(CursorChannelClient *ccc,
                                                             PipeItem
                                                             *item);
-static void red_wait_pipe_item_sent(RedChannelClient *rcc, PipeItem
*item);

   static void red_push_monitors_config(DisplayChannelClient *dcc);
-static inline uint64_t red_now(void);

   /*
    * Macros to make iterating over stuff easier
@@ -2035,6 +2028,16 @@ static void red_current_clear(RedWorker *worker, int
surface_id)
       }
   }

+static void dcc_wait_pipe_item_sent(RedChannelClient *rcc, PipeItem *item)
+{
+    DrawablePipeItem *dpi;
+
+    dpi = SPICE_CONTAINEROF(item, DrawablePipeItem, dpi_pipe_item);
+    ref_drawable_pipe_item(dpi);
+    red_channel_client_wait_pipe_item_sent(rcc, item);
+    put_drawable_pipe_item(dpi);
+}
+
   static void red_clear_surface_drawables_from_pipe(DisplayChannelClient
   *dcc, int surface_id,
                                                     int force)
   {
@@ -2095,12 +2098,10 @@ static void
red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
       }

       if (item) {
-        red_wait_pipe_item_sent(&dcc->common.base, item);
+        dcc_wait_pipe_item_sent(&dcc->common.base, item);
       }
   }

-static void red_wait_outgoing_item(RedChannelClient *rcc);
-
   static void red_clear_surface_drawables_from_pipes(RedWorker *worker, int
   surface_id,
       int force, int wait_for_outgoing_item)
   {
@@ -5086,15 +5087,6 @@ static void qxl_process_cursor(RedWorker *worker,
RedCursorCmd *cursor_cmd, uint
       red_release_cursor(worker, cursor_item);
   }

-static inline uint64_t red_now(void)
-{
-    struct timespec time;
-
-    clock_gettime(CLOCK_MONOTONIC, &time);
-
-    return ((uint64_t) time.tv_sec) * 1000000000 + time.tv_nsec;
-}
-
   static int red_process_cursor(RedWorker *worker, uint32_t max_pipe_size,
   int *ring_is_empty)
   {
       QXLCommandExt ext_cmd;
@@ -10986,105 +10978,6 @@ typedef struct __attribute__ ((__packed__))
CursorData {
       SpiceCursor _cursor;
   } CursorData;

-static void red_wait_outgoing_item(RedChannelClient *rcc)
-{
-    uint64_t end_time;
-    int blocked;
-
-    if (!red_channel_client_blocked(rcc)) {
-        return;
-    }
-    end_time = red_now() + DETACH_TIMEOUT;
-    spice_info("blocked");
-
-    do {
-        usleep(DETACH_SLEEP_DURATION);
-        red_channel_client_receive(rcc);
-        red_channel_client_send(rcc);
-    } while ((blocked = red_channel_client_blocked(rcc)) && 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);
-    } else {
-        spice_assert(red_channel_client_no_item_being_sent(rcc));
-    }
-}
-
-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 void red_wait_all_sent(RedChannel *channel)
-{
-    uint64_t end_time;
-    uint32_t max_pipe_size;
-    int blocked = FALSE;
-
-    end_time = red_now() + DETACH_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) {
-        spice_debug("pipe-size %u blocked %d", max_pipe_size, blocked);
-        usleep(DETACH_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);
-    } else {
-        spice_assert(red_channel_no_item_being_sent(channel));
-    }
-}
-
-/* TODO: more evil sync stuff. anything with the word wait in it's name.
*/
-static void red_wait_pipe_item_sent(RedChannelClient *rcc, PipeItem *item)
-{
-    DrawablePipeItem *dpi;
-    uint64_t end_time;
-    int item_in_pipe;
-
-    spice_info(NULL);
-    dpi = SPICE_CONTAINEROF(item, DrawablePipeItem, dpi_pipe_item);
-    ref_drawable_pipe_item(dpi);
-
-    end_time = red_now() + CHANNEL_PUSH_TIMEOUT;
-
-    if (red_channel_client_blocked(rcc)) {
-        red_channel_client_receive(rcc);
-        red_channel_client_send(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);
-        red_channel_client_receive(rcc);
-        red_channel_client_send(rcc);
-        red_channel_client_push(rcc);
-    }
-
-    if (item_in_pipe) {
-        spice_warning("timeout");
-        red_channel_client_disconnect(rcc);
-    } else {
-        red_wait_outgoing_item(rcc);
-    }
-    put_drawable_pipe_item(dpi);
-}
-
   static void surface_dirty_region_to_rects(RedSurface *surface,
                                             QXLRect *qxl_dirty_rects,
                                             uint32_t num_dirty_rects,






_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]