If the receive window presented to the guest closes, slirp should send a
window update once the window reopens sufficiently, rather than forcing
the guest to send a window probe, which can take several seconds.

Signed-off-by: James Clarke <jrtc27@jrtc27.com>
---

Hi,
I encountered this whilst running a (k)FreeBSD build server for Debian.
The host's upload link is over ADSL and thus rather slow, so slirp's
outgoing buffer soon fills up, causing it to close its receive window.
However, without this patch, slirp does not send a window update back to
the guest once it starts draining its outgoing buffer and thus opening
its receive window, causing the guest to remain blocked until its
persist timer next fires and it sends a zero window probe. In the case
of a Linux guest, this starts at ~200ms and grows exponentially, though
most of the time the receive window has already opened by then and thus
the unnecessary delay doesn't have too much of an effect, but the
FreeBSD network stack defaults to a 5s minimum for the persist timer and
thus spends the vast majority of its time not transmitting data, with
the upload speed achieved around 10 KiB/s for this particular guest and
link.

VirtualBox, which uses slirp for its NAT networking mode, also
encountered this and fixed it back in 2014 with [1], but interestingly
decided to set its own delayed ACK flag and wait for its own timer to
fire, rather than calling tcp_output directly. I don't know what their
motivation for that decision was, but to me that seems to add an
unnecessary delay of up to a few hundred ms (though that is of course
still much better than 5s).

I have been testing this patch for the past few days with the build
server uploading its huge backlog of packages one at a time. It is now
reaching 1.3 Mbit/s and networking has remained seemingly stable.

Regards,
James

[1] https://github.com/mirror/vbox/commit/87c7aa2e8d26b989da6e85d532695f1e3b050aaa

 slirp/slirp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 1cb6b07004..238fb04115 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -676,13 +676,13 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
                         /* continue; */
                     } else {
                         ret = sowrite(so);
+                        if (ret > 0) {
+                            /* Call tcp_output in case we need to send a window
+                             * update to the guest, otherwise it will be stuck
+                             * until it sends a window probe. */
+                            tcp_output(sototcpcb(so));
+                        }
                     }
-                    /*
-                     * XXXXX If we wrote something (a lot), there
-                     * could be a need for a window update.
-                     * In the worst case, the remote will send
-                     * a window probe to get things going again
-                     */
                 }

                 /*
--
2.17.0

James Clarke, le mar. 17 avril 2018 15:10:58 +0100, a ecrit:
> If the receive window presented to the guest closes, slirp should send a
> window update once the window reopens sufficiently, rather than forcing
> the guest to send a window probe, which can take several seconds.
> 
> Signed-off-by: James Clarke <jrtc27@jrtc27.com>

Makes sense indeed, I have applied it to my tree.

Thanks!
Samuel

> ---
> 
> Hi,
> I encountered this whilst running a (k)FreeBSD build server for Debian.
> The host's upload link is over ADSL and thus rather slow, so slirp's
> outgoing buffer soon fills up, causing it to close its receive window.
> However, without this patch, slirp does not send a window update back to
> the guest once it starts draining its outgoing buffer and thus opening
> its receive window, causing the guest to remain blocked until its
> persist timer next fires and it sends a zero window probe. In the case
> of a Linux guest, this starts at ~200ms and grows exponentially, though
> most of the time the receive window has already opened by then and thus
> the unnecessary delay doesn't have too much of an effect, but the
> FreeBSD network stack defaults to a 5s minimum for the persist timer and
> thus spends the vast majority of its time not transmitting data, with
> the upload speed achieved around 10 KiB/s for this particular guest and
> link.
> 
> VirtualBox, which uses slirp for its NAT networking mode, also
> encountered this and fixed it back in 2014 with [1], but interestingly
> decided to set its own delayed ACK flag and wait for its own timer to
> fire, rather than calling tcp_output directly. I don't know what their
> motivation for that decision was, but to me that seems to add an
> unnecessary delay of up to a few hundred ms (though that is of course
> still much better than 5s).
> 
> I have been testing this patch for the past few days with the build
> server uploading its huge backlog of packages one at a time. It is now
> reaching 1.3 Mbit/s and networking has remained seemingly stable.
> 
> Regards,
> James
> 
> [1] https://github.com/mirror/vbox/commit/87c7aa2e8d26b989da6e85d532695f1e3b050aaa
> 
>  slirp/slirp.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 1cb6b07004..238fb04115 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -676,13 +676,13 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
>                          /* continue; */
>                      } else {
>                          ret = sowrite(so);
> +                        if (ret > 0) {
> +                            /* Call tcp_output in case we need to send a window
> +                             * update to the guest, otherwise it will be stuck
> +                             * until it sends a window probe. */
> +                            tcp_output(sototcpcb(so));
> +                        }
>                      }
> -                    /*
> -                     * XXXXX If we wrote something (a lot), there
> -                     * could be a need for a window update.
> -                     * In the worst case, the remote will send
> -                     * a window probe to get things going again
> -                     */
>                  }
> 
>                  /*
> --
> 2.17.0
> 

-- 
Samuel
"...Unix, MS-DOS, and Windows NT (also known as the Good, the Bad, and
the Ugly)."
(By Matt Welsh)