Re: [PATCH v3 3/7] tcp: Rewind sequence when guest shrinks window to zero

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 1 Sep 2025 14:28:33 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Aug 29, 2025 at 10:11:28PM +0200, Stefano Brivio wrote:
> > A window shrunk to zero means by definition that anything else that
> > might be in flight is now out of window. Restart from the currently
> > acknowledged sequence.
> > 
> > We need to do that both in tcp_tap_window_update(), where we already
> > check for zero-window updates, as well as in tcp_data_from_tap(),
> > because we might get one of those updates in a batch of packets that
> > also contains a non-zero window update.
> > 
> > Suggested-by: Jon Maloy <jmaloy@redhat.com>
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> > ---
> >  tcp.c | 34 +++++++++++++++++++++++++---------
> >  1 file changed, 25 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tcp.c b/tcp.c
> > index 1402ca2..11c9c84 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -1257,19 +1257,25 @@ static void tcp_get_tap_ws(struct tcp_tap_conn *conn,
> >  
> >  /**
> >   * tcp_tap_window_update() - Process an updated window from tap side
> > + * @c:		Execution context
> >   * @conn:	Connection pointer
> >   * @wnd:	Window value, host order, unscaled
> >   */
> > -static void tcp_tap_window_update(struct tcp_tap_conn *conn, unsigned wnd)
> > +static void tcp_tap_window_update(const struct ctx *c,
> > +				  struct tcp_tap_conn *conn, unsigned wnd)
> >  {
> >  	wnd = MIN(MAX_WINDOW, wnd << conn->ws_from_tap);
> >  
> >  	/* Work-around for bug introduced in peer kernel code, commit
> > -	 * e2142825c120 ("net: tcp: send zero-window ACK when no memory").
> > -	 * We don't update if window shrank to zero.
> > +	 * e2142825c120 ("net: tcp: send zero-window ACK when no memory"): don't
> > +	 * update the window if it shrank to zero, so that we'll eventually
> > +	 * retry to send data, but rewind the sequence as that obviously implies
> > +	 * that no data beyond the updated window will ever be acknowledged.  
> 
> Nit: Arguably "no data...will ever" is not quite right.  It presumbly
> won't be acknowledged until we resend it at least once, but we
> certainly hope it will be acknowledged after that point.

Hmm, yes, it's "ever" in a rather relative sense (we'll time out,
eventually, but that takes a while), which makes it not "ever", strictly
speaking.

Let me just drop "ever", unless you have better ideas on how to improve
the comment.

-- 
Stefano