[ipxe-devel] About recent TCP stack improvements

[Guo-Fu Tseng]

On Thu, 22 Jul 2010 12:27:58 +0100, Michael Brown wrote
> (Redirecting to ipxe-devel)
> 
> On Thursday 22 Jul 2010 11:09:23 Michael Brown wrote:
> > [tcp] Receive and Close flow adjustment
> > 
> > This is difficult to apply following the changes to support out-of-order
> > packets, and I'm not sure how valuable it is.  Since commit 9ff8229 ("[tcp]
> > Update received sequence number before delivering received data"), which
> > fixes a problem that you identified (thank you!), I think that the TCP
> > state is consistent at the time the data is delivered to the upper layer,
> > so any actions it might take are safe.
> 
> Ignore that; I've just worked out that this patch makes perfect sense 
> *if* we also distinguish between passive and active close, and I see 
> how it can be implemented cleanly on top of the recent out-of-order changes.
> 
> tcp_rx_data() should add the I/O buffer to a list rather than 
> delivering it via xfer_deliver_iob().  tcp_rx_fin() should not call 
> tcp_close().  tcp_process_rx_queue() should be adjusted to do 
> something like:
> 
>   struct list_head received = LIST_HEAD_INIT ( received );
>   ...
>   while ( ! list_empty ( &tcp->rx_queue ) ) {
>      ...
>      tcp_rx_data ( tcp, seq, iob_disown ( iobuf ), &received );
>      ...
>   }
> 
>   list_for_each_entry_safe ( iobuf, tmp, &received, list ) {
>     // deliver iobuf via xfer_deliver_iob()
>   }
> 
>   if ( tcp->state & TCP_STATE_RCVD ( TCP_FIN ) )
>     tcp_close ( tcp, 0 );
> 
> I think that would handle everything sensibly, and would mean that we 
> could properly handle passive close since, in the case of a received 
> data+FIN packet that will also cause our higher layer protocol (e.g. 
> http.c) to close the connection, tcp_rx_fin() would see the FIN before 
> http.c called xfer_close().
> 
> Does this seem correct to you?  If so, would you like to put together 
> a patch?
> 
> Michael
Yes! And I think it is be better than what I did in
"[tcp] Receive and Close flow adjustment". to put xfer_deliver_iob()
in tcp_process_rx_queue() seems more reasonable.

I would be honored to put together a patch of the passive close
facility and the above you suggested on top of current iPXE TCP stack.

BTW, do you think it's reasonable to do something like:
"[tcp] Cleanup TCP closing actions" patch which it sumbitted
on gpxe-devel list?[2]

I think it might be useful for following reasons:
  1. We don't have to think of what would tcp_close() do if we call it
somewhere. The behavior of calling tcp_close() would be always the same.
  2. Reduced some duplicate code.
  3. We don't have to separate tcp_close() from tcp_rx_fin(). And have the same
behavior. Seems a little more neat to me.

Above results are accomplished by:
  1. Separate terminate action.
  2. Separate terminate action.
  3. Separate nullify xfer interface.
     (Which is part of "[tcp] Receive and Close flow adjustment"[1])

[1]:
http://git.etherboot.org/?p=people/cooldavid/gpxe.git;a=commitdiff;h=8fc73d18c8528cbcc1b1c3849b51d3ee3682c937
[2]:
http://git.etherboot.org/?p=people/cooldavid/gpxe.git;a=commitdiff;h=660e96200f67c981e7397eb05fbb4e91ed253f50

Guo-Fu Tseng