>
> On 04/13/2018 03:25 PM, 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 for network
> > packets to be complete.
> >
> > Using some replay capture and some instrumentation resulted in a
> > bandwith reduction of 11% and a packet reduction of 56%.
> >
> > 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 <fziglio@xxxxxxxxxx>
> > ---
> > server/red-stream.c | 40 +++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/server/red-stream.c b/server/red-stream.c
> > index 9a9c1a0f..18c4a935 100644
> > --- a/server/red-stream.c
> > +++ b/server/red-stream.c
> > @@ -24,6 +24,7 @@
> > #include <unistd.h>
> > #include <sys/socket.h>
> > #include <fcntl.h>
> > +#include <netinet/tcp.h>
> >
> > #include <glib.h>
> >
> > @@ -37,6 +38,11 @@
> > #include "red-stream.h"
> > #include "reds.h"
> >
> > +// compatibility for *BSD systems
> > +#ifndef TCP_CORK
> > +#define TCP_CORK TCP_NOPUSH
> > +#endif
> > +
> > struct AsyncRead {
> > void *opaque;
> > uint8_t *now;
> > @@ -83,6 +89,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 +100,16 @@ struct RedStreamPrivate {
> > SpiceCoreInterfaceInternal *core;
> > };
> >
> > +/**
> > + * Set TCP_CORK on socket
> > + */
> > +/* NOTE: enabled must be int */
> > +static int socket_set_cork(int socket, int enabled)
> > +{
> > + SPICE_VERIFY(sizeof(enabled) == sizeof(int));
> > + return setsockopt(socket, IPPROTO_TCP, TCP_CORK, &enabled,
> > sizeof(enabled));
> > +}
> > +
> > static ssize_t stream_write_cb(RedStream *s, const void *buf, size_t
> > size)
> > {
> > return write(s->socket, buf, size);
> > @@ -205,11 +223,31 @@ bool red_stream_write_all(RedStream *stream, const
> > void *in_buf, size_t n)
> >
> > bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)
> > {
> > - return auto_flush;
> > + if (s->priv->use_cork == !auto_flush) {
> > + return true;
> > + }
> > +
> > + s->priv->use_cork = !auto_flush;
> > + if (s->priv->use_cork) {
> > + if (socket_set_cork(s->socket, 1)) {
> > + s->priv->use_cork = false;
> > + return false;
> > + } else {
> > + s->priv->corked = true;
> > + }
> > + } else if (s->priv->corked) {
> > + socket_set_cork(s->socket, 0);
> > + s->priv->corked = false;
> > + }
> Hi Frediano,
>
> It would be simpler to use !auto_flash or s->priv->use_cork
> and also only keep one of use_cork and corked.
> Possibly the logic is a bit different and not exactly what you want.
>
> if (socket_set_cork(s->socket, !auto_flash)) {
> return false;
> }
>
> s->priv->use_cork = !auto_flash;
>
>
> > + return true;
> > }
>
> Uri.
>
Not proper... the exact equivalent is this:
bool red_stream_set_auto_flush_new(RedStream *s, bool auto_flush)
{
if (s->priv->use_cork == !auto_flush) {
return true;
}
if (!auto_flush || s->priv->corked) {
if (socket_set_cork(s->socket, !auto_flush) && !auto_flush) {
return false;
}
s->priv->corked = !auto_flush;
}
s->priv->use_cork = !auto_flush;
return true;
}
but is confusing, maybe easier to remove the "} else {" after a return
and move use_cork change after the ifs as suggested to avoid assigning
to false, like:
--- a/server/red-stream.c
+++ b/server/red-stream.c
@@ -227,18 +227,16 @@ bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)
return true;
}
- s->priv->use_cork = !auto_flush;
- if (s->priv->use_cork) {
+ if (!auto_flush) {
if (socket_set_cork(s->socket, 1)) {
- s->priv->use_cork = false;
return false;
- } else {
- s->priv->corked = true;
}
+ s->priv->corked = true;
} else if (s->priv->corked) {
socket_set_cork(s->socket, 0);
s->priv->corked = false;
}
+ s->priv->use_cork = !auto_flush;
return true;
}
A bit longer than the previous but IMO more readable.
> >
> > void red_stream_flush(RedStream *s)
> > {
> > + if (s->priv->corked) {
> > + socket_set_cork(s->socket, 0);
> > + socket_set_cork(s->socket, 1);
> > + }
> > }
> >
> > #if HAVE_SASL
> >
>
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel