[Spice-devel] [PATCH spice-server 03/14] char_device: Introducing shared flow control code for char devices.
Yonit Halperin
yhalperi at redhat.com
Mon Jul 2 08:36:12 PDT 2012
On 07/02/2012 02:11 PM, Alon Levy wrote:
> On Wed, Jun 27, 2012 at 06:16:41PM +0300, Yonit Halperin wrote:
>
> I didn't review the whole series, but since I see Hans already provided
> some comments on this patch I'll add my own.
>
> I also think the patchset looks very good.
>
> For the future, it would be nice to have byte level accounting as well
> as token (message) level. For the future! not for now :)
>
> Comments inline.
>
>> SpiceCharDeviceState manages the (1) write-to-device queue
>> (2) wakeup and reading from the device (3) client tokens (4)
>> sending messages from the device to the client/s, considering the
>> available tokens.
>> SpiceCharDeviceState can be also stopped and started. When the device
>> is stopped, no reading or writing is done from/to the device. Messages
>> addressed from the client to the device are being queued.
>> Later, an api for stop/start will be added to spice.h and it should
>> be called from qemu.
>>
>> This patch does not yet remove the wakeup callback from
>> SpiceCharDeviceState, but once all the char devices (agent/spicevmc/smartcard)
>> code will switch to the new implementation, SpiceCharDeviceState
>> will be moved to the c file and its reference to the wakeup callback will be removed.
>> ---
>> server/Makefile.am | 1 +
>> server/char_device.c | 741 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> server/char_device.h | 198 ++++++++++++++
>> 3 files changed, 940 insertions(+), 0 deletions(-)
>> create mode 100644 server/char_device.c
>>
>> diff --git a/server/Makefile.am b/server/Makefile.am
>> index 47b3c10..e7b4977 100644
>> --- a/server/Makefile.am
>> +++ b/server/Makefile.am
>> @@ -43,6 +43,7 @@ libspice_server_la_LIBADD = \
>> libspice_server_la_SOURCES = \
>> agent-msg-filter.c \
>> agent-msg-filter.h \
>> + char_device.c \
>> char_device.h \
>> demarshallers.h \
>> glz_encoder.c \
>> diff --git a/server/char_device.c b/server/char_device.c
>> new file mode 100644
>> index 0000000..13a3a58
>> --- /dev/null
>> +++ b/server/char_device.c
>> @@ -0,0 +1,741 @@
>> +/* spice-server char device flow control code
>> +
>> + Copyright (C) 2012 Red Hat, Inc.
>> +
>> + Red Hat Authors:
>> + Yonit Halperin<yhalperi at redhat.com>
>> +
>> + This library is free software; you can redistribute it and/or
>> + modify it under the terms of the GNU Lesser General Public
>> + License as published by the Free Software Foundation; either
>> + version 2.1 of the License, or (at your option) any later version.
>> +
>> + This library is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + Lesser General Public License for more details.
>> +
>> + You should have received a copy of the GNU Lesser General Public
>> + License along with this library; if not, see<http:www.gnu.org/licenses/>.
>> +*/
>> +
>> +
>> +#include "char_device.h"
>> +#include "red_channel.h"
>> +#include "reds.h"
>> +
>> +#define CHAR_DEVICE_WRITE_TO_TIMEOUT 100
>> +#define SPICE_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT 30000
>> +
>> +typedef struct SpiceCharDeviceClientState SpiceCharDeviceClientState;
>> +struct SpiceCharDeviceClientState {
>> + RingItem link;
>> + SpiceCharDeviceState *dev;
>> + RedClient *client;
>> + int do_flow_control;
>> + uint64_t num_client_tokens;
>> + uint64_t num_client_tokens_free; /* client messages that were consumed by the device */
>> + 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;
>> + uint32_t max_send_queue_size;
>> +};
>> +
>> +/* Holding references for avoiding access violation if the char device was
>> + * destroyed during a callback */
>> +static void spice_char_device_state_ref(SpiceCharDeviceState *char_dev);
>> +static void spice_char_device_state_unref(SpiceCharDeviceState *char_dev);
>> +
>> +static void spice_char_dev_write_retry(void *opaque);
>> +
>> +typedef struct SpiceCharDeviceMsgToClientItem {
>> + RingItem link;
>> + SpiceCharDeviceMsgToClient *msg;
>> +} SpiceCharDeviceMsgToClientItem;
>> +
>> +static void write_buffers_queue_free(Ring *write_queue)
>> +{
>> + while (!ring_is_empty(write_queue)) {
>> + RingItem *item = ring_get_tail(write_queue);
>> + SpiceCharDeviceWriteBuffer *buf;
>> +
>> + ring_remove(item);
>> + buf = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer, link);
>> + free(buf->buf);
>> + free(buf);
>> + }
>> +}
>> +
>> +static void spice_char_device_write_buffer_pool_add(SpiceCharDeviceState *dev,
>> + SpiceCharDeviceWriteBuffer *buf)
>> +{
>> + buf->buf_used = 0;
>> + buf->client_origin = NULL;
>> + ring_add(&dev->write_bufs_pool,&buf->link);
>> +}
>> +
>> +static void spice_char_device_client_send_queue_free(SpiceCharDeviceState *dev,
>> + SpiceCharDeviceClientState *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);
>> + SpiceCharDeviceMsgToClientItem *msg_item = SPICE_CONTAINEROF(item,
>> + SpiceCharDeviceMsgToClientItem,
>> + link);
>> +
>> + ring_remove(item);
>> + dev->cbs.unref_msg_to_client(msg_item->msg, dev->opaque);
>> + free(msg_item);
>> + }
>> + dev_client->num_send_tokens += dev_client->send_queue_size;
>> + dev_client->send_queue_size = 0;
>> +}
>> +
>> +static void spice_char_device_client_free(SpiceCharDeviceState *dev,
>> + SpiceCharDeviceClientState *dev_client)
>> +{
>> + RingItem *item, *next;
>> +
>> + if (dev_client->wait_for_tokens_timer) {
>> + core->timer_remove(dev_client->wait_for_tokens_timer);
>> + }
>> +
>> + spice_char_device_client_send_queue_free(dev, dev_client);
>> +
>> + /* remove write buffers that are associated with the client */
>> + spice_debug("write_queue_is_empty %d", ring_is_empty(&dev->write_queue)&& !dev->cur_write_buf);
>> + RING_FOREACH_SAFE(item, next,&dev->write_queue) {
>> + SpiceCharDeviceWriteBuffer *write_buf;
>> +
>> + write_buf = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer, link);
>> + if (write_buf->client_origin == dev_client->client) {
>> + ring_remove(item);
>> + spice_char_device_write_buffer_pool_add(dev, write_buf);
>> + }
>> + }
>> +
>> + if (dev->cur_write_buf&&
>> + dev->cur_write_buf->client_origin == dev_client->client) {
>> + dev->cur_write_buf->client_origin = NULL;
>
> By setting client_origin to NULL when we release this buffer via
> spice_char_device_write_to_device->spice_char_device_write_buffer_release
> we will increment num_self_tokens wrongly. Perhaps use a special -1
> value (will never be used by a real pointer that is 4 or 8 bytes
> aligned)?
>
Good catch. I'll add enum for the origin type.
>> + }
>> +
>> + dev->num_clients--;
>> + ring_remove(&dev_client->link);
>> + free(dev_client);
>> +}
>> +
>> +static void spice_char_device_handle_client_overflow(SpiceCharDeviceClientState *dev_client)
>> +{
>> + SpiceCharDeviceState *dev = dev_client->dev;
>> + spice_printerr("dev %p client %p ", dev, dev_client);
>> + dev->cbs.remove_client(dev_client->client, dev->opaque);
>> +}
>> +
>> +static SpiceCharDeviceClientState *spice_char_device_client_find(SpiceCharDeviceState *dev,
>> + RedClient *client)
>> +{
>> + RingItem *item;
>> +
>> + RING_FOREACH(item,&dev->clients) {
>> + SpiceCharDeviceClientState *dev_client;
>> +
>> + dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState, link);
>> + if (dev_client->client == client) {
>> + return dev_client;
>> + }
>> + }
>> + return NULL;
>> +}
>> +
>> +/***************************
>> + * Reading from the device *
>> + **************************/
>> +
>> +static void device_client_wait_for_tokens_timeout(void *opaque)
>> +{
>> + SpiceCharDeviceClientState *dev_client = opaque;
>> +
>> + spice_char_device_handle_client_overflow(dev_client);
>> +}
>> +
>> +static uint64_t spice_char_device_max_send_tokens(SpiceCharDeviceState *dev)
>> +{
>> + RingItem *item;
>> + uint64_t max = 0;
>> +
>> + RING_FOREACH(item,&dev->clients) {
>> + SpiceCharDeviceClientState *dev_client;
>> +
>> + dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState, link);
>> + if (dev_client->num_send_tokens> max) {
>> + max = dev_client->num_send_tokens;
>> + }
>> + }
>> + return max;
>> +}
>> +
>> +static void spice_char_device_add_msg_to_client_queue(SpiceCharDeviceClientState *dev_client,
>> + SpiceCharDeviceMsgToClient *msg)
>> +{
>> + SpiceCharDeviceState *dev = dev_client->dev;
>> + SpiceCharDeviceMsgToClientItem *msg_item;
>> +
>> + if (dev_client->send_queue_size>= dev_client->max_send_queue_size) {
>> + spice_char_device_handle_client_overflow(dev_client);
>> + return;
>> + }
>> +
>> + msg_item = spice_new0(SpiceCharDeviceMsgToClientItem, 1);
>> + msg_item->msg = dev->cbs.ref_msg_to_client(msg, dev->opaque);
>> + ring_add(&dev_client->send_queue,&msg_item->link);
>> + dev_client->send_queue_size++;
>> + if (!dev_client->wait_for_tokens_started) {
>> + core->timer_start(dev_client->wait_for_tokens_timer,
>> + SPICE_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
>> + dev_client->wait_for_tokens_started = TRUE;
>> + }
>> +}
>> +
>> +static void spice_char_device_send_msg_to_client(SpiceCharDeviceState *dev,
>
> nitpick: add s at the end? i.e.
> s/spice_char_device_send_msg_to_client/spice_char_device_send_msg_to_clients/
>
sure
>> + SpiceCharDeviceMsgToClient *msg)
>> +{
>> + RingItem *item, *next;
>> +
>> + RING_FOREACH_SAFE(item, next,&dev->clients) {
>> + SpiceCharDeviceClientState *dev_client;
>> +
>> + dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState, link);
>> + if (dev_client->num_send_tokens) {
>> + dev_client->num_send_tokens--;
>> + spice_assert(ring_is_empty(&dev_client->send_queue));
>> + dev->cbs.send_msg_to_client(msg, dev_client->client, dev->opaque);
>> +
>> + /* don't refer to dev_client anymore, it may have been released */
>> + } else {
>> + spice_char_device_add_msg_to_client_queue(dev_client, msg);
>> + }
>> + }
>> +}
>> +
>> +static int spice_char_device_read_from_device(SpiceCharDeviceState *dev)
>> +{
>> + static int inside_call = 0;
>
> This is a function static, if we have multiple threads (sometime in the
> future?) it will break. Not a problem right now though.
>
>> + uint64_t max_send_tokens;
>> + int did_read = FALSE;
>> +
>> + if (!dev->running) {
>> + return FALSE;
>> + }
>> +
>> + /* There are 2 scenarios where we can get called recursively:
>> + * 1) spice-vmc vmc_read triggering flush of throttled data, recalling wakeup
>> + * (virtio)
>> + * 2) in case of sending messages to the client, and unreferencing the
>> + * msg, we trigger another read.
>> + */
>> + if (inside_call++> 0) {
>> + return FALSE;
>> + }
>> +
>> + max_send_tokens = spice_char_device_max_send_tokens(dev);
>> + spice_char_device_state_ref(dev);
>> + /*
>> + * Reading from the device only in case at least one of the clients have a free token.
>> + * All messages will be discarded if no client is attached to the device
>> + */
>> + while (max_send_tokens || ring_is_empty(&dev->clients)) {
>> + SpiceCharDeviceMsgToClient *msg;
>> +
>> + msg = dev->cbs.read_one_msg_from_device(dev->sin, dev->opaque);
>> + if (!msg) {
>> + if (inside_call> 1) {
>> + inside_call = 1;
>> + continue; /* a wakeup might have been called during the read -
>> + make sure it doesn't get lost */
>> + }
>> + break;
>> + }
>> + did_read = TRUE;
>> + spice_char_device_send_msg_to_client(dev, msg);
>> + dev->cbs.unref_msg_to_client(msg, dev->opaque);
>> + max_send_tokens--;
>> + }
>> + inside_call = 0;
>> + spice_char_device_state_unref(dev);
>> + return did_read;
>> +}
>> +
>> +static void spice_char_device_client_send_queue_push(SpiceCharDeviceClientState *dev_client)
>> +{
>> + RingItem *item;
>> + while ((item = ring_get_tail(&dev_client->send_queue))&& dev_client->num_send_tokens) {
>> + SpiceCharDeviceMsgToClientItem *msg_item;
>> +
>> + msg_item = SPICE_CONTAINEROF(item, SpiceCharDeviceMsgToClientItem, link);
>> + ring_remove(item);
>> +
>> + dev_client->num_send_tokens--;
>> + dev_client->dev->cbs.send_msg_to_client(msg_item->msg,
>> + dev_client->client,
>> + dev_client->dev->opaque);
>> + dev_client->dev->cbs.unref_msg_to_client(msg_item->msg, dev_client->dev->opaque);
>> + dev_client->send_queue_size--;
>> + free(msg_item);
>> + }
>> +}
>> +
>> +void spice_char_device_send_to_client_tokens_add(SpiceCharDeviceState *dev,
>> + RedClient *client,
>> + uint32_t tokens)
>> +{
>> + SpiceCharDeviceClientState *dev_client;
>> +
>> + dev_client = spice_char_device_client_find(dev, client);
>> +
>> + if (!dev_client) {
>> + spice_error("client wasn't found dev %p client %p", dev, client);
>> + return;
>> + }
>> +
>> + dev_client->num_send_tokens += tokens;
>> +
>> + if (dev_client->send_queue_size) {
>> + spice_assert(dev_client->num_send_tokens == tokens);
>> + spice_char_device_client_send_queue_push(dev_client);
>> + }
>> +
>> + if (dev_client->num_send_tokens) {
>> + core->timer_cancel(dev_client->wait_for_tokens_timer);
>> + dev_client->wait_for_tokens_started = FALSE;
>> + spice_char_device_read_from_device(dev);
>> + } else if (dev_client->send_queue_size) {
>> + core->timer_start(dev_client->wait_for_tokens_timer,
>> + SPICE_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
>> + dev_client->wait_for_tokens_started = TRUE;
>> + }
>> +}
>> +
>> +void spice_char_device_send_to_client_tokens_set(SpiceCharDeviceState *dev,
>> + RedClient *client,
>> + uint32_t tokens)
>> +{
>> + SpiceCharDeviceClientState *dev_client;
>> +
>> + dev_client = spice_char_device_client_find(dev, client);
>> +
>> + if (!dev_client) {
>> + spice_error("client wasn't found dev %p client %p", dev, client);
>> + return;
>> + }
>> +
>> + dev_client->num_send_tokens = tokens;
>> +
>> + if (dev_client->send_queue_size) {
>> + spice_assert(dev_client->num_send_tokens == tokens);
>> + spice_char_device_client_send_queue_push(dev_client);
>> + }
>> +
>> + if (dev_client->num_send_tokens) {
>> + core->timer_cancel(dev_client->wait_for_tokens_timer);
>> + dev_client->wait_for_tokens_started = FALSE;
>> + spice_char_device_read_from_device(dev);
>> + } else if (dev_client->send_queue_size) {
>> + core->timer_start(dev_client->wait_for_tokens_timer,
>> + SPICE_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
>> + dev_client->wait_for_tokens_started = TRUE;
>> + }
>> +}
>> +
>> +/**************************
>> + * Writing to the device *
>> +***************************/
>> +
>> +static void spice_char_device_client_token_add(SpiceCharDeviceState *dev,
>> + SpiceCharDeviceClientState *dev_client)
>> +{
>> + if (++dev_client->num_client_tokens_free == dev->client_tokens_interval) {
>> + dev_client->num_client_tokens += dev->client_tokens_interval;
>> + dev_client->num_client_tokens_free = 0;
>> + if (dev_client->do_flow_control) {
>> + dev->cbs.send_tokens_to_client(dev_client->client,
>> + dev->client_tokens_interval,
>> + dev->opaque);
>> + }
>> + }
>> +}
>> +
>> +static int spice_char_device_write_to_device(SpiceCharDeviceState *dev)
>> +{
>> + SpiceCharDeviceInterface *sif;
>> + int total = 0;
>> + int n;
>> +
>> + if (!dev->running) {
>> + return 0;
>> + }
>> +
>> + spice_char_device_state_ref(dev);
>> + core->timer_cancel(dev->write_to_dev_timer);
>> +
>> + sif = SPICE_CONTAINEROF(dev->sin->base.sif, SpiceCharDeviceInterface, base);
>> + while (1) {
>> + uint32_t write_len;
>> +
>> + if (!dev->cur_write_buf) {
>> + RingItem *item = ring_get_tail(&dev->write_queue);
>> + if (!item) {
>> + break;
>> + }
>> + dev->cur_write_buf = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer, link);
>> + dev->cur_write_buf_pos = dev->cur_write_buf->buf;
>> + ring_remove(item);
>> + }
>> +
>> + write_len = dev->cur_write_buf->buf + dev->cur_write_buf->buf_used -
>> + dev->cur_write_buf_pos;
>> + n = sif->write(dev->sin, dev->cur_write_buf_pos, write_len);
>> + if (n<= 0) {
>> + break;
>> + }
>> + total += n;
>> + write_len -= n;
>> + if (!write_len) {
>> + SpiceCharDeviceWriteBuffer *release_buf = dev->cur_write_buf;
>> + dev->cur_write_buf = NULL;
>> + spice_char_device_write_buffer_release(dev, release_buf);
>> + continue;
>> + }
>> + dev->cur_write_buf_pos += n;
>> + }
>> + /* retry writing as long as the write queue is not empty */
>> + if (dev->cur_write_buf) {
>> + core->timer_start(dev->write_to_dev_timer, CHAR_DEVICE_WRITE_TO_TIMEOUT);
>> + } else {
>> + spice_assert(ring_is_empty(&dev->write_queue));
>> + }
>> + spice_char_device_state_unref(dev);
>> + return total;
>> +}
>> +
>> +static void spice_char_dev_write_retry(void *opaque)
>> +{
>> + SpiceCharDeviceState *dev = opaque;
>> +
>> + core->timer_cancel(dev->write_to_dev_timer);
>> + spice_char_device_write_to_device(dev);
>> +}
>> +
>> +SpiceCharDeviceWriteBuffer *spice_char_device_write_buffer_get(SpiceCharDeviceState *dev,
>> + RedClient *client, int size)
>> +{
>> + RingItem *item;
>> + SpiceCharDeviceWriteBuffer *ret;
>> +
>> + if (!client&& !dev->num_self_tokens) {
>> + spice_printerr("internal buf is not available");
>> + return NULL;
>> + }
>> +
>> + if ((item = ring_get_tail(&dev->write_bufs_pool))) {
>> + ret = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer, link);
>> + ring_remove(item);
>> + } else {
>> + ret = spice_new0(SpiceCharDeviceWriteBuffer, 1);
>> + }
>> +
>> + spice_assert(!ret->buf_used);
>> +
>> + if (ret->buf_size< size) {
>> + ret->buf = spice_realloc(ret->buf, size);
>> + ret->buf_size = size;
>> + }
>> +
>> + if (client) {
>> + SpiceCharDeviceClientState *dev_client = spice_char_device_client_find(dev, client);
>> + if (dev_client) {
>> + if (!dev_client->num_client_tokens) {
>> + spice_printerr("token violation: dev %p client %p", dev, client);
>> + spice_char_device_handle_client_overflow(dev_client);
>> + goto error;
>> + }
>> + ret->client_origin = client;
>> + dev_client->num_client_tokens--;
>> + } else {
>> + /* it is possible that the client was removed due to send tokens underflow, but
>> + * the caller still receive messages from the client */
>> + spice_printerr("client not found: dev %p client %p", dev, client);
>> + goto error;
>> + }
>> + } else {
>> + dev->num_self_tokens--;
>> + }
>> +
>> + return ret;
>> +error:
>> + ring_add(&dev->write_bufs_pool,&ret->link);
>> + return NULL;
>> +}
>> +
>> +void spice_char_device_write_buffer_add(SpiceCharDeviceState *dev,
>> + SpiceCharDeviceWriteBuffer *write_buf)
>> +{
>> + spice_assert(dev);
>> + /* caller shouldn't add buffers for client that was removed */
>> + if (write_buf->client_origin&&
>> + !spice_char_device_client_find(dev, write_buf->client_origin)) {
>> + spice_printerr("client not found: dev %p client %p", dev, write_buf->client_origin);
>> + spice_char_device_write_buffer_pool_add(dev, write_buf);
>> + return;
>> + }
>> +
>> + ring_add(&dev->write_queue,&write_buf->link);
>> + spice_char_device_write_to_device(dev);
>> +}
>> +
>> +void spice_char_device_write_buffer_release(SpiceCharDeviceState *dev,
>> + SpiceCharDeviceWriteBuffer *write_buf)
>> +{
>> + RedClient *client = write_buf->client_origin;
>> +
>> + spice_assert(!ring_item_is_linked(&write_buf->link));
>> + if (!dev) {
>> + spice_printerr("no device. write buffer is freed");
>> + free(write_buf->buf);
>> + free(write_buf);
>> + return;
>> + }
>> +
>> + spice_assert(dev->cur_write_buf != write_buf);
>> +
>> + spice_char_device_write_buffer_pool_add(dev, write_buf);
>> + if (client) {
>> + SpiceCharDeviceClientState *dev_client;
>> + dev_client = spice_char_device_client_find(dev, client);
>> + /* when a client is removed, we remove all the buffers that are associated with it */
>> + spice_assert(dev_client);
>> + spice_char_device_client_token_add(dev, dev_client);
>> + } else {
>> + dev->num_self_tokens++;
>> + if (dev->cbs.on_free_self_token) {
>> + dev->cbs.on_free_self_token(dev->opaque);
>> + }
>> + }
>> +
>> +}
>> +
>> +
>> +/********************************
>> + * char_device_state management *
>> + ********************************/
>> +
>> +SpiceCharDeviceState *spice_char_device_state_create(SpiceCharDeviceInstance *sin,
>> + uint32_t client_tokens_interval,
>> + uint32_t self_tokens,
>> + SpiceCharDeviceCallbacks *cbs,
>> + void *opaque)
>> +{
>> + SpiceCharDeviceState *char_dev;
>> +
>> + spice_assert(sin);
>> + spice_assert(cbs->read_one_msg_from_device&& cbs->ref_msg_to_client&&
>> + cbs->unref_msg_to_client&& cbs->send_msg_to_client&&
>> + cbs->send_tokens_to_client&& cbs->remove_client);
>> +
>> + char_dev = spice_new0(SpiceCharDeviceState, 1);
>> + char_dev->sin = sin;
>> + char_dev->cbs = *cbs;
>> + char_dev->opaque = opaque;
>> + char_dev->client_tokens_interval = client_tokens_interval;
>> + char_dev->num_self_tokens = self_tokens;
>> +
>> + ring_init(&char_dev->write_queue);
>> + ring_init(&char_dev->write_bufs_pool);
>> + ring_init(&char_dev->clients);
>> +
>> + char_dev->write_to_dev_timer = core->timer_add(spice_char_dev_write_retry, char_dev);
>> + if (!char_dev->write_to_dev_timer) {
>> + spice_error("failed creating char dev write timer");
>> + }
>> + char_dev->refs = 1;
>> + sin->st = char_dev;
>> + spice_debug("sin %p dev_state %p", sin, char_dev);
>> + return char_dev;
>> +}
>> +
>> +void spice_char_device_state_reset_dev_instance(SpiceCharDeviceState *state,
>> + SpiceCharDeviceInstance *sin)
>> +{
>> + spice_debug("sin %p dev_state %p", sin, state);
>> + state->sin = sin;
>> + sin->st = state;
>> +}
>> +
>> +void *spice_char_device_state_opaque_get(SpiceCharDeviceState *dev)
>> +{
>> + return dev->opaque;
>> +}
>> +
>> +static void spice_char_device_state_ref(SpiceCharDeviceState *char_dev)
>> +{
>> + char_dev->refs++;
>> +}
>> +
>> +static void spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
>> +{
>> + if (!--char_dev->refs) {
>
> Here refs == 0, but then below..
>
>> + spice_char_device_state_destroy(char_dev);
>> + }
>> +}
>> +
>> +void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
>> +{
>> + core->timer_remove(char_dev->write_to_dev_timer);
>> + write_buffers_queue_free(&char_dev->write_queue);
>> + write_buffers_queue_free(&char_dev->write_bufs_pool);
>> +
>> + while (!ring_is_empty(&char_dev->clients)) {
>> + RingItem *item = ring_get_tail(&char_dev->clients);
>> + SpiceCharDeviceClientState *dev_client;
>> +
>> + dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState, link);
>> + spice_char_device_client_free(char_dev, dev_client);
>> + }
>> + char_dev->running = FALSE;
>> +
>> + if (!--char_dev->refs) {
>
> refs is uint32_t, so it will now be large positive, this will never be
> called. Drop the if.
>
Oops, forgot to remove this. Another good catch!
Thanks,
Yonit.
>> + free(char_dev);
>> + }
>> +}
>> +
>> +void spice_char_device_client_add(SpiceCharDeviceState *dev,
>> + RedClient *client,
>> + int do_flow_control,
>> + uint32_t max_send_queue_size,
>> + uint32_t num_client_tokens,
>> + uint32_t num_send_tokens)
>> +{
>> + SpiceCharDeviceClientState *dev_client;
>> +
>> + spice_assert(dev);
>> + spice_assert(client);
>> +
>> + spice_debug("dev_state %p client %p", dev, client);
>> + dev_client = spice_new0(SpiceCharDeviceClientState, 1);
>> + dev_client->dev = dev;
>> + dev_client->client = client;
>> + ring_init(&dev_client->send_queue);
>> + dev_client->send_queue_size = 0;
>> + dev_client->max_send_queue_size = max_send_queue_size;
>> + dev_client->do_flow_control = do_flow_control;
>> + if (do_flow_control) {
>> + dev_client->wait_for_tokens_timer = core->timer_add(device_client_wait_for_tokens_timeout,
>> + dev_client);
>> + if (!dev_client->wait_for_tokens_timer) {
>> + spice_error("failed to create wait for tokens timer");
>> + }
>> + dev_client->num_client_tokens = num_client_tokens;
>> + dev_client->num_send_tokens = num_send_tokens;
>> + } else {
>> + dev_client->num_client_tokens = ~0;
>> + dev_client->num_send_tokens = ~0;
>> + }
>> + ring_add(&dev->clients,&dev_client->link);
>> + dev->num_clients++;
>> + /* Now that we have a client, forward any pending device data */
>> + spice_char_device_wakeup(dev);
>> +}
>> +
>> +
>> +uint32_t spice_char_device_client_num_tokens_get(SpiceCharDeviceState *dev,
>> + RedClient *client)
>> +{
>> + SpiceCharDeviceClientState *dev_client;
>> +
>> + dev_client = spice_char_device_client_find(dev, client);
>> +
>> + if (!dev_client) {
>> + spice_error("client wasn't found");
>> + return 0;
>> + }
>> +
>> + return dev_client->num_client_tokens;
>> +}
>> +
>> +void spice_char_device_client_remove(SpiceCharDeviceState *dev,
>> + RedClient *client)
>> +{
>> + SpiceCharDeviceClientState *dev_client;
>> +
>> + spice_debug("dev_state %p client %p", dev, client);
>> + dev_client = spice_char_device_client_find(dev, client);
>> +
>> + if (!dev_client) {
>> + spice_error("client wasn't found");
>> + return;
>> + }
>> +
>> + spice_char_device_client_free(dev, dev_client);
>> +}
>> +
>> +int spice_char_device_client_exists(SpiceCharDeviceState *dev,
>> + RedClient *client)
>> +{
>> + return (spice_char_device_client_find(dev, client) != NULL);
>> +}
>> +
>> +void spice_char_device_start(SpiceCharDeviceState *dev)
>> +{
>> + spice_debug("dev_state %p", dev);
>> + dev->running = TRUE;
>> + spice_char_device_state_ref(dev);
>> + while (spice_char_device_write_to_device(dev) ||
>> + spice_char_device_read_from_device(dev));
>> + spice_char_device_state_unref(dev);
>> +}
>> +
>> +void spice_char_device_stop(SpiceCharDeviceState *dev)
>> +{
>> + spice_debug("dev_state %p", dev);
>> + dev->running = FALSE;
>> + core->timer_cancel(dev->write_to_dev_timer);
>> +}
>> +
>> +void spice_char_device_reset(SpiceCharDeviceState *dev)
>> +{
>> + RingItem *client_item;
>> + spice_char_device_stop(dev);
>> +
>> + spice_debug("dev_state %p", dev);
>> + while (!ring_is_empty(&dev->write_queue)) {
>> + RingItem *item = ring_get_tail(&dev->write_queue);
>> + SpiceCharDeviceWriteBuffer *buf;
>> +
>> + ring_remove(item);
>> + buf = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer, link);
>> + /* tracking the tokens */
>> + spice_char_device_write_buffer_release(dev, buf);
>> + }
>> + if (dev->cur_write_buf) {
>> + SpiceCharDeviceWriteBuffer *release_buf = dev->cur_write_buf;
>> +
>> + dev->cur_write_buf = NULL;
>> + spice_char_device_write_buffer_release(dev, release_buf);
>> + }
>> +
>> + RING_FOREACH(client_item,&dev->clients) {
>> + SpiceCharDeviceClientState *dev_client;
>> +
>> + dev_client = SPICE_CONTAINEROF(client_item, SpiceCharDeviceClientState, link);
>> + spice_char_device_client_send_queue_free(dev, dev_client);
>> + }
>> + dev->sin = NULL;
>> +}
>> +
>> +void spice_char_device_wakeup(SpiceCharDeviceState *dev)
>> +{
>> + spice_char_device_read_from_device(dev);
>> +}
>> +
>> diff --git a/server/char_device.h b/server/char_device.h
>> index bdb32ae..e3cd52d 100644
>> --- a/server/char_device.h
>> +++ b/server/char_device.h
>> @@ -2,11 +2,209 @@
>> #define __CHAR_DEVICE_H__
>>
>> #include "spice.h"
>> +#include "red_channel.h"
>> +
>> +/*
>> + * Shared code for char devices, mainly for flow control.
>> + *
>> + * How to use the api:
>> + * ==================
>> + * device attached: call spice_char_device_state_create
>> + * device detached: call spice_char_device_state_destroy/reset
>> + *
>> + * client connected and assoicated with a device: spice_char_device_client_add
>> + * client disconnected: spice_char_device_client_remove
>> + *
>> + * Writing to the device
>> + * ---------------------
>> + * Write the data into SpiceCharDeviceWriteBuffer:
>> + * call spice_char_device_write_buffer_get in order to get an appropriate buffer.
>> + * call spice_char_device_write_buffer_add in order to push the buffer to the write queue.
>> + * If you choose not to push the buffer to the device, call
>> + * spice_char_device_write_buffer_release
>> + *
>> + * reading from the device
>> + * -----------------------
>> + * The callback read_one_msg_from_device (see below) should be implemented
>> + * (using sif->read).
>> + * When the device is ready, this callback is called, and is expected to
>> + * return one message which is addressed to the client, or NULL if the read
>> + * hasn't completed.
>> + *
>> + * calls triggered from the device (qemu):
>> + * --------------------------------------
>> + * spice_char_device_start
>> + * spice_char_device_stop
>> + * spice_char_device_wakeup (for reading from the device)
>> + */
>> +
>> +/*
>> + * Note about multiple-clients:
>> + * Multiclients are currently not supported in any of the character devices:
>> + * spicevmc does not allow more than one client (and at least for usb, it should stay this way).
>> + * smartcard code is not compatible with more than one reader.
>> + * The server and guest agent code doesn't distinguish messages from different clients.
>> + * In addition, its current flow control code (e.g., tokens handling) is wrong and doesn't
>> + * take into account the different clients.
>> + *
>> + * Nonetheless, the following code introduces some support for multiple-clients:
>> + * We track the number of tokens for all the clients, and we read from the device
>> + * if one of the clients have enough tokens. For the clients that don't have tokens,
>> + * we queue the messages, till they receive tokens, or till a timeout.
>> + *
>> + * TODO:
>> + * At least for the agent, not all the messages from the device will be directed to all
>> + * the clients (e.g., copy from guest to a specific client). Thus, support for
>> + * client-specific-messages should be added.
>> + * In addition, we should have support for clients that are being connected
>> + * in the middle of a message transfer from the agent to the clients.
>> + *
>> + * */
>> +
>> +/* buffer that is used for writing to the device */
>> +typedef struct SpiceCharDeviceWriteBuffer {
>> + RingItem link;
>> + RedClient *client_origin; /* The client that sent the message to the device.
>> + NULL if the server created the message */
>> +
>> + uint8_t *buf;
>> + uint32_t buf_size;
>> + uint32_t buf_used;
>> +} SpiceCharDeviceWriteBuffer;
>> +
>> +typedef void SpiceCharDeviceMsgToClient;
>> +
>> +typedef struct SpiceCharDeviceCallbacks {
>> + /*
>> + * Messages that are addressed to the client can be queued in case we have
>> + * multiple clients and some of them don't have enough tokens.
>> + */
>> +
>> + /* reads from the device till reaching a msg that should be sent to the client,
>> + * or till the reading fails */
>> + SpiceCharDeviceMsgToClient* (*read_one_msg_from_device)(SpiceCharDeviceInstance *sin,
>> + void *opaque);
>> + SpiceCharDeviceMsgToClient* (*ref_msg_to_client)(SpiceCharDeviceMsgToClient *msg,
>> + void *opaque);
>> + void (*unref_msg_to_client)(SpiceCharDeviceMsgToClient *msg,
>> + void *opaque);
>> + void (*send_msg_to_client)(SpiceCharDeviceMsgToClient *msg,
>> + RedClient *client,
>> + void *opaque); /* after this call, the message is unreferenced */
>> +
>> + /* The cb is called when a predefined number of write buffers were consumed by the
>> + * device */
>> + void (*send_tokens_to_client)(RedClient *client, uint32_t tokens, void *opaque);
>> +
>> + /* The cb is called when a server (self) message that was addressed to the device,
>> + * has been completely written to it */
>> + void (*on_free_self_token)(void *opaque);
>> +
>> + /* This cb is called if it is recommanded that a client will be removed
>> + * due to slow flow or due to some other error.
>> + * The called instance should disconnect the client, or at least the corresponding channel */
>> + void (*remove_client)(RedClient *client, void *opaque);
>> +} SpiceCharDeviceCallbacks;
>> +
>> +typedef struct SpiceCharDeviceState SpiceCharDeviceState;
>>
>> struct SpiceCharDeviceState {
>> + int running;
>> + uint32_t refs;
>> +
>> + Ring write_queue;
>> + Ring write_bufs_pool;
>> + SpiceCharDeviceWriteBuffer *cur_write_buf;
>> + uint8_t *cur_write_buf_pos;
>> + SpiceTimer *write_to_dev_timer;
>> + uint64_t num_self_tokens;
>> +
>> + Ring clients;
>> + uint32_t num_clients;
>> +
>> + uint64_t client_tokens_interval; /* frequency of returning tokens to the client */
>> + SpiceCharDeviceInstance *sin;
>> +
>> + SpiceCharDeviceCallbacks cbs;
>> + void *opaque;
>> + /* tmp till all spice char devices will employ the new SpiceCharDeviceState
>> + * implementation. Then, SpiceCharDeviceState will be moved to char_device.c and
>> + * this callback will be removed */
>> void (*wakeup)(SpiceCharDeviceInstance *sin);
>> };
>>
>> +
>> +SpiceCharDeviceState *spice_char_device_state_create(SpiceCharDeviceInstance *sin,
>> + uint32_t client_tokens_interval,
>> + uint32_t self_tokens,
>> + SpiceCharDeviceCallbacks *cbs,
>> + void *opaque);
>> +
>> +void spice_char_device_state_reset_dev_instance(SpiceCharDeviceState *dev,
>> + SpiceCharDeviceInstance *sin);
>> +void spice_char_device_state_destroy(SpiceCharDeviceState *dev);
>> +
>> +void *spice_char_device_state_opaque_get(SpiceCharDeviceState *dev);
>> +
>> +
>> +/*
>> + * Resets write/read queues, and moves that state to being stopped.
>> + * This routine is a workaround for a bad tokens management in the vdagent
>> + * protocol:
>> + * The client tokens' are set only once, when the main channel is initialized.
>> + * Instead, it would have been more appropriate to reset them upon AGEN_CONNECT.
>> + * The client tokens are tracked as part of the SpiceCharDeviceClientState. Thus,
>> + * in order to be backwartd compatible with the client, we need to track the tokens
>> + * event when the agent is detached. We don't destroy the the char_device state, and
>> + * instead we just reset it.
>> + * In addition, there is a misshandling of AGENT_TOKENS message in spice-gtk: it
>> + * overrides the amount of tokens, instead of adding the given amount.
>> + *
>> + * todo: change AGENT_CONNECT msg to contain tokens count.
>> + */
>> +void spice_char_device_reset(SpiceCharDeviceState *dev);
>> +
>> +/* max_send_queue_size = how many messages we can read from the device and enqueue for this client,
>> + * when we have tokens for other clients and no tokens for this one */
>> +void spice_char_device_client_add(SpiceCharDeviceState *dev,
>> + RedClient *client,
>> + int do_flow_control,
>> + uint32_t max_send_queue_size,
>> + uint32_t num_client_tokens,
>> + uint32_t num_send_tokens);
>> +
>> +void spice_char_device_client_remove(SpiceCharDeviceState *dev,
>> + RedClient *client);
>> +int spice_char_device_client_exists(SpiceCharDeviceState *dev,
>> + RedClient *client);
>> +
>> +void spice_char_device_start(SpiceCharDeviceState *dev);
>> +void spice_char_device_stop(SpiceCharDeviceState *dev);
>> +
>> +/** Read from device **/
>> +
>> +void spice_char_device_wakeup(SpiceCharDeviceState *dev);
>> +
>> +void spice_char_device_send_to_client_tokens_add(SpiceCharDeviceState *dev,
>> + RedClient *client,
>> + uint32_t tokens);
>> +
>> +
>> +void spice_char_device_send_to_client_tokens_set(SpiceCharDeviceState *dev,
>> + RedClient *client,
>> + uint32_t tokens);
>> +/** Write to device **/
>> +
>> +SpiceCharDeviceWriteBuffer *spice_char_device_write_buffer_get(SpiceCharDeviceState *dev,
>> + RedClient *client, int size);
>> +/* Either add the buffer to the write queue or release it */
>> +void spice_char_device_write_buffer_add(SpiceCharDeviceState *dev,
>> + SpiceCharDeviceWriteBuffer *write_buf);
>> +void spice_char_device_write_buffer_release(SpiceCharDeviceState *dev,
>> + SpiceCharDeviceWriteBuffer *write_buf);
>> +
>> +/* api for specific char devices */
>> +
>> void spicevmc_device_connect(SpiceCharDeviceInstance *sin,
>> uint8_t channel_type);
>> void spicevmc_device_disconnect(SpiceCharDeviceInstance *char_device);
>> --
>> 1.7.7.6
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list