[Spice-commits] 3 commits - server/common-graphics-channel.c server/red-channel-client.c server/red-stream.c server/red-stream.h
Frediano Ziglio
fziglio at kemper.freedesktop.org
Tue Apr 17 14:48:20 UTC 2018
server/common-graphics-channel.c | 17 +++++++-------
server/red-channel-client.c | 5 ++++
server/red-stream.c | 47 +++++++++++++++++++++++++++++++++++++++
server/red-stream.h | 20 ++++++++++++++++
4 files changed, 81 insertions(+), 8 deletions(-)
New commits:
commit 357677af9a32299104e2569cad8560923350e8fb
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Fri Jan 29 00:43:52 2016 +0000
common-graphics-channel: Use manual flushing on stream to decrease packet fragmentation
In order to use the new TCP_CORK feature, disable auto flush.
Depending on channel implementation and purpose of the channel enabling
blindly for all channels could cause performance issues, specifically if
flush is not done at the right time.
CommonGraphicsChannel channels were tested to make sure is not that case.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/common-graphics-channel.c b/server/common-graphics-channel.c
index ce6b5e57..083ab3eb 100644
--- a/server/common-graphics-channel.c
+++ b/server/common-graphics-channel.c
@@ -83,14 +83,15 @@ bool common_channel_client_config_socket(RedChannelClient *rcc)
// TODO - this should be dynamic, not one time at channel creation
is_low_bandwidth = main_channel_client_is_low_bandwidth(mcc);
- /* FIXME: Using Nagle's Algorithm can lead to apparent delays, depending
- * on the delayed ack timeout on the other side.
- * Instead of using Nagle's, we need to implement message buffering on
- * the application level.
- * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
- */
- red_stream_set_no_delay(stream, !is_low_bandwidth);
-
+ if (!red_stream_set_auto_flush(stream, false)) {
+ /* FIXME: Using Nagle's Algorithm can lead to apparent delays, depending
+ * on the delayed ack timeout on the other side.
+ * Instead of using Nagle's, we need to implement message buffering on
+ * the application level.
+ * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
+ */
+ red_stream_set_no_delay(stream, !is_low_bandwidth);
+ }
// TODO: move wide/narrow ack setting to red_channel.
red_channel_client_ack_set_client_window(rcc,
is_low_bandwidth ?
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index d46c299c..893a764d 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -1328,6 +1328,11 @@ void red_channel_client_push(RedChannelClient *rcc)
if ((red_channel_client_no_item_being_sent(rcc) && g_queue_is_empty(&rcc->priv->pipe)) ||
red_channel_client_waiting_for_ack(rcc)) {
red_channel_client_watch_update_mask(rcc, SPICE_WATCH_EVENT_READ);
+ /* channel has no pending data to send so now we can flush data in
+ * order to avoid data stall into buffers in case of manual
+ * flushing
+ */
+ red_stream_flush(rcc->priv->stream);
}
rcc->priv->during_send = FALSE;
g_object_unref(rcc);
commit 4b1d33d384e392a9bf0e79d5b033983dd2e0cf99
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Thu Jan 28 09:16:12 2016 +0000
red-stream: Implements flush using TCP_CORK
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>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
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;
+ }
+ return true;
}
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
commit 63d02ab10e2dae78d6bdb567621c244c868ebec3
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Thu Jan 28 08:59:40 2016 +0000
red-stream: Define interface for manual flush
The writing to network was always immediate.
Every write in the stream causes a write to the OS.
This can have some penalty if you don't write large data as network
packets can be more fragmented or you encrypt data in smaller chunks
(when data are encrypted some padding is added then data is split in
multiple of encryption block which is usually the size of encryption
key and this is done for every write).
Define an interface to allow higher levels code to tell low level when
data should be sent to remote or when can wait more data.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red-stream.c b/server/red-stream.c
index bdc8bc1f..9a9c1a0f 100644
--- a/server/red-stream.c
+++ b/server/red-stream.c
@@ -203,6 +203,15 @@ bool red_stream_write_all(RedStream *stream, const void *in_buf, size_t n)
return true;
}
+bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)
+{
+ return auto_flush;
+}
+
+void red_stream_flush(RedStream *s)
+{
+}
+
#if HAVE_SASL
static ssize_t red_stream_sasl_write(RedStream *s, const void *buf, size_t nbyte);
#endif
diff --git a/server/red-stream.h b/server/red-stream.h
index 4d5075ed..15af0a59 100644
--- a/server/red-stream.h
+++ b/server/red-stream.h
@@ -69,6 +69,26 @@ bool red_stream_set_no_delay(RedStream *stream, bool no_delay);
int red_stream_get_no_delay(RedStream *stream);
int red_stream_send_msgfd(RedStream *stream, int fd);
+/**
+ * Set auto flush flag.
+ * If set, stream will send data to the underlying socket as
+ * soon as data are written. This is the default.
+ * If not set, you should call red_stream_flush to force
+ * data to be sent. Failing to call red_stream_flush on a
+ * manual flush stream could lead to latency.
+ * Disabling auto flush can fail while enabling cannot.
+ *
+ * Returns true on success or false on failure.
+ */
+bool red_stream_set_auto_flush(RedStream *stream, bool auto_flush);
+
+/**
+ * Flush data to the underlying socket.
+ * Calling this function on a stream with auto flush set has
+ * no result.
+ */
+void red_stream_flush(RedStream *stream);
+
typedef enum {
RED_SASL_ERROR_OK,
RED_SASL_ERROR_GENERIC,
More information about the Spice-commits
mailing list