[Spice-devel] [PATCH 06/10] RedChannelClient: store pipe items in a GList
Christophe Fergeau
cfergeau at redhat.com
Fri Sep 9 11:30:15 UTC 2016
On Thu, Sep 08, 2016 at 11:52:56AM -0500, Jonathon Jongsma wrote:
> Instead of using a Ring (and having a ring item link in every pipe
> item), store them in a GList. This also necesitated changing
> RedCharDeviceVDIPort->priv->read_bufs to a GList as well.
> ---
> server/cursor-channel.c | 2 -
> server/dcc-send.c | 22 +++++------
> server/dcc.c | 22 +++++------
> server/display-channel.c | 7 +++-
> server/red-channel-client-private.h | 3 +-
> server/red-channel-client.c | 75 ++++++++++++++++++++++---------------
> server/red-channel-client.h | 2 +-
> server/red-pipe-item.c | 1 -
> server/red-pipe-item.h | 6 ---
> server/reds.c | 14 +++----
> server/stream.c | 3 +-
> 11 files changed, 78 insertions(+), 79 deletions(-)
>
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 9b0f493..aa02ff3 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -190,8 +190,6 @@ static void cursor_pipe_item_free(RedPipeItem *base)
>
> RedCursorPipeItem *pipe_item = SPICE_UPCAST(RedCursorPipeItem, base);
>
> - spice_assert(!red_pipe_item_is_linked(&pipe_item->base));
> -
> cursor_item_unref(pipe_item->cursor_item);
> free(pipe_item);
> }
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 972ccc6..187eaca 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -189,7 +189,9 @@ static int is_brush_lossy(RedChannelClient *rcc, SpiceBrush *brush,
>
> static RedPipeItem *dcc_get_tail(DisplayChannelClient *dcc)
> {
> - return (RedPipeItem*)ring_get_tail(red_channel_client_get_pipe(RED_CHANNEL_CLIENT(dcc)));
> + GList *pipe = red_channel_client_get_pipe(RED_CHANNEL_CLIENT(dcc));
> + GList *last = g_list_last(pipe);
> + return last->data;
> }
>
> static void red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
> @@ -617,17 +619,13 @@ static int pipe_rendered_drawables_intersect_with_areas(DisplayChannelClient *dc
> SpiceRect *surface_areas[],
> int num_surfaces)
> {
> - RedPipeItem *pipe_item;
> - Ring *pipe;
> + GList *l;
>
> spice_assert(num_surfaces);
> - pipe = red_channel_client_get_pipe(RED_CHANNEL_CLIENT(dcc));
>
> - for (pipe_item = (RedPipeItem *)ring_get_head(pipe);
> - pipe_item;
> - pipe_item = (RedPipeItem *)ring_next(pipe, &pipe_item->link))
> - {
> + for (l = red_channel_client_get_pipe(RED_CHANNEL_CLIENT(dcc)); l != NULL; l = l->next) {
> Drawable *drawable;
> + RedPipeItem *pipe_item = l->data;
>
> if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)
> continue;
> @@ -706,8 +704,7 @@ static void red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
> int resent_surface_ids[MAX_PIPE_SIZE];
> SpiceRect resent_areas[MAX_PIPE_SIZE]; // not pointers since drawables may be released
> int num_resent;
> - RedPipeItem *pipe_item;
> - Ring *pipe;
> + GList *l, *pipe;
>
> resent_surface_ids[0] = first_surface_id;
> resent_areas[0] = *first_area;
> @@ -716,9 +713,8 @@ static void red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
> pipe = red_channel_client_get_pipe(RED_CHANNEL_CLIENT(dcc));
>
> // going from the oldest to the newest
> - for (pipe_item = (RedPipeItem *)ring_get_tail(pipe);
> - pipe_item;
> - pipe_item = (RedPipeItem *)ring_prev(pipe, &pipe_item->link)) {
> + for (l = g_list_last(pipe); l != NULL; l = l->prev) {
> + RedPipeItem *pipe_item = l->data;
> Drawable *drawable;
> RedDrawablePipeItem *dpi;
> RedImageItem *image;
> diff --git a/server/dcc.c b/server/dcc.c
> index 9fea119..b12d034 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -68,8 +68,8 @@ int dcc_drawable_is_in_pipe(DisplayChannelClient *dcc, Drawable *drawable)
> int dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface_id,
> int wait_if_used)
> {
> - Ring *ring;
> - RedPipeItem *item;
> + GList *l;
> + RedPipeItem *item = NULL;
> int x;
> RedChannelClient *rcc;
>
> @@ -78,13 +78,15 @@ int dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
> no other drawable depends on them */
>
> rcc = RED_CHANNEL_CLIENT(dcc);
> - ring = &rcc->priv->pipe;
> - item = (RedPipeItem *) ring;
> - while ((item = (RedPipeItem *)ring_next(ring, &item->link))) {
> + l = rcc->priv->pipe;
> + while (l != NULL) {
> + GList *cur = l;
> Drawable *drawable;
> RedDrawablePipeItem *dpi = NULL;
> int depend_found = FALSE;
>
> + l = l->next;
> + item = cur->data;
> if (item->type == RED_PIPE_ITEM_TYPE_DRAW) {
> dpi = SPICE_CONTAINEROF(item, RedDrawablePipeItem, dpi_pipe_item);
> drawable = dpi->drawable;
> @@ -95,12 +97,7 @@ int dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
> }
>
> if (drawable->surface_id == surface_id) {
> - RedPipeItem *tmp_item = item;
> - item = (RedPipeItem *)ring_prev(ring, &item->link);
> - red_channel_client_pipe_remove_and_release(rcc, tmp_item);
> - if (!item) {
> - item = (RedPipeItem *)ring;
> - }
> + red_channel_client_pipe_remove_and_release(rcc, item);
> continue;
> }
>
> @@ -289,7 +286,8 @@ static void red_drawable_pipe_item_free(RedPipeItem *item)
> dpi_pipe_item);
> spice_assert(item->refcount == 0);
>
> - spice_warn_if_fail(!ring_item_is_linked(&item->link));
> + spice_warn_if_fail(!red_channel_client_pipe_item_is_linked(RED_CHANNEL_CLIENT(dpi->dcc),
> + &dpi->dpi_pipe_item));
> if (ring_item_is_linked(&dpi->base)) {
> ring_remove(&dpi->base);
> }
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 581cf18..bd2907f 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -327,9 +327,12 @@ static void drawable_remove_from_pipes(Drawable *drawable)
> RingItem *item, *next;
>
> RING_FOREACH_SAFE(item, next, &drawable->pipes) {
> + RedChannelClient *rcc;
> +
> dpi = SPICE_UPCAST(RedDrawablePipeItem, item);
> - if (red_pipe_item_is_linked(&dpi->dpi_pipe_item)) {
> - red_channel_client_pipe_remove_and_release(RED_CHANNEL_CLIENT(dpi->dcc),
> + rcc = RED_CHANNEL_CLIENT(dpi->dcc);
> + if (red_channel_client_pipe_item_is_linked(rcc, &dpi->dpi_pipe_item)) {
> + red_channel_client_pipe_remove_and_release(rcc,
> &dpi->dpi_pipe_item);
> }
> }
> diff --git a/server/red-channel-client-private.h b/server/red-channel-client-private.h
> index 532bfa3..b13f905 100644
> --- a/server/red-channel-client-private.h
> +++ b/server/red-channel-client-private.h
> @@ -59,8 +59,7 @@ struct RedChannelClientPrivate
>
> int during_send;
> int id; // debugging purposes
> - Ring pipe;
> - uint32_t pipe_size;
> + GList *pipe;
>
> RedChannelCapabilities remote_caps;
> int is_mini_header;
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 0eed02b..9f678d7 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -344,7 +344,7 @@ void red_channel_client_on_out_msg_done(void *opaque)
> } else {
> if (rcc->priv->latency_monitor.timer
> && !rcc->priv->send_data.blocked
> - && rcc->priv->pipe_size == 0) {
> + && rcc->priv->pipe == NULL) {
> /* It is possible that the socket will become idle, so we may be able to test latency */
> red_channel_client_restart_ping_timer(rcc);
> }
> @@ -354,8 +354,7 @@ void red_channel_client_on_out_msg_done(void *opaque)
>
> static void red_channel_client_pipe_remove(RedChannelClient *rcc, RedPipeItem *item)
> {
> - rcc->priv->pipe_size--;
> - ring_remove(&item->link);
> + rcc->priv->pipe = g_list_remove(rcc->priv->pipe, item);
> }
>
> static void red_channel_client_set_remote_caps(RedChannelClient* rcc,
> @@ -681,8 +680,7 @@ RedChannelClient *red_channel_client_create(int size, RedChannel *channel, RedCl
> goto error;
> }
>
> - ring_init(&rcc->priv->pipe);
> - rcc->priv->pipe_size = 0;
> + rcc->priv->pipe = NULL;
>
> stream->watch = channel->core->watch_add(channel->core,
> stream->socket,
> @@ -743,7 +741,7 @@ RedChannelClient *red_channel_client_create_dummy(int size,
>
> rcc->incoming.header.data = rcc->incoming.header_buf;
> rcc->incoming.serial = 1;
> - ring_init(&rcc->priv->pipe);
> + rcc->priv->pipe = NULL;
>
> rcc->priv->dummy = TRUE;
> rcc->priv->dummy_connected = TRUE;
> @@ -1039,14 +1037,16 @@ void red_channel_client_send(RedChannelClient *rcc)
>
> static inline RedPipeItem *red_channel_client_pipe_item_get(RedChannelClient *rcc)
> {
> + GList *l;
> RedPipeItem *item;
>
> if (!rcc || rcc->priv->send_data.blocked
> || red_channel_client_waiting_for_ack(rcc)
> - || !(item = (RedPipeItem *)ring_get_tail(&rcc->priv->pipe))) {
> + || !(l = g_list_last(rcc->priv->pipe))) {
I think the condition only triggers if the list is empty? In which case
I'd just test for rcc->priv->pipe == NULL here, and move the
assignment out of the condition.
> return NULL;
> }
> - red_channel_client_pipe_remove(rcc, item);
> + item = l->data;
> + rcc->priv->pipe = g_list_delete_link(rcc->priv->pipe, l);
> return item;
> }
>
> @@ -1072,7 +1072,7 @@ void red_channel_client_push(RedChannelClient *rcc)
> while ((pipe_item = red_channel_client_pipe_item_get(rcc))) {
> red_channel_client_send_item(rcc, pipe_item);
> }
> - if (red_channel_client_no_item_being_sent(rcc) && ring_is_empty(&rcc->priv->pipe)
> + if (red_channel_client_no_item_being_sent(rcc) && rcc->priv->pipe == NULL
> && rcc->priv->stream->watch) {
> rcc->priv->channel->core->watch_update_mask(rcc->priv->stream->watch,
> SPICE_WATCH_EVENT_READ);
> @@ -1286,7 +1286,7 @@ void red_channel_client_set_message_serial(RedChannelClient *rcc, uint64_t seria
> rcc->priv->send_data.serial = serial;
> }
>
> -static inline gboolean client_pipe_add(RedChannelClient *rcc, RedPipeItem *item, RingItem *pos)
> +static inline gboolean prepare_pipe_add(RedChannelClient *rcc, RedPipeItem *item)
> {
> spice_assert(rcc && item);
> if (SPICE_UNLIKELY(!red_channel_client_is_connected(rcc))) {
> @@ -1294,20 +1294,21 @@ static inline gboolean client_pipe_add(RedChannelClient *rcc, RedPipeItem *item,
> red_pipe_item_unref(item);
> return FALSE;
> }
> - if (ring_is_empty(&rcc->priv->pipe) && rcc->priv->stream->watch) {
> + if (rcc->priv->pipe == NULL && rcc->priv->stream->watch) {
> rcc->priv->channel->core->watch_update_mask(rcc->priv->stream->watch,
> SPICE_WATCH_EVENT_READ |
> SPICE_WATCH_EVENT_WRITE);
> }
> - rcc->priv->pipe_size++;
> - ring_add(pos, &item->link);
> return TRUE;
> }
>
> void red_channel_client_pipe_add(RedChannelClient *rcc, RedPipeItem *item)
> {
>
> - client_pipe_add(rcc, item, &rcc->priv->pipe);
> + if (!prepare_pipe_add(rcc, item)) {
> + return;
> + }
> + rcc->priv->pipe = g_list_prepend(rcc->priv->pipe, item);
> }
>
> void red_channel_client_pipe_add_push(RedChannelClient *rcc, RedPipeItem *item)
> @@ -1320,27 +1321,40 @@ void red_channel_client_pipe_add_after(RedChannelClient *rcc,
> RedPipeItem *item,
> RedPipeItem *pos)
> {
> + GList *prev = NULL;
> +
> spice_assert(pos);
> - client_pipe_add(rcc, item, &pos->link);
> + if (!prepare_pipe_add(rcc, item)) {
> + return;
> + }
> + prev = g_list_find(rcc->priv->pipe, pos);
> + g_return_if_fail(prev != NULL);
> +
> + rcc->priv->pipe = g_list_insert_before(rcc->priv->pipe, prev->next, item);
> }
>
> int red_channel_client_pipe_item_is_linked(RedChannelClient *rcc,
> RedPipeItem *item)
> {
> - return ring_item_is_linked(&item->link);
> + return g_list_find(rcc->priv->pipe, item) != NULL;
> }
This makes this call much more expensive than before (though does not
really matter if we don't have many items in the pipe at any given
time). In particular, the use from drawable_remove_from_pipes() could
probably be improved, or dropped.
>
> void red_channel_client_pipe_add_tail(RedChannelClient *rcc,
> RedPipeItem *item)
> {
> - client_pipe_add(rcc, item, rcc->priv->pipe.prev);
> + if (!prepare_pipe_add(rcc, item)) {
> + return;
> + }
> + rcc->priv->pipe = g_list_append(rcc->priv->pipe, item);
> }
>
> void red_channel_client_pipe_add_tail_and_push(RedChannelClient *rcc, RedPipeItem *item)
> {
> - if (client_pipe_add(rcc, item, rcc->priv->pipe.prev)) {
> - red_channel_client_push(rcc);
> + if (!prepare_pipe_add(rcc, item)) {
> + return;
> }
> + rcc->priv->pipe = g_list_append(rcc->priv->pipe, item);
I think you could directly call red_channel_client_pipe_add_tail();
> + red_channel_client_push(rcc);
> }
>
> void red_channel_client_pipe_add_type(RedChannelClient *rcc, int pipe_item_type)
> @@ -1365,17 +1379,17 @@ void red_channel_client_pipe_add_empty_msg(RedChannelClient *rcc, int msg_type)
> gboolean red_channel_client_pipe_is_empty(RedChannelClient *rcc)
> {
> g_return_val_if_fail(rcc != NULL, TRUE);
> - return (rcc->priv->pipe_size == 0) && (ring_is_empty(&rcc->priv->pipe));
> + return (rcc->priv->pipe == NULL);
> }
>
> uint32_t red_channel_client_get_pipe_size(RedChannelClient *rcc)
> {
> - return rcc->priv->pipe_size;
> + return g_list_length(rcc->priv->pipe);
> }
>
> -Ring* red_channel_client_get_pipe(RedChannelClient *rcc)
> +GList* red_channel_client_get_pipe(RedChannelClient *rcc)
> {
> - return &rcc->priv->pipe;
> + return rcc->priv->pipe;
> }
>
> gboolean red_channel_client_is_mini_header(RedChannelClient *rcc)
> @@ -1405,16 +1419,15 @@ static void red_channel_client_clear_sent_item(RedChannelClient *rcc)
> // are we reading from an fd here? arghh
> static void red_channel_client_pipe_clear(RedChannelClient *rcc)
> {
> - RedPipeItem *item;
> + GList *l;
>
> if (rcc) {
> red_channel_client_clear_sent_item(rcc);
> }
> - while ((item = (RedPipeItem *)ring_get_head(&rcc->priv->pipe))) {
> - ring_remove(&item->link);
> - red_pipe_item_unref(item);
> + while ((l = rcc->priv->pipe)) {
> + red_pipe_item_unref(l->data);
> + rcc->priv->pipe = g_list_delete_link(rcc->priv->pipe, l);
> }
Could be g_list_free_full()
> - rcc->priv->pipe_size = 0;
> }
>
> void red_channel_client_ack_zero_messages_window(RedChannelClient *rcc)
> @@ -1545,8 +1558,8 @@ int red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
> }
> red_channel_client_push(rcc);
>
> - while(item_in_pipe &&
> - (timeout == -1 || spice_get_monotonic_time_ns() < end_time)) {
> + while (item_in_pipe &&
> + (timeout == -1 || spice_get_monotonic_time_ns() < end_time)) {
Unrelated
> usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
> red_channel_client_receive(rcc);
> red_channel_client_send(rcc);
> @@ -1598,7 +1611,7 @@ int red_channel_client_wait_outgoing_item(RedChannelClient *rcc,
>
> void red_channel_client_disconnect_if_pending_send(RedChannelClient *rcc)
> {
> - if (red_channel_client_is_blocked(rcc) || rcc->priv->pipe_size > 0) {
> + if (red_channel_client_is_blocked(rcc) || rcc->priv->pipe != NULL) {
> red_channel_client_disconnect(rcc);
> } else {
> spice_assert(red_channel_client_no_item_being_sent(rcc));
> diff --git a/server/red-channel-client.h b/server/red-channel-client.h
> index d2f7b3e..249e7fe 100644
> --- a/server/red-channel-client.h
> +++ b/server/red-channel-client.h
> @@ -114,7 +114,7 @@ void red_channel_client_pipe_add_type(RedChannelClient *rcc, int pipe_item_type)
> void red_channel_client_pipe_add_empty_msg(RedChannelClient *rcc, int msg_type);
> gboolean red_channel_client_pipe_is_empty(RedChannelClient *rcc);
> uint32_t red_channel_client_get_pipe_size(RedChannelClient *rcc);
> -Ring* red_channel_client_get_pipe(RedChannelClient *rcc);
> +GList* red_channel_client_get_pipe(RedChannelClient *rcc);
> gboolean red_channel_client_is_mini_header(RedChannelClient *rcc);
>
> void red_channel_client_ack_zero_messages_window(RedChannelClient *rcc);
> diff --git a/server/red-pipe-item.c b/server/red-pipe-item.c
> index 31262fa..d899610 100644
> --- a/server/red-pipe-item.c
> +++ b/server/red-pipe-item.c
> @@ -42,7 +42,6 @@ void red_pipe_item_init_full(RedPipeItem *item,
> gint type,
> red_pipe_item_free_t *free_func)
> {
> - ring_item_init(&item->link);
> item->type = type;
> item->refcount = 1;
> item->free_func = free_func ? free_func : (red_pipe_item_free_t *)free;
> diff --git a/server/red-pipe-item.h b/server/red-pipe-item.h
> index bf13b0b..a589c68 100644
> --- a/server/red-pipe-item.h
> +++ b/server/red-pipe-item.h
> @@ -26,7 +26,6 @@ struct RedPipeItem;
> typedef void red_pipe_item_free_t(struct RedPipeItem *item);
>
> typedef struct RedPipeItem {
> - RingItem link;
> int type;
>
> /* private */
> @@ -39,11 +38,6 @@ void red_pipe_item_init_full(RedPipeItem *item, int type, red_pipe_item_free_t f
> RedPipeItem *red_pipe_item_ref(RedPipeItem *item);
> void red_pipe_item_unref(RedPipeItem *item);
>
> -static inline int red_pipe_item_is_linked(RedPipeItem *item)
> -{
> - return ring_item_is_linked(&item->link);
> -}
> -
> static inline void red_pipe_item_init(RedPipeItem *item, int type)
> {
> red_pipe_item_init_full(item, type, NULL);
> diff --git a/server/reds.c b/server/reds.c
> index 153e201..9056c52 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -238,7 +238,7 @@ struct RedCharDeviceVDIPortPrivate {
> AgentMsgFilter write_filter;
>
> /* read from agent */
> - Ring read_bufs;
> + GList *read_bufs;
> uint32_t read_state;
> uint32_t message_receive_len;
> uint8_t *receive_pos;
> @@ -806,15 +806,15 @@ static void vdi_read_buf_init(RedVDIReadBuf *buf)
>
> static RedVDIReadBuf *vdi_port_get_read_buf(RedCharDeviceVDIPort *dev)
> {
> - RingItem *item;
> + GList *item;
> RedVDIReadBuf *buf;
>
> - if (!(item = ring_get_head(&dev->priv->read_bufs))) {
> + if (!(item = g_list_first(dev->priv->read_bufs))) {
> return NULL;
> }
I'd split this condition too
if (dev->pvir->read_bufs == NULL) {
return NULL;
}
buf = dev->priv->read_bufs->data;
Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160909/02fbf4a7/attachment-0001.sig>
More information about the Spice-devel
mailing list