[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 23:29:49 PDT 2012
Hi,
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.
>
<snip>
.
.
.
</snip>
>> +
>> +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.
>
spice_char_device_state_unref is always called coupled to
spice_char_device_state_ref. char_dev->refs can be 0
after spice_char_device_state_unref is called, only in case
spice_char_device_state_destroy is called in between
spice_char_device_state_ref and spice_char_device_state_unref. We can't
free(char_dev) in this scenario (that is why we needed the refs in the
first place).
We can't drop the "if". Instead, it should be,
if (char_dev->refs == 0)
free(char_dev)
else
char_dev->refs--
Regards,
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