[Spice-devel] [PATCH v2 6/9] server: move three functions to red_channel
Yonit Halperin
yhalperi at redhat.com
Mon Aug 12 12:44:49 PDT 2013
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,
>>>
>>
>>
More information about the Spice-devel
mailing list