[Spice-devel] [PATCH spice-server 03/13] char-device: Allocate all write buffer in a single block

[Victor Toso]

Hi,

On Thu, May 30, 2019 at 03:22:44PM +0100, Frediano Ziglio wrote:
> There are no much data other than the buffer, reduce the
> allocations.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/char-device.c | 27 +++++++++++++++------------
>  server/char-device.h |  2 +-
>  2 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/server/char-device.c b/server/char-device.c
> index 89581ea42..059e8e5f6 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -149,11 +149,9 @@ red_char_device_remove_client(RedCharDevice *dev, RedClient *client)
>  
>  static void red_char_device_write_buffer_free(RedCharDeviceWriteBuffer *buf)
>  {
> -    if (buf == NULL)
> -        return;
> -
> -    g_free(buf->buf);
> -    g_free(buf);
> +    if (buf) {
> +        g_free(buf->priv);
> +    }
>  }
>  
>  static void write_buffers_queue_free(GQueue *write_queue)
> @@ -542,22 +540,27 @@ red_char_device_write_buffer_get(RedCharDevice *dev, RedClient *client, int size
>      ret = g_queue_pop_tail(&dev->priv->write_bufs_pool);
>      if (ret) {
>          dev->priv->cur_pool_size -= ret->buf_size;
> -    } else {
> +        if (ret->buf_size < size) {
> +            spice_assert(!spice_extra_checks || ret->priv->refs == 1);

Weird we didn't check this before...

> +            red_char_device_write_buffer_free(ret);
> +            ret = NULL;

I think that in the kernel they do this kind of _free() function
to return NULL just to make this kind of function call nicer,
that is,
    
    ret = red_char_device_write_buffer_free(ret);

while we often use glib's

    g_clear_pointer(&ret, red_char_device_write_buffer_free);

... just a silly comment :)

> +        }
> +    }
> +    if (ret == NULL) {
>          struct RedCharDeviceWriteBufferFull {
> -            RedCharDeviceWriteBuffer buffer;
>              RedCharDeviceWriteBufferPrivate priv;
> +            RedCharDeviceWriteBuffer buffer;
>          } *write_buf;
> -        write_buf = g_new0(struct RedCharDeviceWriteBufferFull, 1);
> +        write_buf = g_malloc(sizeof(struct RedCharDeviceWriteBufferFull) + size);
> +        memset(write_buf, 0, sizeof(*write_buf));

g_malloc0?

> +        write_buf->priv.refs = 1;

Weird that this was not being set before...

>          ret = &write_buf->buffer;
> +        ret->buf_size = size;
>          ret->priv = &write_buf->priv;
>      }
>  
>      spice_assert(!ret->buf_used);
>  
> -    if (ret->buf_size < size) {
> -        ret->buf = g_realloc(ret->buf, size);
> -        ret->buf_size = size;
> -    }

Looks fine,
Acked-by: Victor Toso <victortoso at redhat.com>

>      ret->priv->origin = origin;
>  
>      if (origin == WRITE_BUFFER_ORIGIN_CLIENT) {
> diff --git a/server/char-device.h b/server/char-device.h
> index 7d3ad8b3a..5327c25e5 100644
> --- a/server/char-device.h
> +++ b/server/char-device.h
> @@ -151,11 +151,11 @@ GType red_char_device_get_type(void) G_GNUC_CONST;
>  /* buffer that is used for writing to the device */
>  typedef struct RedCharDeviceWriteBufferPrivate RedCharDeviceWriteBufferPrivate;
>  typedef struct RedCharDeviceWriteBuffer {
> -    uint8_t *buf;
>      uint32_t buf_size;
>      uint32_t buf_used;
>  
>      RedCharDeviceWriteBufferPrivate *priv;
> +    uint8_t buf[0];
>  } RedCharDeviceWriteBuffer;
>  
>  void red_char_device_reset_dev_instance(RedCharDevice *dev,
> -- 
> 2.20.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190802/31e39aa2/attachment.sig>