[Spice-devel] [PATCH spice-server v2 1/4] spicevmc: Do not use RedCharDevice pipe items handling

[Victor Toso]

Hi,

On Mon, Jun 17, 2019 at 04:40:08PM +0100, Frediano Ziglio wrote:
> As we don't use any token there's no reason to not queue directly instead
> of passing through RedCharDevice.
> This will make easier to limit the queue which currently is unlimited.
> 
> RedCharDevice flow control has some problems:
> - it's really designed with VDI in mind. This for SpiceVMC would cause
>   code implementation to be confusing having to satisfy the agent token
>   handling;
> - it's using items as unit not allowing counting bytes;
> - it duplicates some queue management between RedChannelClient;
> - it's broken (see comments in char-device.h);
> - it forces "clients" to behave in some way not altering for instance the
>   queued items (which is very useful if items can be collapsed together);
> - it replicates the token handling on the device queue too. This could
>   seems right but is not that if you have a network card going @ 1 GBit/s
>   you are able to upload more than 1 Gbit/s just using multiple
>   connections. The kernel will use a single queue for the network interface
>   limiting and sharing de facto the various buffers between the multiple
>   connections.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> Changes in v2:
> - more commit message comments
> ---
>  server/spicevmc.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 84bbb73c2..8c41573ae 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -360,29 +360,24 @@ static RedPipeItem *spicevmc_chardev_read_msg_from_dev(RedCharDevice *self,
>  
>          msg_item_compressed = try_compress_lz4(channel, n, msg_item);
>          if (msg_item_compressed != NULL) {
> -            return &msg_item_compressed->base;
> +            red_channel_client_pipe_add_push(channel->rcc, &msg_item_compressed->base);
> +            return NULL;
>          }
>  #endif
>          stat_inc_counter(channel->out_data, n);
>          msg_item->uncompressed_data_size = n;
>          msg_item->buf_used = n;
> -        return &msg_item->base;
> -    } else {
> -        channel->pipe_item = msg_item;
> +        red_channel_client_pipe_add_push(channel->rcc, &msg_item->base);
>          return NULL;
>      }
> +    channel->pipe_item = msg_item;
> +    return NULL;
>  }
>  
>  static void spicevmc_chardev_send_msg_to_client(RedCharDevice *self,
>                                                  RedPipeItem *msg,
>                                                  RedClient *client)
>  {
> -    RedCharDeviceSpiceVmc *vmc = RED_CHAR_DEVICE_SPICEVMC(self);
> -    RedVmcChannel *channel = RED_VMC_CHANNEL(vmc->channel);
> -
> -    spice_assert(red_channel_client_get_client(channel->rcc) == client);
> -    red_pipe_item_ref(msg);
> -    red_channel_client_pipe_add_push(channel->rcc, msg);
>  }

Is it worth to 1) update char-device.h that this is is not used
in spicevmc; 2) update char-device.c with

    -   klass->send_msg_to_client(dev, msg, client);
    +   if (klass->send_msg_to_client) {
    +       klass->send_msg_to_client(dev, msg, client);
    +   }

and remove this function?;

Patch looks good.

>  static void red_port_init_item_free(struct RedPipeItem *base)
> -- 
> 2.20.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190924/53a50dd0/attachment-0001.sig>