On 07/02/2012 01:36 PM, Hans de Goede wrote:
Hi, Overall looks good, 2 comments: 1) When not using flow-control you set the tokens to max_uint32, and assume that is enough, but with uwb-2 isoc traffic you can get upto 8000 packets / sec. Which translates to running out of tokens in approx 145 hours. So if someone leaves a spice-client viewing a feed from a usb-2 webcam open for 145 hours (so less then a week) we run out of tokens. Not a very likely scenario but still not good, so I suggest that instead we store the do_flow_control in the state, and don't deal with tokens at all when not doing flow control.
Good point.
2) These 2 functions are almost identical:+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; + } +}The only difference is: > + dev_client->num_send_tokens += tokens; versus: > + dev_client->num_send_tokens = tokens; Please refactor this into a private helper function doing all the work, and 2 wrappers for external use doing the set/add, removing the code duplication.
Will do. Thanks, Yonit.
Regards, Hans _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel