[Spice-devel] [PATCH 14/20] Use GQueue for RedCharDevice::send_queue
David Jaša
djasa at redhat.com
Wed Apr 20 15:57:46 UTC 2016
On Po, 2016-04-18 at 05:17 -0400, Frediano Ziglio wrote:
> > On Thu, 2016-04-14 at 16:50 -0500, Jonathon Jongsma wrote:
> > > From: Christophe Fergeau <cfergeau at redhat.com>
> > >
> > > There was an extra RedCharDeviceMsgToClientItem type whose only
> > > purpose was to manage a linked list of items to send. GQueue has the
> > > same purpose as this type in addition to being generic. As the length of
> > > the send queue is tracked, a GQueue is more appropriate than a GList and
> > > allow to remove RedCharDevice::send_queue_size.
> > > ---
> > > server/char-device.c | 69
> > > ++++++++++++++++++---------------------------------
> > > -
> > > 1 file changed, 23 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/server/char-device.c b/server/char-device.c
> > > index f3e16da..6b9596e 100644
> > > --- a/server/char-device.c
> > > +++ b/server/char-device.c
> > > @@ -41,8 +41,7 @@ struct RedCharDeviceClient {
> > > uint64_t num_send_tokens; /* send to client */
> > > SpiceTimer *wait_for_tokens_timer;
> > > int wait_for_tokens_started;
> > > - Ring send_queue;
> > > - uint32_t send_queue_size;
> > > + GQueue *send_queue;
> > > uint32_t max_send_queue_size;
> > > };
> > >
> > > @@ -105,11 +104,6 @@ static guint signals[RED_CHAR_DEVICE_LAST_SIGNAL];
> > > static void red_char_device_write_buffer_unref(RedCharDeviceWriteBuffer
> > > *write_buf);
> > > static void red_char_device_write_retry(void *opaque);
> > >
> > > -typedef struct RedCharDeviceMsgToClientItem {
> > > - RingItem link;
> > > - PipeItem *msg;
> > > -} RedCharDeviceMsgToClientItem;
> > > -
> > > static PipeItem *
> > > red_char_device_read_one_msg_from_device(RedCharDevice *dev)
> > > {
> > > @@ -187,19 +181,10 @@ static void
> > > red_char_device_write_buffer_pool_add(RedCharDevice *dev,
> > > static void red_char_device_client_send_queue_free(RedCharDevice *dev,
> > > RedCharDeviceClient
> > > *dev_client)
> > > {
> > > - spice_debug("send_queue_empty %d", ring_is_empty(&dev_client
> > > ->send_queue));
> > > - while (!ring_is_empty(&dev_client->send_queue)) {
> > > - RingItem *item = ring_get_tail(&dev_client->send_queue);
> > > - RedCharDeviceMsgToClientItem *msg_item = SPICE_CONTAINEROF(item,
> > > -
> > > RedCharDeviceMsgToClientItem,
> > > - link);
> > > -
> > > - ring_remove(item);
> > > - pipe_item_unref(msg_item->msg);
> > > - free(msg_item);
> > > - }
> > > - dev_client->num_send_tokens += dev_client->send_queue_size;
> > > - dev_client->send_queue_size = 0;
> > > + spice_debug("send_queue_empty %d", g_queue_is_empty(dev_client
> > > ->send_queue));
> > > + g_queue_free_full(dev_client->send_queue, pipe_item_unref);
> > > + g_queue_clear(dev_client->send_queue);
> > > + dev_client->num_send_tokens += g_queue_get_length(dev_client
> > > ->send_queue);
> >
> > This looks wrong. We're calling g_queue_get_length() immediately after
> > clearing
> > the queue, so it's guaranteed to be 0. This last line should be moved before
> > the
> > g_queue_free_full() call.
> >
> > > }
> > >
> > > static void red_char_device_client_free(RedCharDevice *dev,
> > > @@ -303,17 +288,14 @@ static void
> > > red_char_device_add_msg_to_client_queue(RedCharDeviceClient *dev_cli
> > > PipeItem *msg)
> > > {
> > > RedCharDevice *dev = dev_client->dev;
> > > - RedCharDeviceMsgToClientItem *msg_item;
> > >
> > > - if (dev_client->send_queue_size >= dev_client->max_send_queue_size) {
> > > + if (g_queue_get_length(dev_client->send_queue) >= dev_client
> > > ->max_send_queue_size) {
> > > red_char_device_handle_client_overflow(dev_client);
> > > return;
> > > }
> > >
> > > - msg_item = spice_new0(RedCharDeviceMsgToClientItem, 1);
> > > - msg_item->msg = pipe_item_ref(msg);
> > > - ring_add(&dev_client->send_queue, &msg_item->link);
> > > - dev_client->send_queue_size++;
> > > + pipe_item_ref(msg);
> > > + g_queue_push_head(dev_client->send_queue, msg);
> > > if (!dev_client->wait_for_tokens_started) {
> > > reds_core_timer_start(dev->priv->reds, dev_client
> > > ->wait_for_tokens_timer,
> > > RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
> > > @@ -332,7 +314,7 @@ static void
> > > red_char_device_send_msg_to_clients(RedCharDevice *dev,
> > > dev_client = SPICE_CONTAINEROF(item, RedCharDeviceClient, link);
> > > if (red_char_device_can_send_to_client(dev_client)) {
> > > dev_client->num_send_tokens--;
> > > - spice_assert(ring_is_empty(&dev_client->send_queue));
> > > + spice_assert(g_queue_is_empty(dev_client->send_queue));
> > > red_char_device_send_msg_to_client(dev, msg,
> > > dev_client->client);
> > >
> > > /* don't refer to dev_client anymore, it may have been
> > > released
> > > */
> > > @@ -394,21 +376,17 @@ static int
> > > red_char_device_read_from_device(RedCharDevice *dev)
> > >
> > > static void red_char_device_client_send_queue_push(RedCharDeviceClient
> > > *dev_client)
> > > {
> > > - RingItem *item;
> > > - while ((item = ring_get_tail(&dev_client->send_queue)) &&
> > > - red_char_device_can_send_to_client(dev_client)) {
> > > - RedCharDeviceMsgToClientItem *msg_item;
> > > -
> > > - msg_item = SPICE_CONTAINEROF(item, RedCharDeviceMsgToClientItem,
> > > link);
> > > - ring_remove(item);
> > > -
> > > + while (!g_queue_is_empty(dev_client->send_queue)) {
> > > + GList *msg;
> >
> > GList* ?? Doesn't this GQueue contain PipeItem*?
> >
>
> This is a weird question from you. I forecast this situation 6/7 months ago
> and reported to the group many time. The fact that we are moving seriously
> spice-server to a very type unsafe code was not a secret and we (as a group)
> agreed to do it to speed up the merge but then only finished the merge
> do something about (not decided/spoken/propose on that anyway).
> Yes, should be a PipeItem *.
>
> > > + if (!red_char_device_can_send_to_client(dev_client)) {
> > > + break;
> > > + }
>
> Why this code change (previously this check was in the while
> condition).
>
> > > + msg = g_queue_pop_tail(dev_client->send_queue);
> > > + g_assert(msg != NULL);
> > > dev_client->num_send_tokens--;
> > > - red_char_device_send_msg_to_client(dev_client->dev,
> > > - msg_item->msg,
> > > + red_char_device_send_msg_to_client(dev_client->dev, msg->data,
> >
> > I think the second argument should simply be 'msg' instead of 'msg->data',
> > no? I
> > wonder why it works...
> >
>
> I think it get the first member of PipeItem which is a Ring and the first
> field is a pointer to another ring which now should be NULL.
> I don't understand too why it does not crash. Surely wrong.
>
> > > dev_client->client);
> > > - pipe_item_unref(msg_item->msg);
> > > - dev_client->send_queue_size--;
> > > - free(msg_item);
> > > + pipe_item_unref(msg);
> >
> >
> > ...and here, we free this GList* variable by calling pipe_item_unref().
> > That's
> > one of the pitfalls of making pipe_item_unref() accept a gpointer argument --
> > the compiler didn't warn us...
> >
>
> I personally stopped complaining too much about type safety some months ago.
>
g_slice can be told to detect free's of wrong size by setting G_SLICE to
debug-blocks. [1][2] I have no idea however if this will still be
available when g_slice allocator is dropped from glib...
David
[1] https://testbit.eu/28122006-g_slicedebug-blocks/
[2] https://developer.gnome.org/glib/2.48/glib-running.html#G_SLICE
> > > }
> > > }
> > >
> > > @@ -418,7 +396,7 @@ static void
> > > red_char_device_send_to_client_tokens_absorb(RedCharDeviceClient *de
> > > RedCharDevice *dev = dev_client->dev;
> > > dev_client->num_send_tokens += tokens;
> > >
> > > - if (dev_client->send_queue_size) {
> > > + if (g_queue_get_length(dev_client->send_queue)) {
> > > spice_assert(dev_client->num_send_tokens == tokens);
> > > red_char_device_client_send_queue_push(dev_client);
> > > }
> > > @@ -427,7 +405,7 @@ static void
> > > red_char_device_send_to_client_tokens_absorb(RedCharDeviceClient *de
> > > reds_core_timer_cancel(dev->priv->reds, dev_client
> > > ->wait_for_tokens_timer);
> > > dev_client->wait_for_tokens_started = FALSE;
> > > red_char_device_read_from_device(dev_client->dev);
> > > - } else if (dev_client->send_queue_size) {
> > > + } else if (!g_queue_is_empty(dev_client->send_queue)) {
> > > reds_core_timer_start(dev->priv->reds, dev_client
> > > ->wait_for_tokens_timer,
> > > RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
> > > dev_client->wait_for_tokens_started = TRUE;
> > > @@ -751,8 +729,7 @@ static RedCharDeviceClient
> > > *red_char_device_client_new(RedClient *client,
> > >
> > > dev_client = spice_new0(RedCharDeviceClient, 1);
> > > dev_client->client = client;
> > > - ring_init(&dev_client->send_queue);
> > > - dev_client->send_queue_size = 0;
> > > + dev_client->send_queue = g_queue_new();
> > > dev_client->max_send_queue_size = max_send_queue_size;
> > > dev_client->do_flow_control = do_flow_control;
> > > if (do_flow_control) {
> > > @@ -934,9 +911,9 @@ void
> > > red_char_device_migrate_data_marshall(RedCharDevice
> > > *dev,
> > > RedCharDeviceClient,
> > > link);
> > > /* FIXME: if there were more than one client before the marshalling,
> > > - * it is possible that the send_queue_size > 0, and the send data
> > > + * it is possible that the send_queue length > 0, and the send data
> > > * should be migrated as well */
> > > - spice_assert(dev_client->send_queue_size == 0);
> > > + spice_assert(g_queue_is_empty(dev_client->send_queue));
> > > spice_marshaller_add_uint32(m,
> > > SPICE_MIGRATE_DATA_CHAR_DEVICE_VERSION);
> > > spice_marshaller_add_uint8(m, 1); /* connected */
> > > spice_marshaller_add_uint32(m, dev_client->num_client_tokens);
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list