[Spice-devel] [spice-server v2 5/6] Make RedCharDeviceWriteBuffer::refs private

[Christophe de Dinechin]

> 
> On 28 Apr 2017, at 11:11, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 
> Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> ---
> server/char-device.c | 11 ++++++-----
> server/char-device.h |  1 -
> 2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/server/char-device.c b/server/char-device.c
> index 7e77b465b..481ea2a13 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -36,6 +36,7 @@ struct RedCharDeviceWriteBufferPrivate {
>     RedClient *client; /* The client that sent the message to the device.
>                           NULL if the server created the message */
>     uint32_t token_price;
> +    uint32_t refs;
> };
> 
> typedef struct RedCharDeviceClient RedCharDeviceClient;
> @@ -165,7 +166,7 @@ static void write_buffers_queue_free(GQueue *write_queue)
> static void red_char_device_write_buffer_pool_add(RedCharDevice *dev,
>                                                   RedCharDeviceWriteBuffer *buf)
> {
> -    if (buf->refs == 1 &&
> +    if (buf->priv->refs == 1 &&
>         dev->priv->cur_pool_size < MAX_POOL_SIZE) {
>         buf->buf_used = 0;
>         buf->priv->origin = WRITE_BUFFER_ORIGIN_NONE;
> @@ -584,7 +585,7 @@ static RedCharDeviceWriteBuffer *__red_char_device_write_buffer_get(
>     }
> 
>     ret->priv->token_price = migrated_data_tokens ? migrated_data_tokens : 1;
> -    ret->refs = 1;
> +    ret->priv->refs = 1;
>     return ret;
> error:
>     dev->priv->cur_pool_size += ret->buf_size;
> @@ -612,7 +613,7 @@ static RedCharDeviceWriteBuffer *red_char_device_write_buffer_ref(RedCharDeviceW
> {
>     spice_assert(write_buf);
> 
> -    write_buf->refs++;
> +    write_buf->priv->refs++;

Increasing / decreasing refs is done in a non-atomic way. I assume this means that we know we cannot enter red_char_device_write_buffer_ref from different threads simultaneously for the same buffer. Just for my education, could someone explain why? My own mental model is that marshalling is a very sequential operation anyway, so it’s hard to think of a valid scenario where we’d parallelize it. But that makes me wonder if there is some kind of convention allowing us to easily identify that structures that are possibly shared between threads.
 

>     return write_buf;
> }
> 
> @@ -620,8 +621,8 @@ static void red_char_device_write_buffer_unref(RedCharDeviceWriteBuffer *write_b
> {
>     spice_assert(write_buf);
> 
> -    write_buf->refs--;
> -    if (write_buf->refs == 0)
> +    write_buf->priv->refs--;
> +    if (write_buf->priv->refs == 0)
>         red_char_device_write_buffer_free(write_buf);
> }
> 
> diff --git a/server/char-device.h b/server/char-device.h
> index 13e625fe1..f66e2a158 100644
> --- a/server/char-device.h
> +++ b/server/char-device.h
> @@ -151,7 +151,6 @@ typedef struct RedCharDeviceWriteBuffer {
>     uint8_t *buf;
>     uint32_t buf_size;
>     uint32_t buf_used;
> -    uint32_t refs;
> 
>     RedCharDeviceWriteBufferPrivate *priv;
> } RedCharDeviceWriteBuffer;
> -- 
> 2.12.2
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel