Re: [PATCH spice-server 3/9] reds: Remove RedVDIReadBuf pooling code

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

 



Hi,

On Thu, Dec 07, 2017 at 07:32:10AM -0500, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > On Tue, Dec 05, 2017 at 08:41:06AM +0000, Frediano Ziglio wrote:
> > > Originally this pool was used to avoid allocation/deallocations.
> > > However the introduction of GList cause the code to do dynamic
> > > allocations in order to update the list making this pooling
> > > something useless.
> > > The buffers limitation is now implemented with a simple counter.
> > 
> > I'm not sure about this one. Seems that a pool here was overkill but I
> > only paid attention to the pool in the char-device, not this one...
> > 
> 
> This however is not a regression, I just maintained the current
> behaviour.

Its not a regression till we find the regression? :)

> Do you have any idea of the best way to test this?

Not really. The client-server communications in regard to agent messages
is token based, so at some point we run out of tokens (in a big file
transfer for instance) and the transfer is slowed down considerably [0]

[0]  https://bugs.freedesktop.org/show_bug.cgi?id=96818

> I would prefer to have this in and maybe a follow up, I won't like
> to have the entire patch reverted just because removing this limit
> trigger some condition (like file transfer, one I can think of)
> causing excessive resource consumption (memory, bandwidth or whatever).

Yeah, I thought it was overkill because I don't see a need for a memory
pool in this case too.

I agree with the patch, just did not acked before as maybe someone can
have a justification to keep it - if not, feel free to push it later on

Acked-by: Victor Toso <victortoso@xxxxxxxxxx>

> > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > ---
> > > Paranoia note: potentially the current allocations are bigger than
> > > the GList ones and this could affects the speed as usually allocators
> > > keep pool of different sizes
> > > ---
> > >  server/reds.c | 48 +++++++++++++++++-------------------------------
> > >  1 file changed, 17 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/server/reds.c b/server/reds.c
> > > index 40c82ccc..ab41244c 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -237,7 +237,7 @@ struct RedCharDeviceVDIPortPrivate {
> > >      AgentMsgFilter write_filter;
> > >  
> > >      /* read from agent */
> > > -    GList *read_bufs;
> > > +    uint32_t num_read_buf;
> > >      uint32_t read_state;
> > >      uint32_t message_receive_len;
> > >      uint8_t *receive_pos;
> > > @@ -722,32 +722,28 @@ static AgentMsgFilterResult
> > > vdi_port_read_buf_process(RedCharDeviceVDIPort *dev,
> > >      }
> > >  }
> > >  
> > > -static void vdi_read_buf_init(RedVDIReadBuf *buf)
> > > +static RedVDIReadBuf *vdi_read_buf_new(RedCharDeviceVDIPort *dev)
> > >  {
> > > -    g_return_if_fail(buf != NULL);
> > > +    RedVDIReadBuf *buf = g_new(RedVDIReadBuf, 1);
> > > +
> > >      /* Bogus pipe item type, we only need the RingItem and refcounting
> > >       * from the base class and are not going to use the type
> > >       */
> > >      red_pipe_item_init_full(&buf->base, -1,
> > >                              vdi_port_read_buf_free);
> > > +    buf->dev = dev;
> > > +    buf->len = 0;
> > > +    return buf;
> > >  }
> > >  
> > >  static RedVDIReadBuf *vdi_port_get_read_buf(RedCharDeviceVDIPort *dev)
> > >  {
> > > -    GList *item;
> > > -    RedVDIReadBuf *buf;
> > > -
> > > -    if (!(item = g_list_first(dev->priv->read_bufs))) {
> > > +    if (dev->priv->num_read_buf >= REDS_VDI_PORT_NUM_RECEIVE_BUFFS) {
> > >          return NULL;
> > >      }
> > >  
> > > -    buf = item->data;
> > > -    dev->priv->read_bufs = g_list_delete_link(dev->priv->read_bufs, item);
> > > -
> > > -    g_warn_if_fail(buf->base.refcount == 0);
> > > -    vdi_read_buf_init(buf);
> > > -
> > > -    return buf;
> > > +    dev->priv->num_read_buf++;
> > > +    return vdi_read_buf_new(dev);
> > >  }
> > >  
> > >  static void vdi_port_read_buf_free(RedPipeItem *base)
> > > @@ -755,15 +751,16 @@ static void vdi_port_read_buf_free(RedPipeItem *base)
> > >      RedVDIReadBuf *buf = SPICE_UPCAST(RedVDIReadBuf, base);
> > >  
> > >      g_warn_if_fail(buf->base.refcount == 0);
> > > -    buf->dev->priv->read_bufs = g_list_prepend(buf->dev->priv->read_bufs,
> > > buf);
> > > +    buf->dev->priv->num_read_buf--;
> > >  
> > > -    /* read_one_msg_from_vdi_port may have never completed because the
> > > read_bufs
> > > -       ring was empty. So we call it again so it can complete its work if
> > > +    /* read_one_msg_from_vdi_port may have never completed because we
> > > +       reached buffer limit. So we call it again so it can complete its
> > > work if
> > >         necessary. Note that since we can be called from
> > >         red_char_device_wakeup
> > >         this can cause recursion, but we have protection for that */
> > >      if (buf->dev->priv->agent_attached) {
> > >         red_char_device_wakeup(RED_CHAR_DEVICE(buf->dev));
> > >      }
> > > +    g_free(buf);
> > >  }
> > >  
> > >  static void agent_adjust_capabilities(VDAgentMessage *message,
> > > @@ -4511,24 +4508,11 @@ static void
> > > red_char_device_vdi_port_constructed(GObject *object)
> > >  static void
> > >  red_char_device_vdi_port_init(RedCharDeviceVDIPort *self)
> > >  {
> > > -    int i;
> > > -
> > >      self->priv = RED_CHAR_DEVICE_VDIPORT_PRIVATE(self);
> > >  
> > >      self->priv->read_state = VDI_PORT_READ_STATE_READ_HEADER;
> > >      self->priv->receive_pos = (uint8_t *)&self->priv->vdi_chunk_header;
> > >      self->priv->receive_len = sizeof(self->priv->vdi_chunk_header);
> > > -
> > > -    for (i = 0; i < REDS_VDI_PORT_NUM_RECEIVE_BUFFS; i++) {
> > > -        RedVDIReadBuf *buf = g_new0(RedVDIReadBuf, 1);
> > > -        vdi_read_buf_init(buf);
> > > -        buf->dev = self;
> > > -        g_warn_if_fail(!self->priv->agent_attached);
> > > -        /* This ensures the newly created buffer is placed in the
> > > -         * RedCharDeviceVDIPort::read_bufs queue ready to be reused
> > > -         */
> > > -        red_pipe_item_unref(&buf->base);
> > > -    }
> > >  }
> > >  
> > >  static void
> > > @@ -4542,8 +4526,10 @@ red_char_device_vdi_port_finalize(GObject *object)
> > >          red_pipe_item_unref(&dev->priv->current_read_buf->base);
> > >          dev->priv->current_read_buf = NULL;
> > >      }
> > > -    g_list_free_full(dev->priv->read_bufs, g_free);
> > >      g_free(dev->priv->mig_data);
> > > +    if (ENABLE_EXTRA_CHECKS) {
> > > +        spice_assert(dev->priv->num_read_buf == 0);
> > > +    }
> > >  
> > >      G_OBJECT_CLASS(red_char_device_vdi_port_parent_class)->finalize(object);
> > >  }
> 
> Frediano

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://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]