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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel