[Spice-devel] [PATCH 04/16] Use GQueue for RedCharDevice::send_queue

Jonathon Jongsma jjongsma at redhat.com
Wed Apr 20 14:48:01 UTC 2016


On Wed, 2016-04-20 at 04:38 -0400, Frediano Ziglio 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.
> > ---
> > Changes since last version:
> >  - red_char_device_client_send_queue_free(): update tokens before clearing
> >  queue
> >  - red_char_device_client_send_queue_push(): change GList* variable to
> >  PipeItem*
> > 
> >  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..2206c21 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));
> > +    dev_client->num_send_tokens +=
> > g_queue_get_length(dev_client->send_queue);
> > +    g_queue_free_full(dev_client->send_queue, pipe_item_unref);
> > +    g_queue_clear(dev_client->send_queue);
> >  }
> >  
> >  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)) {
> > +        PipeItem *msg;
> > +        if (!red_char_device_can_send_to_client(dev_client)) {
> > +            break;
> > +        }
> 
> I still would prefer this check to be done in the while as
> done before.

sure, I can move it back.

> 
> > +        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,
> >                                             dev_client->client);
> > -        pipe_item_unref(msg_item->msg);
> > -        dev_client->send_queue_size--;
> > -        free(msg_item);
> > +        pipe_item_unref(msg);
> >      }
> >  }
> >  
> > @@ -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);
> 
> I noted we call g_queue_new but not g_queue_free. I suspect we are inserting
> a leak here.



There is a call to g_queue_free_full() within
 red_char_device_client_send_queue_free(), which is called from
red_char_device_client_free(), so I don't think we're introducing a leak.

Jonathon


More information about the Spice-devel mailing list