[Spice-devel] [PATCH spice-server 3/4] Do not set TCP_NODELAY flag twice
Christophe Fergeau
cfergeau at redhat.com
Tue Feb 14 15:05:12 UTC 2017
On Mon, Feb 13, 2017 at 11:03:18AM +0000, Frediano Ziglio wrote:
> TCP_NODELAY flag is set by default for all connection inside
> reds.c so there's no need to set again for the single
> client channel.
>
> Note that there are still some call to setsockopt to set this
> option but in this case the flag can reset the flag.
I happen to have written the attached patch yesterday too which is
related, can you pick it up in that patch series?
Christophe
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> server/inputs-channel.c | 11 -----------
> server/spicevmc.c | 16 ----------------
> 2 files changed, 27 deletions(-)
>
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index f105b4d..897e8e7 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -490,17 +490,6 @@ static void inputs_pipe_add_init(RedChannelClient *rcc)
>
> static int inputs_channel_config_socket(RedChannelClient *rcc)
> {
> - int delay_val = 1;
> - RedsStream *stream = red_channel_client_get_stream(rcc);
> -
> - if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
> - &delay_val, sizeof(delay_val)) == -1) {
> - if (errno != ENOTSUP && errno != ENOPROTOOPT) {
> - spice_printerr("setsockopt failed, %s", strerror(errno));
> - return FALSE;
> - }
> - }
> -
> return TRUE;
> }
>
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 9bcbada..1003a2f 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -427,22 +427,6 @@ static void spicevmc_char_dev_remove_client(RedCharDevice *self,
>
> static int spicevmc_red_channel_client_config_socket(RedChannelClient *rcc)
> {
> - int delay_val = 1;
> - RedsStream *stream = red_channel_client_get_stream(rcc);
> - RedChannel *channel = red_channel_client_get_channel(rcc);
> - uint32_t type;
> -
> - g_object_get(channel, "channel-type", &type, NULL);
> - if (type == SPICE_CHANNEL_USBREDIR) {
> - if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
> - &delay_val, sizeof(delay_val)) != 0) {
> - if (errno != ENOTSUP && errno != ENOPROTOOPT) {
> - spice_printerr("setsockopt failed, %s", strerror(errno));
> - return FALSE;
> - }
> - }
> - }
> -
> return TRUE;
> }
>
> --
> 2.9.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
From 43d68824e6622dc0c93769907da67c2d32c5c365 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau at redhat.com>
Date: Mon, 13 Feb 2017 16:10:04 +0100
Subject: [spice-server] reds-stream: Introduce reds_stream_set_no_delay()
helper
The code to enable/disable on a TCP socket is duplicated in multiple
places in the code base, this commit replaces this duplicated code with
a helper in RedsStream.
---
server/common-graphics-channel.c | 13 ++-----------
server/inputs-channel.c | 13 ++-----------
server/red-channel-client.c | 17 ++---------------
server/reds-stream.c | 16 ++++++++++++++++
server/reds-stream.h | 1 +
server/reds.c | 8 +-------
server/sound.c | 8 +-------
server/spicevmc.c | 9 ++-------
8 files changed, 27 insertions(+), 58 deletions(-)
diff --git a/server/common-graphics-channel.c b/server/common-graphics-channel.c
index 86541ba..6b94820 100644
--- a/server/common-graphics-channel.c
+++ b/server/common-graphics-channel.c
@@ -19,9 +19,6 @@
#endif
#include <fcntl.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <netinet/tcp.h>
#include "common-graphics-channel.h"
#include "dcc.h"
@@ -118,7 +115,6 @@ bool common_channel_config_socket(RedChannelClient *rcc)
MainChannelClient *mcc = red_client_get_main(client);
RedsStream *stream = red_channel_client_get_stream(rcc);
int flags;
- int delay_val;
gboolean is_low_bandwidth;
if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
@@ -133,19 +129,14 @@ bool common_channel_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);
- delay_val = is_low_bandwidth ? 0 : 1;
/* 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/
*/
- if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, &delay_val,
- sizeof(delay_val)) == -1) {
- if (errno != ENOTSUP) {
- spice_warning("setsockopt failed, %s", strerror(errno));
- }
- }
+ reds_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/inputs-channel.c b/server/inputs-channel.c
index 5f12c10..1867982 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -19,11 +19,7 @@
#include <config.h>
#endif
-#include <netinet/in.h> // IPPROTO_TCP
-#include <netinet/tcp.h> // TCP_NODELAY
-#include <fcntl.h>
#include <stddef.h> // NULL
-#include <errno.h>
#include <spice/macros.h>
#include <spice/vd_agent.h>
#include <spice/protocol.h>
@@ -489,15 +485,10 @@ static void inputs_pipe_add_init(RedChannelClient *rcc)
static bool inputs_channel_config_socket(RedChannelClient *rcc)
{
- int delay_val = 1;
RedsStream *stream = red_channel_client_get_stream(rcc);
- if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
- &delay_val, sizeof(delay_val)) == -1) {
- if (errno != ENOTSUP && errno != ENOPROTOOPT) {
- spice_printerr("setsockopt failed, %s", strerror(errno));
- return FALSE;
- }
+ if (!reds_stream_set_no_delay(stream, 1)) {
+ return FALSE;
}
return TRUE;
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 524ab95..f64d325 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -610,13 +610,7 @@ static void red_channel_client_send_ping(RedChannelClient *rcc)
} else {
rcc->priv->latency_monitor.tcp_nodelay = delay_val;
if (!delay_val) {
- delay_val = 1;
- if (setsockopt(rcc->priv->stream->socket, IPPROTO_TCP, TCP_NODELAY, &delay_val,
- sizeof(delay_val)) == -1) {
- if (errno != ENOTSUP) {
- spice_warning("setsockopt failed, %s", strerror(errno));
- }
- }
+ reds_stream_set_no_delay(rcc->priv->stream, TRUE);
}
}
}
@@ -1459,14 +1453,7 @@ static void red_channel_client_handle_pong(RedChannelClient *rcc, SpiceMsgPing *
/* set TCP_NODELAY=0, in case we reverted it for the test*/
if (!rcc->priv->latency_monitor.tcp_nodelay) {
- int delay_val = 0;
-
- if (setsockopt(rcc->priv->stream->socket, IPPROTO_TCP, TCP_NODELAY, &delay_val,
- sizeof(delay_val)) == -1) {
- if (errno != ENOTSUP) {
- spice_warning("setsockopt failed, %s", strerror(errno));
- }
- }
+ reds_stream_set_no_delay(rcc->priv->stream, FALSE);
}
/*
diff --git a/server/reds-stream.c b/server/reds-stream.c
index fc7fcdb..d3cca1b 100644
--- a/server/reds-stream.c
+++ b/server/reds-stream.c
@@ -21,6 +21,7 @@
#include <errno.h>
#include <netdb.h>
+#include <netinet/tcp.h>
#include <unistd.h>
#include <sys/socket.h>
#include <fcntl.h>
@@ -256,6 +257,21 @@ bool reds_stream_is_plain_unix(const RedsStream *s)
}
+bool reds_stream_set_no_delay(RedsStream *stream, bool no_delay)
+{
+ int delay_val = no_delay;
+
+ if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
+ &delay_val, sizeof(delay_val)) != 0) {
+ if (errno != ENOTSUP && errno != ENOPROTOOPT) {
+ spice_printerr("setsockopt failed, %s", strerror(errno));
+ return FALSE;
+ }
+ }
+
+ return TRUE;
+}
+
int reds_stream_send_msgfd(RedsStream *stream, int fd)
{
struct msghdr msgh = { 0, };
diff --git a/server/reds-stream.h b/server/reds-stream.h
index 3ca439d..0cbd0c7 100644
--- a/server/reds-stream.h
+++ b/server/reds-stream.h
@@ -73,6 +73,7 @@ int reds_stream_enable_ssl(RedsStream *stream, SSL_CTX *ctx);
void reds_stream_set_info_flag(RedsStream *stream, unsigned int flag);
int reds_stream_get_family(const RedsStream *stream);
bool reds_stream_is_plain_unix(const RedsStream *stream);
+bool reds_stream_set_no_delay(RedsStream *stream, bool no_delay);
int reds_stream_send_msgfd(RedsStream *stream, int fd);
typedef enum {
diff --git a/server/reds.c b/server/reds.c
index 0baeeea..87ac0cf 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2374,7 +2374,6 @@ static bool reds_init_keepalive(int socket)
static RedLinkInfo *reds_init_client_connection(RedsState *reds, int socket)
{
RedLinkInfo *link;
- int delay_val = 1;
int flags;
if ((flags = fcntl(socket, F_GETFL)) == -1) {
@@ -2387,17 +2386,12 @@ static RedLinkInfo *reds_init_client_connection(RedsState *reds, int socket)
goto error;
}
- if (setsockopt(socket, IPPROTO_TCP, TCP_NODELAY, &delay_val, sizeof(delay_val)) == -1) {
- if (errno != ENOTSUP) {
- spice_warning("setsockopt failed, %s", strerror(errno));
- }
- }
-
reds_init_keepalive(socket);
link = spice_new0(RedLinkInfo, 1);
link->reds = reds;
link->stream = reds_stream_new(reds, socket);
+ reds_stream_set_no_delay(link->stream, TRUE);
/* gather info + send event */
diff --git a/server/sound.c b/server/sound.c
index 0e4ea92..f6b8b5e 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -739,7 +739,6 @@ static void record_channel_send_item(RedChannelClient *rcc, G_GNUC_UNUSED RedPip
static bool snd_channel_config_socket(RedChannelClient *rcc)
{
- int delay_val;
int flags;
#ifdef SO_PRIORITY
int priority;
@@ -771,12 +770,7 @@ static bool snd_channel_config_socket(RedChannelClient *rcc)
}
}
- delay_val = main_channel_client_is_low_bandwidth(mcc) ? 0 : 1;
- if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, &delay_val, sizeof(delay_val)) == -1) {
- if (errno != ENOTSUP) {
- spice_printerr("setsockopt failed, %s", strerror(errno));
- }
- }
+ reds_stream_set_no_delay(stream, main_channel_client_is_low_bandwidth(mcc));
if (fcntl(stream->socket, F_SETFL, flags | O_NONBLOCK) == -1) {
spice_printerr("accept failed, %s", strerror(errno));
diff --git a/server/spicevmc.c b/server/spicevmc.c
index 83a2b10..c1aa039 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -423,19 +423,14 @@ static void spicevmc_char_dev_remove_client(RedCharDevice *self,
static bool spicevmc_red_channel_client_config_socket(RedChannelClient *rcc)
{
- int delay_val = 1;
RedsStream *stream = red_channel_client_get_stream(rcc);
RedChannel *channel = red_channel_client_get_channel(rcc);
uint32_t type;
g_object_get(channel, "channel-type", &type, NULL);
if (type == SPICE_CHANNEL_USBREDIR) {
- if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
- &delay_val, sizeof(delay_val)) != 0) {
- if (errno != ENOTSUP && errno != ENOPROTOOPT) {
- spice_printerr("setsockopt failed, %s", strerror(errno));
- return FALSE;
- }
+ if (!reds_stream_set_no_delay(stream, TRUE)) {
+ return FALSE;
}
}
--
2.9.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170214/7833bd2e/attachment.sig>
More information about the Spice-devel
mailing list