[Spice-devel] [PATCH spice-server 2/3] spicevmc: Do not use RedCharDevice pipe items handling

[Frediano Ziglio]

> 
> On 6/1/19 3:14 PM, 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.
> 
> Hi,
> 
> If we need flow control, how difficult would it be to add support
> for tokens ?
> ( there is a "todo: add flow control" comment in spicevmc ).
> 

Flow control is what the following patch added. I can remove
the comment.

There are different things I don't like on RedCharDevice token
handling:
- it's really designed with VDI in mind;
- it's using items as unit not allowing counting bytes (as my patch
  does);
- it duplicates some queue management between RedChannelClient;
- 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.
Not taking into account that if you have at maximum (like VMC and smartcard)
1 client it's just a dummy layer that you have to satisfy.

> Uri.
> 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >   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);
> >   }
> >   
> >   static void red_port_init_item_free(struct RedPipeItem *base)
> > 
> 
>