[Spice-devel] [PATCH spice-server] reds: Send link replies with less chunks
Frediano Ziglio
fziglio at redhat.com
Tue Mar 7 15:05:13 UTC 2017
>
> On Tue, Mar 07, 2017 at 12:35:35PM +0000, Frediano Ziglio wrote:
> > Send header and reply together.
> > This potentially save up to 160 bytes on the network.
> > This as sending multiple chunks you can have different framing,
> > specifically:
> > - if you use TLS every chunk get encrypted separately
> > (reds-stream, currently usually 29 bytes for every chunks);
> > - tcp settings and no delay on socket. More likely with fast
> > connections or better network cards. The tcp framing is
> > usually about 80 bytes;
> > - additional tcp acknowledge (usually 64 bytes).
> > So 80 + 29 + 64 = 173 bytes.
>
> Thanks for the additional details! You made a comment on IRC about the
> size of the 2 messages, which is about 50 bytes, I'd also mention it in
> the commit log... (still to answer the same question, "Why are we doing
> this, and why do we want to do it?")
The answer to "Why are we doing this, and why do we want to do it?" it's
"This potentially save up to 160 bytes on the network.".
> (I could probably argue that this kind of buffering might be best done at a
> lower level layer)
>
The answer you probably want is to "Why we do in this way instead of doing
at lower level?"
This would require a way to say "now flush data" if you don't want
to reimplement some sort of "smart" Nagle algorithm, which we are
disabling to avoid latencies. I'm sending the patches to add lower
level APIs for flushing since a year, repeatedly with explanations and
statistics so the answer is apparently "Because the team seems to not want
a lower level implementation"
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
>
Frediano
>
> >
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> > server/reds.c | 58
> > +++++++++++++++++++++++++++++++---------------------------
> > 1 file changed, 31 insertions(+), 27 deletions(-)
> >
> > Changes since v2:
> > - explain the comment.
> >
> > Changes since v1:
> > - added some comment;
> > - check structure alignment.
> >
> >
> > diff --git a/server/reds.c b/server/reds.c
> > index fc720a3..41f24bb 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -1459,8 +1459,10 @@ static bool red_link_info_test_capability(const
> > RedLinkInfo *link, uint32_t cap)
> >
> > static int reds_send_link_ack(RedsState *reds, RedLinkInfo *link)
> > {
> > - SpiceLinkHeader header;
> > - SpiceLinkReply ack;
> > + struct {
> > + SpiceLinkHeader header;
> > + SpiceLinkReply ack;
> > + } msg;
> > RedChannel *channel;
> > const RedChannelCapabilities *channel_caps;
> > BUF_MEM *bmBuf;
> > @@ -1468,12 +1470,14 @@ static int reds_send_link_ack(RedsState *reds,
> > RedLinkInfo *link)
> > int ret = FALSE;
> > size_t hdr_size;
> >
> > - header.magic = SPICE_MAGIC;
> > - hdr_size = sizeof(ack);
> > - header.major_version = GUINT32_TO_LE(SPICE_VERSION_MAJOR);
> > - header.minor_version = GUINT32_TO_LE(SPICE_VERSION_MINOR);
> > + SPICE_VERIFY(sizeof(msg) == sizeof(SpiceLinkHeader) +
> > sizeof(SpiceLinkReply));
> >
> > - ack.error = GUINT32_TO_LE(SPICE_LINK_ERR_OK);
> > + msg.header.magic = SPICE_MAGIC;
> > + hdr_size = sizeof(msg.ack);
> > + msg.header.major_version = GUINT32_TO_LE(SPICE_VERSION_MAJOR);
> > + msg.header.minor_version = GUINT32_TO_LE(SPICE_VERSION_MINOR);
> > +
> > + msg.ack.error = GUINT32_TO_LE(SPICE_LINK_ERR_OK);
> >
> > channel = reds_find_channel(reds, link->link_mess->channel_type,
> > link->link_mess->channel_id);
> > @@ -1489,12 +1493,12 @@ static int reds_send_link_ack(RedsState *reds,
> > RedLinkInfo *link)
> > reds_channel_init_auth_caps(link, channel); /* make sure common caps
> > are set */
> >
> > channel_caps = red_channel_get_local_capabilities(channel);
> > - ack.num_common_caps = GUINT32_TO_LE(channel_caps->num_common_caps);
> > - ack.num_channel_caps = GUINT32_TO_LE(channel_caps->num_caps);
> > + msg.ack.num_common_caps =
> > GUINT32_TO_LE(channel_caps->num_common_caps);
> > + msg.ack.num_channel_caps = GUINT32_TO_LE(channel_caps->num_caps);
> > hdr_size += channel_caps->num_common_caps * sizeof(uint32_t);
> > hdr_size += channel_caps->num_caps * sizeof(uint32_t);
> > - header.size = GUINT32_TO_LE(hdr_size);
> > - ack.caps_offset = GUINT32_TO_LE(sizeof(SpiceLinkReply));
> > + msg.header.size = GUINT32_TO_LE(hdr_size);
> > + msg.ack.caps_offset = GUINT32_TO_LE(sizeof(SpiceLinkReply));
> > if (!reds->config->sasl_enabled
> > || !red_link_info_test_capability(link,
> > || SPICE_COMMON_CAP_AUTH_SASL)) {
> > if (!(link->tiTicketing.rsa = RSA_new())) {
> > @@ -1520,7 +1524,7 @@ static int reds_send_link_ack(RedsState *reds,
> > RedLinkInfo *link)
> >
> > i2d_RSA_PUBKEY_bio(bio, link->tiTicketing.rsa);
> > BIO_get_mem_ptr(bio, &bmBuf);
> > - memcpy(ack.pub_key, bmBuf->data, sizeof(ack.pub_key));
> > + memcpy(msg.ack.pub_key, bmBuf->data, sizeof(msg.ack.pub_key));
> > } else {
> > /* if the client sets the AUTH_SASL cap, it indicates that it
> > * supports SASL, and will use it if the server supports SASL as
> > @@ -1532,12 +1536,10 @@ static int reds_send_link_ack(RedsState *reds,
> > RedLinkInfo *link)
> > * will fail.
> > */
> > spice_warning("not initialising RSA key");
> > - memset(ack.pub_key, '\0', sizeof(ack.pub_key));
> > + memset(msg.ack.pub_key, '\0', sizeof(msg.ack.pub_key));
> > }
> >
> > - if (!reds_stream_write_all(link->stream, &header, sizeof(header)))
> > - goto end;
> > - if (!reds_stream_write_all(link->stream, &ack, sizeof(ack)))
> > + if (!reds_stream_write_all(link->stream, &msg, sizeof(msg)))
> > goto end;
> > for (unsigned int i = 0; i < channel_caps->num_common_caps; i++) {
> > guint32 cap = GUINT32_TO_LE(channel_caps->common_caps[i]);
> > @@ -1560,17 +1562,19 @@ end:
> >
> > static bool reds_send_link_error(RedLinkInfo *link, uint32_t error)
> > {
> > - SpiceLinkHeader header;
> > - SpiceLinkReply reply;
> > -
> > - header.magic = SPICE_MAGIC;
> > - header.size = GUINT32_TO_LE(sizeof(reply));
> > - header.major_version = GUINT32_TO_LE(SPICE_VERSION_MAJOR);
> > - header.minor_version = GUINT32_TO_LE(SPICE_VERSION_MINOR);
> > - memset(&reply, 0, sizeof(reply));
> > - reply.error = GUINT32_TO_LE(error);
> > - return reds_stream_write_all(link->stream, &header, sizeof(header)) &&
> > reds_stream_write_all(link->stream, &reply,
> > -
> > sizeof(reply));
> > + struct {
> > + SpiceLinkHeader header;
> > + SpiceLinkReply reply;
> > + } msg;
> > + SPICE_VERIFY(sizeof(msg) == sizeof(SpiceLinkHeader) +
> > sizeof(SpiceLinkReply));
> > +
> > + msg.header.magic = SPICE_MAGIC;
> > + msg.header.size = GUINT32_TO_LE(sizeof(msg.reply));
> > + msg.header.major_version = GUINT32_TO_LE(SPICE_VERSION_MAJOR);
> > + msg.header.minor_version = GUINT32_TO_LE(SPICE_VERSION_MINOR);
> > + memset(&msg.reply, 0, sizeof(msg.reply));
> > + msg.reply.error = GUINT32_TO_LE(error);
> > + return reds_stream_write_all(link->stream, &msg, sizeof(msg));
> > }
> >
> > static void reds_info_new_channel(RedLinkInfo *link, int connection_id)
More information about the Spice-devel
mailing list