Re: [PATCH 1/4] tcp: Fix ACK sequence on FIN to tap

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 7 Oct 2025 10:31:11 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Oct 07, 2025 at 12:32:19AM +0200, Stefano Brivio wrote:
> > On Fri, 3 Oct 2025 13:19:17 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Thu, Oct 02, 2025 at 01:58:41PM +0200, Stefano Brivio wrote:  
> > > > On Thu, 2 Oct 2025 12:41:08 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Thu, Oct 02, 2025 at 02:06:43AM +0200, Stefano Brivio wrote:    
> > > > > > If we reach end-of-file on a socket (or get EPOLLRDHUP / EPOLLHUP) and
> > > > > > send a FIN segment to the guest / container acknowledging a sequence
> > > > > > number that's behind what we received so far, we won't have any
> > > > > > further trigger to send an updated ACK segment, as we are now
> > > > > > switching the epoll socket monitoring to edge-triggered mode.
> > > > > > 
> > > > > > To avoid this situation, in tcp_update_seqack_wnd(), we set the next
> > > > > > acknowledgement sequence to the current observed sequence, regardless
> > > > > > of what was acknowledged socket-side.      
> > > > > 
> > > > > To double check my understanding: things should work if we always
> > > > > acknowledged everything we've received.  Acknowledging only what the
> > > > > peer has acked is a refinement to give the guest a view that's closer
> > > > > to what it would be end-to-end with the peer (which might improve the
> > > > > operation of flow control).    
> > > > 
> > > > Right.
> > > >     
> > > > > We can't use that refined mechanism when the socket is closing
> > > > > (amongst other cases), because while we can get the peer acked bytes
> > > > > from TCP_INFO, we can't get events when that changes, so we have no
> > > > > mechanism to provide updates to the guest at the right time.  So we
> > > > > fall back to the simpler method.
> > > > > 
> > > > > Is that correct?    
> > > > 
> > > > Also correct, yes. If you have a better idea to summarise this in the
> > > > comment in tcp_buf_data_from_sock() let me know.    
> > > 
> > > Hm, I might.  Or actually a way to reorganise the code that I think
> > > will be a bit clearer and probably allow a clearer comment too.  
> > 
> > I would keep reworks for a later moment. Right now, it's already taking
> > me long enough to find a moment to investigate these issues, write these
> > fixes, and test them.  
> 
> I mean... the change I'm proposing reduces lines of code (excepting
> the big new comment), makes it easier to reason about and is localised
> to the immediately surrounding code.  But whatever, I don't
> particularly care about the order we do things.

Sure, I don't think that other patch is particularly complicated or
might ever become problematic at all, but, from your earlier comment
("reorganise the code", as I mentioned the comment in
tcp_buf_data_from_sock()), I thought you wanted to rework this
particular code in tcp_buf_data_from_sock() at the same time.

I guess it's not the case judging from your most recent reply, though.

-- 
Stefano