[Spice-devel] [PATCH spice-server 2/3] red-stream: Implements flush using TCP_CORK
Frediano Ziglio
fziglio at redhat.com
Wed Apr 18 12:35:16 UTC 2018
>
> On 04/17/2018 08:50 PM, Frediano Ziglio wrote:
> >>
> >> 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 at redhat.com>
> >>> ---
> >>> 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:
>
> I know it's not equivalent, but simpler.
>
> Do we really need both use_cork and corked ?
>
> Even if corked==FALSE , still socket_set_cork(s->socket, 0)
> would succeed.
>
> Uri.
>
now I understood, taken in consideration the initial state and
possible changes this is equivalent:
bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)
{
if (s->priv->use_cork == !auto_flush) {
return true;
}
if (socket_set_cork(s->socket, !auto_flush) && !auto_flush) {
return false;
}
s->priv->use_cork = !auto_flush;
return true;
}
The reason is that use_cork is (beside temporary states) always
equal to corked.
Frediano
> >
> >
> > 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
> >>>
> >>
> >>
> >
More information about the Spice-devel
mailing list