[tcp][RFC] Deliver data only after updating TCP state

Michael suggested 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 ); =============================================================== But after some thought I think making the change like this commit can have same behavior, but simplified by not using extra queue. Which can also save some code size. In this patch: 1. We call xfer_deliver_iob() after fully updated/handled received TCP state. In which to have better timing that upper-layer protocol might call tcp_xfer_close() to initiate a shutdown, or sending extra data. 2. We move tcp_close() out from tcp_rx_fin() because tcp_close() nullified the xfer interface. It'll cause error to call xfer_deliver_iob() after tcp_rx_fin(). (So that I proposed an idea to clean up tcp_close(), I'll post it in later commit.)
ipxe · Jul 30, 2010 · e59ba46 · e59ba46
1 parent 02e6092
commit e59ba46
Showing 1 changed file with 19 additions and 15 deletions.
diff --git a/src/net/tcp.c b/src/net/tcp.c
@@ -861,33 +861,25 @@ static int tcp_rx_ack ( struct tcp_connection *tcp, uint32_t ack,
  *
  * This function takes ownership of the I/O buffer.
  */
-static int tcp_rx_data ( struct tcp_connection *tcp, uint32_t seq,
+static struct io_buffer* tcp_rx_data ( struct tcp_connection *tcp, uint32_t seq,
 			 struct io_buffer *iobuf ) {
 	uint32_t already_rcvd;
 	uint32_t len;
-	int rc;
 
 	/* Ignore duplicate or out-of-order data */
 	already_rcvd = ( tcp->rcv_ack - seq );
 	len = iob_len ( iobuf );
 	if ( already_rcvd >= len ) {
 		free_iob ( iobuf );
-		return 0;
+		return NULL;
 	}
 	iob_pull ( iobuf, already_rcvd );
 	len -= already_rcvd;
 
 	/* Acknowledge new data */
 	tcp_rx_seq ( tcp, len );
 
-	/* Deliver data to application */
-	if ( ( rc = xfer_deliver_iob ( &tcp->xfer, iobuf ) ) != 0 ) {
-		DBGC ( tcp, "TCP %p could not deliver %08x..%08x: %s\n",
-		       tcp, seq, ( seq + len ), strerror ( rc ) );
-		return rc;
-	}
-
-	return 0;
+	return iobuf;
 }
 
 /**
@@ -909,9 +901,6 @@ static int tcp_rx_fin ( struct tcp_connection *tcp, uint32_t seq ) {
 	/* Mark FIN as received */
 	tcp->tcp_state |= TCP_STATE_RCVD ( TCP_FIN );
 
-	/* Close connection */
-	tcp_close ( tcp, 0 );
-
 	return 0;
 }
 
@@ -1008,6 +997,7 @@ static void tcp_process_rx_queue ( struct tcp_connection *tcp ) {
 	uint32_t seq;
 	unsigned int flags;
 	size_t len;
+	int rc;
 
 	/* Process all applicable received buffers.  Note that we
 	 * cannot use list_for_each_entry() to iterate over the RX
@@ -1031,14 +1021,28 @@ static void tcp_process_rx_queue ( struct tcp_connection *tcp ) {
 		len = iob_len ( iobuf );
 
 		/* Handle new data, if any */
-		tcp_rx_data ( tcp, seq, iob_disown ( iobuf ) );
+		iobuf = tcp_rx_data ( tcp, seq, iobuf );
 		seq += len;
 
 		/* Handle FIN, if present */
 		if ( flags & TCP_FIN ) {
 			tcp_rx_fin ( tcp, seq );
 			seq++;
 		}
+
+		/* Deliver data to application, if any */
+		if ( iobuf &&
+		     ( rc = xfer_deliver_iob ( &tcp->xfer,
+					       iob_disown ( iobuf ) ) ) != 0 ) {
+			DBGC ( tcp, "TCP %p could not deliver %08x..%08x: %s\n",
+			       tcp, ( seq - len - ( flags & TCP_FIN ) ? 1 : 0 ),
+			       seq, strerror ( rc ) );
+		}
+	}
+
+	if ( tcp->tcp_state & TCP_STATE_RCVD ( TCP_FIN ) ) {
+		/* Close connection */
+		tcp_close ( tcp, 0 );
 	}
 }