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

Submitted by Frediano Ziglio on June 1, 2019, 12:14 p.m.

Details

Message ID 20190601121413.932-2-fziglio@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio June 1, 2019, 12:14 p.m.
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.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/spicevmc.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

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)

Comments

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 ).

Uri.

> 
> Signed-off-by: Frediano Ziglio <fziglio@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)
>
> 
> 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@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)
> > 
> 
>