[Spice-devel] [PATCH spice-server 03/14] char_device: Introducing shared flow control code for char devices.

[Yonit Halperin]

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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel