Re: [Spice-devel] [PATCH spice-server 2/3] stream: implements flush using TCP_CORK

[Frediano Ziglio]

> 
> On Tue, Jan 16, 2018 at 02:18:14PM +, Frediano Ziglio wrote:
> > Cork is a system interface implemented by Linux and some *BSD systems to
> > tell the system that other data are expected to be written to a socket.
> > This allows the system to reduce network fragmentation waiting a network
> 
> 'waiting for network packets to be complete' I think.
> 
> > packet to be complete.
> > 
> > Using some replay capture and some instrumentation resulted in a
> > bandwith reduction of 11% and a packet reduction of 56%.
> 
> I would add a link to
> https://lists.freedesktop.org/archives/spice-devel/2017-February/035577.html
> and maybe even copy the data you put in there.
> 

Added part of it, the TLS part was not strictly related (and surely the
small TLS optimization tests does not belong here).

Specifically I added:

The tests was done using replay utility so results could be a bit different
from real cases as:
- replay goes as fast as it can, for instance packets could
  be merged by the kernel decreasing packet numbers and a bit
  byte spent (this actually make the following improves worse);
- there are fewer channels (no much cursor, sound, etc).
The following tests shows count packet and total bytes from server to
client using a real network. I used a direct cable connection using 1gb
connection and 2 laptops.

cork: 537 1582240
cork: 681 1823754
cork: 524 1583287
cork: 538 1582350
no cork: 1329 1834630
no cork: 1290 1829094
no cork: 1289 1830164
no cork: 1317 1833589
no cork: 1320 1835705

> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/red-stream.c | 34 +-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/red-stream.c b/server/red-stream.c
> > index 4812d8e4..4833077c 100644
> > --- a/server/red-stream.c
> > +++ b/server/red-stream.c
> > @@ -24,6 +24,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  
> > @@ -83,6 +84,8 @@ struct RedStreamPrivate {
> >   * deallocated when main_dispatcher handles the
> >   SPICE_CHANNEL_EVENT_DISCONNECTED
> >   * event, either from same thread or by call back from main thread. */
> >  SpiceChannelEventInfo* info;
> > +bool use_cork;
> > +bool corked;
> >  
> >  ssize_t (*read)(RedStream *s, void *buf, size_t nbyte);
> >  ssize_t (*write)(RedStream *s, const void *buf, size_t nbyte);
> > @@ -92,6 +95,15 @@ struct RedStreamPrivate {
> >  SpiceCoreInterfaceInternal *core;
> >  };
> >  
> > +/**
> > + * Set TCP_CORK on socket
> > + */
> > +/* NOTE: enabled must be int */
> 
> Maybe verify(sizeof(socket) == sizeof(int))?
> 

added a SPICE_VERIFY inside the function, specifically

SPICE_VERIFY(sizeof(enabled) == sizeof(int));

> > +static inline int socket_set_cork(int socket, int enabled)
> 
> I'd drop the 'inline'
> 
> > +{
> > +return setsockopt(socket, IPPROTO_TCP, TCP_CORK, ,
> > sizeof(enabled));
> 
> I suspect we'll need to add a configure check for this? It seems to be
> called TCP_NOPUSH in OpenBSD? https://man.openbsd.org/tcp
> 

I'll add a

#ifndef TCP_CORK
#define TCP_CORK TCP_NOPUSH
#endif

at the beginning of the file

> > +}
> > +
> >  static ssize_t stream_write_cb(RedStream *s, const void *buf, size_t size)
> >  {
> >  return write(s->socket, buf, size);
> > @@ -205,11 +217,31 @@ bool red_stream_write_all(RedStream *stream, const
> > void *in_buf, size_t n)
> >  
> >  bool red_stream_set_auto_flush(RedStream *s, bool enable)
> >  {
> > -return enable;
> > +if (s->priv->use_cork == !enable) {
> 
> Might be slightly more readable if 'enable' is renamed to 'auto_flush'
> 

Agreed, done

> 
> Acked-by: Christophe Fergeau 
> 

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 2/3] stream: implements flush using TCP_CORK

[Christophe Fergeau]

On Tue, Jan 16, 2018 at 02:18:14PM +, Frediano Ziglio wrote:
> Cork is a system interface implemented by Linux and some *BSD systems to
> tell the system that other data are expected to be written to a socket.
> This allows the system to reduce network fragmentation waiting a network

'waiting for network packets to be complete' I think.

> packet to be complete.
> 
> Using some replay capture and some instrumentation resulted in a
> bandwith reduction of 11% and a packet reduction of 56%.

I would add a link to 
https://lists.freedesktop.org/archives/spice-devel/2017-February/035577.html
and maybe even copy the data you put in there.

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-stream.c | 34 +-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/server/red-stream.c b/server/red-stream.c
> index 4812d8e4..4833077c 100644
> --- a/server/red-stream.c
> +++ b/server/red-stream.c
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -83,6 +84,8 @@ struct RedStreamPrivate {
>   * deallocated when main_dispatcher handles the 
> SPICE_CHANNEL_EVENT_DISCONNECTED
>   * event, either from same thread or by call back from main thread. */
>  SpiceChannelEventInfo* info;
> +bool use_cork;
> +bool corked;
>  
>  ssize_t (*read)(RedStream *s, void *buf, size_t nbyte);
>  ssize_t (*write)(RedStream *s, const void *buf, size_t nbyte);
> @@ -92,6 +95,15 @@ struct RedStreamPrivate {
>  SpiceCoreInterfaceInternal *core;
>  };
>  
> +/**
> + * Set TCP_CORK on socket
> + */
> +/* NOTE: enabled must be int */

Maybe verify(sizeof(socket) == sizeof(int))?

> +static inline int socket_set_cork(int socket, int enabled)

I'd drop the 'inline'

> +{
> +return setsockopt(socket, IPPROTO_TCP, TCP_CORK, , 
> sizeof(enabled));

I suspect we'll need to add a configure check for this? It seems to be
called TCP_NOPUSH in OpenBSD? https://man.openbsd.org/tcp

> +}
> +
>  static ssize_t stream_write_cb(RedStream *s, const void *buf, size_t size)
>  {
>  return write(s->socket, buf, size);
> @@ -205,11 +217,31 @@ bool red_stream_write_all(RedStream *stream, const void 
> *in_buf, size_t n)
>  
>  bool red_stream_set_auto_flush(RedStream *s, bool enable)
>  {
> -return enable;
> +if (s->priv->use_cork == !enable) {

Might be slightly more readable if 'enable' is renamed to 'auto_flush'


Acked-by: Christophe Fergeau 


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel