Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 23 Jul 2009 15:07:04 GMT
From:      Andre Oppermann <andre@FreeBSD.org>
To:        Perforce Change Reviews <perforce@freebsd.org>
Subject:   PERFORCE change 166452 for review
Message-ID:  <200907231507.n6NF74vk035511@repoman.freebsd.org>

next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=166452

Change 166452 by andre@andre_t61 on 2009/07/23 15:06:21

	Simplify m_trimhead().
	In tcp_reass_verity() differentiate rcv_nxt check.
	Ignore rcv_reass_size, it's not tracked at the moment.
	Simplify and fix SACK tracking.
	Swap some KASSERT arguments for clarity.
	Simplify and fix the block pre-merge logic.
	Correctly set reassembly queue timeout.
	Separate block trimming into its own function tcp_reass_trim.
	Fix tcp_reass_merge to deal with all situations and edge-cases.

Affected files ...

.. //depot/projects/tcp_reass/netinet/tcp_reass.c#39 edit

Differences ...

==== //depot/projects/tcp_reass/netinet/tcp_reass.c#39 (text+ko) ====

@@ -139,13 +139,10 @@
 static struct mbuf *
 m_trimhead(struct mbuf *m)
 {
-	struct mbuf *n;
+
+	while (m != NULL && m->m_len == 0)
+		m = m_free(m);
 
-	while (m->m_len == 0) {
-		n = m;
-		m = m->m_next;
-		m_free(n);
-	}
 	return (m);
 }
 
@@ -197,7 +194,7 @@
 
 #ifdef INVARIANTS
 static int
-tcp_reass_verify(struct tcpcb *tp)
+tcp_reass_verify(struct tcpcb *tp, int prepost)
 {
 	int i = 0, size = 0, total = 0;
 	struct mbuf *m;
@@ -206,8 +203,8 @@
 	RB_FOREACH_SAFE(trb, tcp_ra, &tp->rcv_reass, trbn) {
 		KASSERT(SEQ_LT(trb->trb_seqs, trb->trb_seqe),
 		    ("%s: trb_seqs >= trb_seqe", __func__));
-		KASSERT(SEQ_GT(trb->trb_seqs, tp->rcv_nxt),
-		    ("%s: rcv_nxt >= trb_seqs", __func__));
+		KASSERT(SEQ_GT(trb->trb_seqs, tp->rcv_nxt) ||
+		    prepost, ("%s: rcv_nxt >= trb_seqs", __func__));
 		KASSERT(trb->trb_m != NULL,
 		    ("%s: trb_m == NULL", __func__));
 		KASSERT(trb->trb_mt != NULL,
@@ -222,8 +219,8 @@
 		total += size;
 		i++;
 	}
-	KASSERT(tp->rcv_reass_size == total,
-	    ("%s: total not correct", __func__));
+	//KASSERT(tp->rcv_reass_size == total,
+	//    ("%s: total not correct", __func__));
 
 	LIST_FOREACH(trb, &tp->rcv_reass_sack, trb_sack) {
 		i--;
@@ -244,7 +241,6 @@
 	LIST_REMOVE(trb, trb_sack);
 	if (trb->trb_m != NULL)
 		m_freem(trb->trb_m);
-	tp->rcv_reass_size -= SEQ_DELTA(trb->trb_seqs, trb->trb_seqe);
 	tp->rcv_reass_blocks--;
 	uma_zfree(tcp_reass_zone, trb);
 }
@@ -255,13 +251,13 @@
 	struct tcp_reass_block *trb, *trbn;
 
 	INP_WLOCK_ASSERT(tp->t_inpcb);
-	KASSERT(tcp_reass_verify(tp),
+	KASSERT(tcp_reass_verify(tp, 0),
 	    ("%s: reassembly queue inconsistent", __func__));
 
 	RB_FOREACH_SAFE(trb, tcp_ra, &tp->rcv_reass, trbn) {
 		tcp_reass_free(tp, trb);
 	}
-	KASSERT(tp->rcv_reass_size == 0, ("%s: snd_sacked not zero", __func__));
+	//KASSERT(tp->rcv_reass_size == 0, ("%s: snd_sacked not zero", __func__));
 }
 
 static __inline void
@@ -269,8 +265,7 @@
 {
 
 	if (LIST_FIRST(&tp->rcv_reass_sack) != trb) {
-		if (!LIST_EMPTY(&tp->rcv_reass_sack))
-			LIST_REMOVE(trb, trb_sack);
+		LIST_REMOVE(trb, trb_sack);
 		LIST_INSERT_HEAD(&tp->rcv_reass_sack, trb, trb_sack);
 	}
 }
@@ -325,11 +320,11 @@
 	if (!tcp_reass_enable && RB_EMPTY(&tp->rcv_reass))
 		goto done;
 
-	KASSERT(SEQ_LT(tp->rcv_nxt, th_seq),
+	KASSERT(SEQ_GEQ(th_seq, tp->rcv_nxt),
 	    ("%s: sequence number below rcv_nxt", __func__));
 	KASSERT(!(tp->rcv_nxt == th_seq) || !(RB_EMPTY(&tp->rcv_reass)),
 	    ("%s: got missing segment but queue is empty", __func__));
-	KASSERT(tcp_reass_verify(tp),
+	KASSERT(tcp_reass_verify(tp, 0),
 	    ("%s: reassembly queue already inconsistent", __func__));
 
 	/*
@@ -392,7 +387,7 @@
 	 */
 	m_demote(m, 1);
 	m = m_trimhead(m);
-	if (tcp_reass_spacetime)
+	if (tcp_reass_spacetime && m->m_next != NULL)
 		m = m_collapse(m, M_DONTWAIT, 1024);
 
 	KASSERT(m != NULL, ("%s: m is NULL after collapse", __func__));
@@ -408,7 +403,8 @@
 	 * insert a new reassembly block.
 	 */
 	if ((trb = RB_FIND(tcp_ra, &tp->rcv_reass, &trbs)) != NULL) {
-		/* Within an already known block. */
+
+		/* Within an already received block. */
 		if (SEQ_GEQ(trbs.trb_seqs, trb->trb_seqs) &&
 		    SEQ_LEQ(trbs.trb_seqe, trb->trb_seqe)) {
 			tcp_reass_sacktrack(tp, trb);
@@ -416,33 +412,25 @@
 			tp->rcv_reass_dsack.end = trbs.trb_seqe;
 			goto done;
 		}
-		tp->rcv_reass_size += SEQ_DELTA(trb->trb_seqs, trb->trb_seqe);
 
-		/* Extends the end, common case. */
-		if (SEQ_GT(trbs.trb_seqe, trb->trb_seqe)) {
-			(void)tcp_reass_merge(trb, &trbs);
-			tcp_reass_sacktrack(tp, trb);
+		/* Merge in the new segment. */
+		(void)tcp_reass_merge(trb, &trbs);
+		tcp_reass_sacktrack(tp, trb);
 
-			/* Merge in next blocks if there is overlap. */
-			while ((trbn = RB_NEXT(tcp_ra, &tp->rcv_reass, trb)) != NULL &&
-			    SEQ_GEQ(trb->trb_seqe, trbn->trb_seqs)) {
-				trbn = tcp_reass_merge(trb, trbn);
-				tcp_reass_free(tp, trbn);
-			}
+		/* Merge in previous block(s) if there is overlap. */
+		while ((trbn = RB_PREV(tcp_ra, &tp->rcv_reass, trb)) != NULL &&
+		    SEQ_LEQ(trb->trb_seqs, trbn->trb_seqe)) {
+			trbn = tcp_reass_merge(trb, trbn);
+			tcp_reass_free(tp, trbn);
 		}
 
-		/* Extends the start. */
-		if (SEQ_LT(trbs.trb_seqs, trb->trb_seqs)) {
-			(void)tcp_reass_merge(trb, &trbs);
-			tcp_reass_sacktrack(tp, trb);
+		/* Merge in next block(s) if there is overlap. */
+		while ((trbn = RB_NEXT(tcp_ra, &tp->rcv_reass, trb)) != NULL &&
+		    SEQ_GEQ(trb->trb_seqe, trbn->trb_seqs)) {
+			trbn = tcp_reass_merge(trb, trbn);
+			tcp_reass_free(tp, trbn);
+		}
 
-			/* Merge in previous blocks if there is overlap. */
-			while ((trbn = RB_PREV(tcp_ra, &tp->rcv_reass, trb)) != NULL &&
-			    SEQ_LEQ(trb->trb_seqs, trbn->trb_seqe)) {
-				trbn = tcp_reass_merge(trb, trbn);
-				tcp_reass_free(tp, trbn);
-			}
-		}
 	} else if (tp->rcv_nxt == th_seq) {
 		trb = &trbs;
 	} else if ((trb = (struct tcp_reass_block *)uma_zalloc(tcp_reass_zone, (M_NOWAIT|M_ZERO))) != NULL) {
@@ -452,14 +440,13 @@
 		trb->trb_mt = trbs.trb_mt;
 		trbn = RB_INSERT(tcp_ra, &tp->rcv_reass, trb);
 		KASSERT(trbn == NULL, ("%s: RB_INSERT failed", __func__));
-		tcp_reass_sacktrack(tp, trb);
-		tp->rcv_reass_size += SEQ_DELTA(trb->trb_seqs, trb->trb_seqe);
+		LIST_INSERT_HEAD(&tp->rcv_reass_sack, trb, trb_sack);
 		tp->rcv_reass_blocks++;
 	} else {
 		/* Memory allocation failure. */
 		goto done;
 	}
-	KASSERT(tcp_reass_verify(tp),
+	KASSERT(tcp_reass_verify(tp, 1),
 	    ("%s: reassembly queue went inconsistent", __func__));
 
 	if (trb->trb_seqs == tp->rcv_nxt)
@@ -500,8 +487,7 @@
 	 * deactivate it.
 	 */
 	if (tcp_reass_timeout && !RB_EMPTY(&tp->rcv_reass))
-		tcp_timer_activate(tp, TT_REASS,
-		    tp->t_rxtcur * tcp_reass_timeout);
+		tcp_timer_activate(tp, TT_REASS, tcp_reass_timeout);
 	else
 		tcp_timer_activate(tp, TT_REASS, 0);
 
@@ -515,8 +501,27 @@
 }
 
 /*
+ * Trim a block.
+ * Positive value is from head, negative from tail.
+ */
+static void
+tcp_reass_trim(struct tcp_reass_block *trb, int i)
+{
+
+	m_adj(trb->trb_m, i);
+
+	if (i < 0) {
+		trb->trb_mt = m_last(trb->trb_m);
+		trb->trb_seqe -= i;
+	} else {
+		trb->trb_m = m_trimhead(trb->trb_m);
+		trb->trb_seqs += i;
+	}
+}
+
+/*
  * Merge one or more consecutive blocks together.
- * Always merge trbn into trb!
+ * NB: trbn is always merged into trb!
  */
 static struct tcp_reass_block *
 tcp_reass_merge(struct tcp_reass_block *trb, struct tcp_reass_block *trbn)
@@ -526,40 +531,53 @@
 	KASSERT(trb != NULL && trbn != NULL,
 	    ("%s: incomplete input", __func__));
 
-	/* Append and prepend. */
-	if (SEQ_GEQ(trb->trb_seqe, trbn->trb_seqs)) {
-		if (SEQ_GEQ(trb->trb_seqe, trbn->trb_seqe))
-			return (trbn);
-		if ((i = SEQ_DELTA(trb->trb_seqe, trbn->trb_seqs)) > 0) {
-			m_adj(trbn->trb_m, i);
-			trbn->trb_m = m_trimhead(trbn->trb_m);
+	KASSERT(SEQ_LT(trbn->trb_seqs, trb->trb_seqs) ||
+	    SEQ_GT(trbn->trb_seqe, trb->trb_seqe),
+	    ("%s: ", __func__));
+
+	/* Replace, prepend and append. */
+	if (SEQ_LEQ(trbn->trb_seqs, trb->trb_seqs) &&
+	    SEQ_GEQ(trbn->trb_seqe, trb->trb_seqe)) {
+
+		m_freem(trb->trb_m);
+		trb->trb_seqs = trbn->trb_seqs;
+		trb->trb_seqe = trbn->trb_seqe;
+		trb->trb_m = trbn->trb_m;
+		trb->trb_mt = trbn->trb_mt;
+
+	} else if (SEQ_LT(trbn->trb_seqs, trb->trb_seqs)) {
+
+		if ((i = SEQ_DELTA(trb->trb_seqs, trbn->trb_seqe)) > 0)
+			tcp_reass_trim(trbn, -i);
+
+		trb->trb_seqs = trbn->trb_seqs;
+		trbn->trb_mt->m_next = trb->trb_m;
+		trb->trb_m = trbn->trb_m;
+
+		if (tcp_reass_spacetime) {
+			trbn->trb_mt = m_collapse(trbn->trb_mt, M_DONTWAIT, 1024);
+			trb->trb_mt = m_last(trbn->trb_mt);
 		}
+
+	} else if (SEQ_GT(trbn->trb_seqe, trb->trb_seqe)) {
+
+		if ((i = SEQ_DELTA(trb->trb_seqe, trbn->trb_seqs)) > 0)
+			tcp_reass_trim(trbn, i);
+
 		trb->trb_seqe = trbn->trb_seqe;
 		trb->trb_mt->m_next = trbn->trb_m;
+
 		if (tcp_reass_spacetime) {
 			trb->trb_mt = m_collapse(trb->trb_mt, M_DONTWAIT, 1024);
 			trb->trb_mt = m_last(trb->trb_mt);
 		} else
 			trb->trb_mt = trbn->trb_mt;
-	} else if (SEQ_LEQ(trb->trb_seqs, trbn->trb_seqe)) {
-		if (SEQ_LEQ(trb->trb_seqs, trbn->trb_seqs))
-			return (trbn);
-		if ((i = SEQ_DELTA(trb->trb_seqs, trbn->trb_seqe)) > 0) {
-			m_adj(trb->trb_m, i);
-			trb->trb_m = m_trimhead(trb->trb_m);
-		}
-		trb->trb_seqs = trbn->trb_seqs;
-		trb->trb_m = trbn->trb_m;
-		trbn->trb_mt->m_next = trb->trb_m;
-		if (tcp_reass_spacetime) {
-			trbn->trb_mt = m_collapse(trbn->trb_mt, M_DONTWAIT, 1024);
-			trb->trb_mt = m_last(trbn->trb_mt);
-		}
+
 	} else
 		return (NULL);
 
 	trbn->trb_seqs = 0;
-	trbn->trb_seqe = i;
+	trbn->trb_seqe = 0;
 	trbn->trb_m = NULL;
 	trbn->trb_mt = NULL;
 	return (trbn);		



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200907231507.n6NF74vk035511>