[dccp] [RFC/RFT] [Patch 0/2]: Test-tree update to handle retransmissions correctly

[dccp] [RFC/RFT] [Patch 0/2]: Test-tree update to handle retransmissions correctly

[Gerrit Renker]

Here are two patches for the test tree. As with all RFC/RFT patches, these are
posted with the main objective of getting review/comments, and are then put into
the test tree for ... testing.

These patches evolved thanks to input provided by Wei Yongjun regarding an
unresolved problem with retransmissions, fixed by the following two updates:

Patch #1: allows to distinguish retransmitted from original packets,
          using a slight reordering of existing code.
          This is the main fix for the earlier problem of not incrementing
	  the sequence number properly on retransmitted Request packets.

	  The patch introduces a generic condition which can also be tested
	  by other code (e.g. for the retransmission of Close/CloseReq).

Patch #2: puts two strongly related statements, dccp_entail() and
          skb_clone() both in dccp_entail(), since they are not 
	  independent. Patch can be a matter of taste and may be omitted.


The patches are not yet committed - if there are no major objections, they will
be added to the test tree tomoroow, on 

	git://eden-feed.erg.abdn.ac.uk/dccp_exp (subtree dccp)

with snapshots on
	http://eden-feed.erg.abdn.ac.uk/latest-dccp-test-tree.tar.bz2
	http://eden-feed.erg.abdn.ac.uk/latest-dccp-test-tree.diff.gz

[dccp] [RFC/RFT] [Patch 1/2]: Allow to distinguish original and retransmitted packets

[Gerrit Renker]

dccp: Allow to distinguish original and retransmitted packets

Re: [dccp] [RFC/RFT] [Patch 1/2]: Allow to distinguish original and retransmitted packets

[Gerrit Renker]

dccp: Allow to distinguish original and retransmitted packets

This patch allows the sending code to distinguish original/initial packets
from retransmitted ones.

This is in particular needed for the retransmission of Request packets, since
 * the first packet uses ISS (generated in net/dccp/ipv?.c) and sets GSS=ISS;
 * all retransmitted packets use GSS' = GSS+1, so that the n-th retransmitted
   packet has sequence number ISS+n (mod 48).

To add this support, the patch reorganises existing code in such a manner that
 * icsk_retransmits == 0 for original packet and
 * icsk_retransmits = n > 0 for n-th retransmitted packet
at the time dccp_transmit_skb() is called via dccp_retransmit_skb().
 
The patch further exploits that, since sk_send_head always contains the original
skb (enqueued/tailed by dccp_entail()), skb_cloned() never evaluated to true.

Lastly, removed the `skb' argument from dccp_retransmit_skb(), since sk_send_head
is used for all retransmissions (the exception is client-Acks in PARTOPEN state,
but these are not put onto the sk_send_head). Updated the documentation also.

Many thanks to Arnaldo Carvalho de Melo and Wei Yongjun for pointing out
unresolved problems during development and for providing helpful input.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/dccp.h   |    2 +-
 net/dccp/output.c |   20 ++++++++++++++++----
 net/dccp/timer.c  |   20 ++++----------------
 3 files changed, 21 insertions(+), 21 deletions(-)

--- a/net/dccp/dccp.h
+++ b/net/dccp/dccp.h
@@ -207,7 +207,7 @@ static inline void dccp_csum_outgoing(st
 
 extern void dccp_v4_send_check(struct sock *sk, int len, struct sk_buff *skb);
 
-extern int  dccp_retransmit_skb(struct sock *sk, struct sk_buff *skb);
+extern int  dccp_retransmit_skb(struct sock *sk);
 
 extern void dccp_send_ack(struct sock *sk);
 extern void dccp_reqsk_send_ack(struct sk_buff *sk, struct request_sock *rsk);
--- a/net/dccp/output.c
+++ b/net/dccp/output.c
@@ -290,14 +290,26 @@ void dccp_write_xmit(struct sock *sk, in
 	}
 }
 
-int dccp_retransmit_skb(struct sock *sk, struct sk_buff *skb)
+/**
+ * dccp_retransmit_skb  -  Retransmit Request, Close, or CloseReq packets
+ * There are only four retransmittable packet types in DCCP:
+ * - Request  in client-REQUEST  state (sec. 8.1.1),
+ * - CloseReq in server-CLOSEREQ state (sec. 8.3),
+ * - Close    in   node-CLOSING  state (sec. 8.3),
+ * - Acks in client-PARTOPEN state (sec. 8.1.5, handled by dccp_delack_timer()).
+ * This function expects sk->sk_send_head to contain the original skb.
+ */
+int dccp_retransmit_skb(struct sock *sk)
 {
+	BUG_TRAP(sk->sk_send_head != NULL);
+
 	if (inet_csk(sk)->icsk_af_ops->rebuild_header(sk) != 0)
 		return -EHOSTUNREACH; /* Routing failure or similar. */
 
-	return dccp_transmit_skb(sk, (skb_cloned(skb) ?
-				      pskb_copy(skb, GFP_ATOMIC):
-				      skb_clone(skb, GFP_ATOMIC)));
+	/* this count is used to distinguish original and retransmitted skb */
+	inet_csk(sk)->icsk_retransmits++;
+
+	return dccp_transmit_skb(sk, skb_clone(sk->sk_send_head, GFP_ATOMIC));
 }
 
 struct sk_buff *dccp_make_response(struct sock *sk, struct dst_entry *dst,
--- a/net/dccp/timer.c
+++ b/net/dccp/timer.c
@@ -88,21 +88,11 @@ static void dccp_retransmit_timer(struct
 	struct inet_connection_sock *icsk = inet_csk(sk);
 
 	/*
-	 * sk->sk_send_head has to have one skb with
-	 * DCCP_SKB_CB(skb)->dccpd_type set to one of the retransmittable DCCP
-	 * packet types. The only packets eligible for retransmission are:
-	 *	-- Requests in client-REQUEST  state (sec. 8.1.1)
-	 *	-- Acks     in client-PARTOPEN state (sec. 8.1.5)
-	 *	-- CloseReq in server-CLOSEREQ state (sec. 8.3)
-	 *	-- Close    in   node-CLOSING  state (sec. 8.3)                */
-	BUG_TRAP(sk->sk_send_head != NULL);
-
-	/*
 	 * More than than 4MSL (8 minutes) has passed, a RESET(aborted) was
 	 * sent, no need to retransmit, this sock is dead.
 	 */
 	if (dccp_write_timeout(sk))
-		goto out;
+		return;
 
 	/*
 	 * We want to know the number of packets retransmitted, not the
@@ -111,29 +101,27 @@ static void dccp_retransmit_timer(struct
 	if (icsk->icsk_retransmits == 0)
 		DCCP_INC_STATS_BH(DCCP_MIB_TIMEOUTS);
 
-	if (dccp_retransmit_skb(sk, sk->sk_send_head) < 0) {
+	if (dccp_retransmit_skb(sk) != 0) {
 		/*
 		 * Retransmission failed because of local congestion,
 		 * do not backoff.
 		 */
-		if (icsk->icsk_retransmits == 0)
+		if (--icsk->icsk_retransmits == 0)
 			icsk->icsk_retransmits = 1;
 		inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
 					  min(icsk->icsk_rto,
 					      TCP_RESOURCE_PROBE_INTERVAL),
 					  DCCP_RTO_MAX);
-		goto out;
+		return;
 	}
 
 	icsk->icsk_backoff++;
-	icsk->icsk_retransmits++;
 
 	icsk->icsk_rto = min(icsk->icsk_rto << 1, DCCP_RTO_MAX);
 	inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS, icsk->icsk_rto,
 				  DCCP_RTO_MAX);
 	if (icsk->icsk_retransmits > sysctl_dccp_retries1)
 		__sk_dst_reset(sk);
-out:;
 }
 
 static void dccp_write_timer(unsigned long data)

[dccp] [RFC/RFT] [Patch 2/2]: Combine the functionality of enqeueing and cloning

[Gerrit Renker]

dccp: Combine the functionality of enqeueing and cloning

Realising that there is a pattern in that

 * first dccp_entail() is called to enqueue a new skb;
 * then skb_clone() is called to transmit a clone of that skb,

this patch integrates these two interrelated steps into dccp_entail.

Note: the return value of skb_clone is not checked. It may be an idea to add a
      warning if this occurs. In both instances, however, a timer is set for
      retransmission, so that cloning is re-tried via dccp_retransmit_skb().

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/output.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

--- a/net/dccp/output.c
+++ b/net/dccp/output.c
@@ -26,11 +26,13 @@ static inline void dccp_event_ack_sent(s
 	inet_csk_clear_xmit_timer(sk, ICSK_TIME_DACK);
 }
 
-static void dccp_skb_entail(struct sock *sk, struct sk_buff *skb)
+/* enqueue @skb on sk_send_head for retransmission, return clone to send now */
+static struct sk_buff *dccp_skb_entail(struct sock *sk, struct sk_buff *skb)
 {
 	skb_set_owner_w(skb, sk);
 	WARN_ON(sk->sk_send_head);
 	sk->sk_send_head = skb;
+	return skb_clone(sk->sk_send_head, gfp_any());
 }
 
 /*
@@ -536,8 +538,7 @@ int dccp_connect(struct sock *sk)
 
 	DCCP_SKB_CB(skb)->dccpd_type = DCCP_PKT_REQUEST;
 
-	dccp_skb_entail(sk, skb);
-	dccp_transmit_skb(sk, skb_clone(skb, GFP_KERNEL));
+	dccp_transmit_skb(sk, dccp_skb_entail(sk, skb));
 	DCCP_INC_STATS(DCCP_MIB_ACTIVEOPENS);
 
 	/* Timer for repeating the REQUEST until an answer. */
@@ -662,8 +663,7 @@ void dccp_send_close(struct sock *sk, co
 		DCCP_SKB_CB(skb)->dccpd_type = DCCP_PKT_CLOSE;
 
 	if (active) {
-		dccp_skb_entail(sk, skb);
-		dccp_transmit_skb(sk, skb_clone(skb, prio));
+		skb = dccp_skb_entail(sk, skb);
 		/*
 		 * Retransmission timer for active-close: RFC 4340, 8.3 requires
 		 * to retransmit the Close/CloseReq until the CLOSING/CLOSEREQ
@@ -676,6 +676,6 @@ void dccp_send_close(struct sock *sk, co
 		 */
 		inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
 					  DCCP_TIMEOUT_INIT, DCCP_RTO_MAX);
-	} else
-		dccp_transmit_skb(sk, skb);
+	}
+	dccp_transmit_skb(sk, skb);
 }

Re: [dccp] [RFC/RFT] [Patch 1/2]: Allow to distinguish original and retransmitted packets

[Arnaldo Carvalho de Melo]

Em Thu, Jun 05, 2008 at 03:45:29PM +0100, Gerrit Renker escreveu:
> dccp: Allow to distinguish original and retransmitted packets

Why not do it the way I suggested? I.e. using
__dccp_transmit_skb/dccp_transmit_skb? Less changes, same effect, or am
I missing something?

- Arnaldo

Re: [dccp] [RFC/RFT] [Patch 1/2]: Allow to distinguish original and retransmitted packets

[Gerrit Renker]

Quoting Arnaldo:
| Em Thu, Jun 05, 2008 at 03:45:29PM +0100, Gerrit Renker escreveu:
| > dccp: Allow to distinguish original and retransmitted packets
| 
| Why not do it the way I suggested? I.e. using
| __dccp_transmit_skb/dccp_transmit_skb? Less changes, same effect, or am
| I missing something?
| 
I had sat down and tried to integrate your solution and had replied in a
separate thread what the difficulties were in doing this. Yes, your
solution solves the problem of not incrementing GSS when sending the
initial Request packet via dccp_connect.

But there are two other problems which it didn't solve
 (a) when GSS is incremented in (__)dccp_transmit_skb() and the
     option-insertion code fails for some reason, there is a hole in the 
     sequence space. I don't know whether this is a big problem, but to
     me it seems cleaner to first assign the incremented value of GSS to
     a temporary variable and update GSS only if option-insertion succeeds;
 (b) in the mainline code updating AWL=GSS is missing, so we additionally
     need to call dccp_update_gss to update AWL. This is a bug which is
     fixed in the test tree. In RFC 4340, 7.5.1 it is defined for DCCP A as
          AWL := max(GSS + 1 - W', ISS).
     where W' is the local value of the Sequence Window. The problem in the
     mainline code is that there is no distinction between local and remote
     Sequence Window, i.e. there is only one variable and no negotiation.
     For this particular problem it  does not need feature negotiation
     since it is the local value and since Sequence Window is a non-negotiable
     feature (RFC 4340, 6.4), i.e. the remote peer has to accept this value
     via Change L(Sequence Window).
     But irrespective of feature negotiation, we need to call dccp_update_gss
     in dccp_transmit_skb, so that the Ack window is kept up-to-date with
     regard to GSS.

So, facing these two problems, I was stuck with how to integrate your
solution with the above and couldn't figure out how to fit that in.

That is because the control flow in dccp_transmit_skb, to solve the above, is
 
	/* assign incremented value to dccp_skb_cb */
	dcb->dccpd_seq = ADD48(dp->dccps_gss, 1);

	switch (dcb->dccpd_type) {
		case DCCP_PKT_REQUEST:
                        set_ack = 0;
                        /* Use ISS on the first (non-retransmitted) Request. */
                        if (icsk->icsk_retransmits == 0)
                                dcb->dccpd_seq = dp->dccps_iss;
			break;
	// ...
	}

	/* if this fails, GSS is not updated */
	if (dccp_insert_options(sk, skb)) {
		kfree_skb(skb);
		return -EPROTO;
	}

	// ...
	dccp_update_gss(sk, dcb->dccpd_seq);	/* this updates GSS, AWH, AWL */
	dccp_hdr_set_seq(dh, dp->dccps_gss);

	// ...
	// transmit the skb via queue_xmit

I have tested my code to work correctly with up to net.dccp.default.retries1=5,
it will also work correctly with higher values.

Gerrit