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

[Hans de Goede]

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.

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.

Regards,

Hans