On Mon, 2016-02-22 at 13:38 -0500, Frediano Ziglio wrote:
> >
> > On Tue, 2016-02-16 at 15:36 +0000, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > ---
> > > server/red-channel.c | 12 ++++++++----
> > > server/red-worker.c | 24 +++++++++++++-----------
> > > 2 files changed, 21 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/server/red-channel.c b/server/red-channel.c
> > > index d7ff37e..c2a0c78 100644
> > > --- a/server/red-channel.c
> > > +++ b/server/red-channel.c
> > > @@ -1364,10 +1364,14 @@ void red_channel_client_push(RedChannelClient
> > > *rcc)
> > > while ((pipe_item = red_channel_client_pipe_item_get(rcc))) {
> > > red_channel_client_send_item(rcc, pipe_item);
> > > }
> > > - if (red_channel_client_no_item_being_sent(rcc) && ring_is_empty(&rcc
> > > ->pipe)
> > > - && rcc->stream->watch) {
> > > - rcc->channel->core->watch_update_mask(rcc->stream->watch,
> > > - SPICE_WATCH_EVENT_READ);
> > > + if (red_channel_client_no_item_being_sent(rcc) && ring_is_empty(&rcc
> > > ->pipe)) {
> > > + if (rcc->stream->watch) {
> > > + rcc->channel->core->watch_update_mask(rcc->stream->watch,
> > > +
> > > SPICE_WATCH_EVENT_READ);
> > > + }
> > > + reds_stream_flush(rcc->stream);
> > > + } else if (red_channel_client_waiting_for_ack(rcc)) {
> > > + reds_stream_flush(rcc->stream);
> >
> > Can you explain the reason for flushing when we're waiting for an ACK? Is
> > there
> > a reason we couldn't just move the flush completely outside of the if
> > statement
> > and flush in all circumstances in this function?
> >
>
> Mainly it's the optimal way.
> For Ack you need to flush as otherwise you risk to wait an pong for a not sent
> ping if the messages are really small.
> For the other if you want to flush only if there are nothing queued (neither
> data neither other messages).
> Note that you can have lot of messages in the queue but you are waiting for
> ack (pong) before sending other data.
>
> OT: I don't like the idea of ACKs on bytes and as discussed with Pavel in Brno
> I don't see the point of using additional Acking on top of tcp (which already
> implements flow control) but one problem at a time.
>
> > > }
> > > rcc->during_send = FALSE;
> > > red_channel_client_unref(rcc);
> > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > index bf12a22..b368a63 100644
> > > --- a/server/red-worker.c
> > > +++ b/server/red-worker.c
> > > @@ -416,17 +416,19 @@ static int
> > > common_channel_config_socket(RedChannelClient
> > > *rcc)
> > >
> > > // TODO - this should be dynamic, not one time at channel creation
> > > ccc->is_low_bandwidth = main_channel_client_is_low_bandwidth(mcc);
> > > - delay_val = ccc->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));
> > > + if (!reds_stream_set_auto_flush(rcc->stream, false)) {
> > > + delay_val = ccc->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));
> > > + }
> >
> >
> > I don't quite understand why you're setting NODELAY if CORK fails here. Can
> > you
> > provide a bit more justification/explanation here?
> >
>
> It's a fallback. Cork and flush are a better way to achieve no delay sending
> data also avoiding also too much fragmentation (which was the reason for
> Nagle).
> It's actually a bit paranoid as the socket and system (Linux) we are using
> supports TCP_CORK so function won't fail but we are ready for failures or
> porting (I don't know... *BSD, Mac).
I apologize, somehow I completely missed the fact that the old code was already
setting NODELAY. For some reason I thought that your patch added that part. I'll
blame it on Monday.
>
> Note that the flush APIs are independent on the implementation. They can be
> used with SSL and compression too.
> Assume we are sending (not impossible!) 20 messages of 50 bytes each. The
> standard framing for ethernet (wire or wireless) + tcp is about 80 bytes.
> Sending as separate network packets you use 20 * ( 50 + 80 ) = 2600 bytes
> while with cork you just spend 1080 ( 20 * 50 + 80 ).
> Using SSL assuming a cipher of 128 bit we can assume to encrypt N bytes
> we need 3 + ROUNDUP(N + 8, 16) (3 is the TLS framing, 8 is for the padding
> and 16 is the key size in bytes)that is for 50 bytes 67 bytes so we could
> have 20 * ( 67 + 80 ) = 2940 bytes instead of
> 3 + ROUNDUP(20 * 50 + 8, 16) = 1011 bytes .
> Similar padding+framing apply to compression algorithms.
>
> >
> > > }
> > > }
> > > return TRUE;
> >
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel