DCCP: Increment sequence number on retransmitted

When retransmit the first REQUEST, the sequence number does not be increased. This is because before retransmit the first REQUEST packet, the icsk->icsk_retransmits is 0, so dccp_transmit_skb() will fetch the dp->dccps_iss as the retransmit sequence number.

This patch fix the problem.

@@ -69,9 +69,6 @@ static int dccp_transmit_skb(struct sock
 
 		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;
 			/* fall through */
 
 		case DCCP_PKT_SYNC:

@@ -526,6 +523,8 @@ int dccp_connect(struct sock *sk)
 
 	/* Initialise GAR as per 8.5; AWL/AWH are set in dccp_transmit_skb() */
 	dp->dccps_gar = dp->dccps_iss;
+	/* Initialise GSS to ISS - 1, will be increased in dccp_transmit_skb() */
+	dp->dccps_gss = SUB48(dp->dccps_iss, 1);
 
 	skb = alloc_skb(sk->sk_prot->max_header, sk->sk_allocation);
 	if (unlikely(skb = NULL))

Em Wed, Jun 04, 2008 at 03:37:19PM +0800, Wei Yongjun escreveu:

Looks like a hack, ugh. Proper fix must be not bumping it on dccp_transmit_skb when the packet is a REQUEST, no?

- Arnaldo

Quoting Arnaldo:

| Em Wed, Jun 04, 2008 at 03:37:19PM +0800, Wei Yongjun escreveu:
| > When retransmit the first REQUEST, the sequence number does not be  
| > increased. This is because before retransmit the first REQUEST packet,  
| > the icsk->icsk_retransmits is 0, so dccp_transmit_skb() will fetch the  
| > dp->dccps_iss as the retransmit sequence number.
| >
| > This patch fix the problem.
| 
| Looks like a hack, ugh. Proper fix must be not bumping it on
| dccp_transmit_skb when the packet is a REQUEST, no?
| 

Yes, correct, it is a hack. I was working on that until one hour ago and then decided to put it aside. The fix is in the test tree for now but is not meant to remain there.

The problem is that, for the three retransmittable packet types in DCCP (Close, CloseReq, Request), there is no obvious way of distinguishing the original packet from the cloned packet.

Here is the sketch of the solution I was working on, I'd be glad for input:
* dccp_entail(), skb_clone(), dccp_transmit_skb() and setting of the icsk retransmit timer always occur together;
* as soon as skb_clone() is called, skb->cloned is true;
* so the idea is to
- use dccp_transmit_skb() to transmit the un-cloned skb;
- sk->send_head = skb_clone(skb, gfp_any())
- need some more careful analysis, but I think the set_owner_w() can also be removed, needs checking against the WARN_ON in dccp_transmit_skb().

Or maybe there is an easier way - the objective is to have a clear indicator whether the skb is a retransmitted one or the first one (since a similar problem arises when sending Close(Req) packets).

Suggestions welcome.

Gerrit

Em Wed, Jun 04, 2008 at 02:38:49PM +0100, Gerrit Renker escreveu:

But why do you have to? If it is a REQUEST, don't bump the GSS... wait, I actually took the time and went to re-read the RFC and I think the changelog Wei wrote is misleading, as:

-------------------- 8< ------------------------------------------------
A client in the REQUEST state SHOULD use an exponential-backoff timer to send new DCCP-Request packets if no response is received. The first retransmission should occur after approximately one second, backing off to not less than one packet every 64 seconds; or the

Kohler, et al. Standards Track [Page 58]
^L
RFC 4340 Datagram Congestion Control Protocol (DCCP) March 2006

endpoint can use whatever retransmission strategy is followed for retransmitting TCP SYNs. Each new DCCP-Request MUST increment the Sequence Number by one and MUST contain the same Service Code and application data as the original DCCP-Request.
-------------------- 8< ------------------------------------------------

So the fix would be for not bumping GSS on the first packet? I guess so, the flow is:

To create a socket:

dccp_v4_connect(): sets iss
dccp_connect()
dccp_connect_init(): iss = gss
dccp_transmit_skb: gss += 1

So the problem is that if this is the first packet being sent on a connection we should not do the gss += 1...

...since I think I got to the same page as you guys... :-) Lets continue from this point:

I think we have to use set_owner_w, its safer, we better get the connection stuck due to using all its socket send buffer space if the packet is never freed because of some stack bug than to get the whole machine with a problem, even if after a long time, if we leak these packets.

Perhaps we can use a flag in skb->cb that we would set only on the first packet? Nah, I think we should have a __dccp_transmit_skb, that... code, lets show it as a patch, see at the end of this message.

WRT Close(Req) I think it is right as it is, i.e. we should bump GSS, if not we'll send the packet with the same sequence number as the last packet sent, i.e. the problem is just on the very first packet.

Or am I missing something?

- Arnaldo

index 1f8a9b6..3110df1 100644
@@ -39,7 +39,7 @@ static void dccp_skb_entail(struct sock *sk, struct sk_buff *skb)
  * IP so it can do the same plus pass the packet off to the
  * device.
  */
-static int dccp_transmit_skb(struct sock *sk, struct sk_buff *skb)
+static int __dccp_transmit_skb(struct sock *sk, struct sk_buff *skb)
 {
 	if (likely(skb != NULL)) {
 		const struct inet_sock *inet = inet_sk(sk);

@@ -54,8 +54,6 @@ static int dccp_transmit_skb(struct sock *sk, struct sk_buff *skb)
 		int err, set_ack = 1;
 		u64 ackno = dp->dccps_gsr;
 
-		dccp_inc_seqno(&dp->dccps_gss);
-
 		switch (dcb->dccpd_type) {
 		case DCCP_PKT_DATA:
 			set_ack = 0;

@@ -132,6 +130,12 @@ static int dccp_transmit_skb(struct sock *sk, struct sk_buff *skb)
 	return -ENOBUFS;
 }
 
+static int dccp_transmit_skb(struct sock *sk, struct sk_buff *skb)
+{
+	dccp_inc_seqno(&dccp_sk(sk)->dccps_gss);
+	__dccp_transmit_skb(sk, skb);
+}
+
 /**
  * dccp_determine_ccmps  -  Find out about CCID-specfic packet-size limits
  * We only consider the HC-sender CCID for setting the CCMPS (RFC 4340, 14.),

@@ -472,7 +476,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, skb_clone(skb, GFP_KERNEL));
 	DCCP_INC_STATS(DCCP_MIB_ACTIVEOPENS);
 
 	/* Timer for repeating the REQUEST until an answer. */

| But why do you have to? If it is a REQUEST, don't bump the GSS... wait,
| I actually took the time and went to re-read the RFC and I think the
| changelog Wei wrote is misleading, as:
| 
| -------------------- 8< ------------------------------------------------
|    A client in the REQUEST state SHOULD use an exponential-backoff timer
|    to send new DCCP-Request packets if no response is received.  The
|    first retransmission should occur after approximately one second,
|    backing off to not less than one packet every 64 seconds; or the
|    endpoint can use whatever retransmission strategy is followed for
|    retransmitting TCP SYNs.  Each new DCCP-Request MUST increment the
|    Sequence Number by one and MUST contain the same Service Code and
|    application data as the original DCCP-Request.
| -------------------- 8< ------------------------------------------------
| 
| So the fix would be for not bumping GSS on the first packet? I guess so,
| the flow is:
| 
| To create a socket:
| 
|  dccp_v4_connect(): sets iss
|    dccp_connect()
|      dccp_connect_init(): iss = gss
|      dccp_transmit_skb: gss += 1
| 
| So the problem is that if this is the first packet being sent on a
| connection we should not do the gss += 1...
| 

Yes exactly that is the problem. And I find it ugly, too, to first set ISS, and then set GSS=ISS+1.

| > Here is the sketch of the solution I was working on, I'd be glad for input:
| >  * dccp_entail(), skb_clone(), dccp_transmit_skb() and setting of the icsk
| >    retransmit timer always occur together;
| >  * as soon as skb_clone() is called, skb->cloned is true;
| >  * so the idea is to 
| >    - use dccp_transmit_skb() to transmit the un-cloned skb;
| >    - sk->send_head = skb_clone(skb, gfp_any())
| >    - need some more careful analysis, but I think the set_owner_w() can
| >      also be removed, needs checking against the WARN_ON in
| >      dccp_transmit_skb().
| > 
| > Or maybe there is an easier way - the objective is to have a clear
| > indicator whether the skb is a retransmitted one or the first one
| > (since a similar problem arises when sending Close(Req) packets).
| 
| ...since I think I got to the same page as you guys... :-) Lets continue
| from this point:
| 
| I think we have to use set_owner_w, its safer, we better get the
| connection stuck due to using all its socket send buffer space if the
| packet is never freed because of some stack bug than to get the whole
| machine with a problem, even if after a long time, if we leak these
| packets.
| 
| Perhaps we can use a flag in skb->cb that we would set only on the first
| packet? Nah, I think we should have a __dccp_transmit_skb, that... code,
| lets show it as a patch, see at the end of this message.
| 

I had a further look, it is actually possible. Your point about set_owner_w is good - I've had to reboot three times today because of non-freed references.

What I found so far is that skb_cloned(skb) in dccp_retransmit_skb is never true, since we put the original skb onto the sk->send_head, so dccp_retransmit_skb() always calls skb_clone. That is why dccp_transmit_skb() never complains that skb->sk is already set.

What I was hoping for was a rewrite of the dccp_retransmit_skb(), so that the original skb is transmitted first and the clone put onto the send-head. But doesn't work since queue_xmit frees the original skb, so was stuck there.

| WRT Close(Req) I think it is right as it is, i.e. we should bump GSS, if
| not we'll send the packet with the same sequence number as the last
| packet sent, i.e. the problem is just on the very first packet.
| 
| Or am I missing something?

Correct - this applies only to the initial packet, so we can safely increment GSS on entering dccp_transmit_skb().

I will look at your code at home, we will be shutting down here everything in 15 minutes.

Thanks a lot for the input,
Gerrit

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: Allow to distinguish original and retransmitted packets

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.

---
 net/dccp/dccp.h   |    2 +-
 net/dccp/output.c |   20 ++++++++++++++++----
 net/dccp/timer.c  |   20 ++++----------------
 3 files changed, 21 insertions(+), 21 deletions(-)

@@ -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);

@@ -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,

@@ -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: 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().

---
 net/dccp/output.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

@@ -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);
 }

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.

---
 net/dccp/dccp.h   |    2 +-
 net/dccp/output.c |   20 ++++++++++++++++----
 net/dccp/timer.c  |   20 ++++----------------
 3 files changed, 21 insertions(+), 21 deletions(-)

@@ -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);

@@ -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,

@@ -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)

Em Thu, Jun 05, 2008 at 03:45:29PM +0100, Gerrit Renker escreveu:

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

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

Em Wed, Jun 04, 2008 at 04:55:40PM +0100, Gerrit Renker escreveu:

One thing to always have in mind is that we should, as long as it remains sane, to try to have the same code structure as TCP, so that people that are used to the TCP code can read the DCCP more easily and to identify areas that are are common.

So when trying to change things like this check if TCP has a similar function and if the change would make sense there too.

OK.

- Arnaldo

| One thing to always have in mind is that we should, as long as it
| remains sane, to try to have the same code structure as TCP, so that
| people that are used to the TCP code can read the DCCP more easily and
| to identify areas that are are common.
| 
| So when trying to change things like this check if TCP has a similar
| function and if the change would make sense there too.
|  

More than happy to do that since the TCP code is so very good.

I was wondering if it would be possible to convert part of CCID-2 to directly call TCP functions?

There is a lot of overlap, and much in the CCID-2 specification is directly from TCP.

So far there are at least three cases where CCID-2 has benefited from TCP:

* the computation of the RTO (with the clamping to 200ms), this is already in the test tree
http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p�cp_exp.git;a=commit;h#62d489189bf73b47ff778f7f5eca1daaea5f7b

* Congestion Window Validation (CWV, RFC 2861)
This is still in my development tree, as I have not had time to write up an experimental evaluation. I have had good experiences on a wireless link with it (where the Acks arrive in a bundle since the communication is not full duplex); cwnd growth was really well-behaved.

* have submitted one patch for RFC 3390 conversion where CCID-2/TCP could use the same function:
http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p�cp_exp.git;a=commitdiff;h"b6d875e14b70e3959641512c15cccef4f90692

With regard to the DCCP retransmissions, will reply in other thread. Gerrit
--
To unsubscribe from this list: send the line "unsubscribe dccp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I have thought over the approach consisting of

* creating __dccp_transmit_skb()
- as the old dccp_transmit_skb(), but without dccp_inc_seqno(&GSS)
- called directly only from dccp_connect(), so that correctly ISS is not incremented before sending the first Request packet.

* creating a new dccp_transmit_skb()
- which is called from all other code paths except from dccp_connect()
- thus including the retransmissions of Requests via dccp_retransmit_skb()
- which increments GSS only when it is already greater than or equal to ISS;
- and calls __dccp_transmit_skb() for the rest.

The approach is correct and works.

I am having a second problem here which the following patch in the test tree tried to solve,
http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p�cp_exp.git;a=commitdiff;h9c8cc01caf3da8e65d82b5d47d377bafbd51bd

The problem is that AWL=GSS was only set in dccp_connect(), so we need to call dccp_update_gss() in addition to dccp_inc_seqno().

It gets a bit more complicated, since
* the option-insertion code may rely on the current value of GSS (Ack Vectors are an example);
* the option-insertion code can fail (e.g. insufficient space left for options on skb);
* so first incrementing GSS and then aborting dccp_transmit_skb() would cause a sequence hole.

The approach taken in the above patch-URL is to
* assign the incremented value of GSS to the temporary `seq' field in dccp_skb_cb;
* overwrite this `seq' field with ISS on the first Request;
* in all instances, update GSS for real only if option-insertion worked;
* that call to dccp_update_gss() also sets AWL, fixing the second problem.

The reason that this fix is at a later stage in the test tree is that updating AWL in turn depends on the value of the local Sequence Window feature (RFC 4340, 7.5.1), which in turn depends on the feature-negotiation code; this part is in
http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p�cp_exp.git;a=commitdiff;h�62a9e2fa53f20bf80f5691d757950fb86c6f30

The second step/bullet-point is, as pointed out by Wei Yongjun, still not working correctly with regard to incrementing sequence numbers on retransmissions.

I have an alternative suggestion for handling the retransmissions, am currently testing this, and will submit patch subsequently.

Gerrit
--
To unsubscribe from this list: send the line "unsubscribe dccp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html