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

Submitted by Frediano Ziglio on June 17, 2019, 3:40 p.m.

Details

Message ID 20190617154011.20310-1-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 17, 2019, 3:40 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.

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@redhat.com>
---
Changes in v2:
- more commit message comments
---
 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

ping the series

> 
> 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@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);
>  }
>  
>  static void red_port_init_item_free(struct RedPipeItem *base)
ping

> 
> ping the series
> 
> > 
> > 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@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);
> >  }
> >  
> >  static void red_port_init_item_free(struct RedPipeItem *base)
ping

> 
> ping
> 
> > 
> > ping the series
> > 
> > > 
> > > 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@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);
> > >  }
> > >  
> > >  static void red_port_init_item_free(struct RedPipeItem *base)
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
ping

> 
> ping
> 
> > 
> > ping
> > 
> > > 
> > > ping the series
> > > 
> > > > 
> > > > 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@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);
> > > >  }
> > > >  
> > > >  static void red_port_init_item_free(struct RedPipeItem *base)
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
ping

> 
> ping
> 
> > 
> > ping
> > 
> > > 
> > > ping
> > > 
> > > > 
> > > > ping the series
> > > > 
> > > > > 
> > > > > 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@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);
> > > > >  }
> > > > >  
> > > > >  static void red_port_init_item_free(struct RedPipeItem *base)