DCCP protocol discussions
 help / color / mirror / Atom feed
* [PATCH - test-tree] DCCP: Increment sequence number on retransmitted
@ 2008-06-04  7:37 Wei Yongjun
  2008-06-04  8:09 ` [PATCH - test-tree] DCCP: Increment sequence number on Gerrit Renker
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Wei Yongjun @ 2008-06-04  7:37 UTC (permalink / raw)
  To: dccp

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.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

--- a/net/dccp/output.c	2008-05-29 22:27:55.000000000 -0400
+++ b/net/dccp/output.c	2008-05-30 19:26:43.000000000 -0400
@@ -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))






^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH - test-tree] DCCP: Increment sequence number on
  2008-06-04  7:37 [PATCH - test-tree] DCCP: Increment sequence number on retransmitted Wei Yongjun
@ 2008-06-04  8:09 ` Gerrit Renker
  2008-06-04 13:21 ` Arnaldo Carvalho de Melo
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Gerrit Renker @ 2008-06-04  8:09 UTC (permalink / raw)
  To: dccp

> 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.
>
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>
> --- a/net/dccp/output.c	2008-05-29 22:27:55.000000000 -0400
> +++ b/net/dccp/output.c	2008-05-30 19:26:43.000000000 -0400
> @@ -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))
>
Ack - excellent spotting. 

The problem is that icsk->retransmits is increased only after calling
dccp_transmit_skb() via dccp_retransmit_skb(), hence the sequence number is
increased only in the second retransmit.

I will put this into the test tree later on today, I am thinking about
finding a different solution around the fact that GSS is always
increased at the top of dccp_transmit_skb().

Thanks a lot for investigating this.

Gerrit

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH - test-tree] DCCP: Increment sequence number on
  2008-06-04  7:37 [PATCH - test-tree] DCCP: Increment sequence number on retransmitted Wei Yongjun
  2008-06-04  8:09 ` [PATCH - test-tree] DCCP: Increment sequence number on Gerrit Renker
@ 2008-06-04 13:21 ` Arnaldo Carvalho de Melo
  2008-06-04 13:38 ` Gerrit Renker
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2008-06-04 13:21 UTC (permalink / raw)
  To: dccp

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?

- Arnaldo

> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>
> --- a/net/dccp/output.c	2008-05-29 22:27:55.000000000 -0400
> +++ b/net/dccp/output.c	2008-05-30 19:26:43.000000000 -0400
> @@ -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))

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH - test-tree] DCCP: Increment sequence number on
  2008-06-04  7:37 [PATCH - test-tree] DCCP: Increment sequence number on retransmitted Wei Yongjun
  2008-06-04  8:09 ` [PATCH - test-tree] DCCP: Increment sequence number on Gerrit Renker
  2008-06-04 13:21 ` Arnaldo Carvalho de Melo
@ 2008-06-04 13:38 ` Gerrit Renker
  2008-06-04 15:44 ` Arnaldo Carvalho de Melo
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Gerrit Renker @ 2008-06-04 13:38 UTC (permalink / raw)
  To: dccp

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH - test-tree] DCCP: Increment sequence number on
  2008-06-04  7:37 [PATCH - test-tree] DCCP: Increment sequence number on retransmitted Wei Yongjun
                   ` (2 preceding siblings ...)
  2008-06-04 13:38 ` Gerrit Renker
@ 2008-06-04 15:44 ` Arnaldo Carvalho de Melo
  2008-06-04 15:55 ` Gerrit Renker
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2008-06-04 15:44 UTC (permalink / raw)
  To: dccp

Em Wed, Jun 04, 2008 at 02:38:49PM +0100, Gerrit Renker escreveu:
> 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.

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...

> 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.

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

diff --git a/net/dccp/output.c b/net/dccp/output.c
index 1f8a9b6..3110df1 100644
--- a/net/dccp/output.c
+++ b/net/dccp/output.c
@@ -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. */

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH - test-tree] DCCP: Increment sequence number on
  2008-06-04  7:37 [PATCH - test-tree] DCCP: Increment sequence number on retransmitted Wei Yongjun
                   ` (3 preceding siblings ...)
  2008-06-04 15:44 ` Arnaldo Carvalho de Melo
@ 2008-06-04 15:55 ` Gerrit Renker
  2008-06-04 16:54 ` Arnaldo Carvalho de Melo
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Gerrit Renker @ 2008-06-04 15:55 UTC (permalink / raw)
  To: dccp

| 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH - test-tree] DCCP: Increment sequence number on
  2008-06-04  7:37 [PATCH - test-tree] DCCP: Increment sequence number on retransmitted Wei Yongjun
                   ` (4 preceding siblings ...)
  2008-06-04 15:55 ` Gerrit Renker
@ 2008-06-04 16:54 ` Arnaldo Carvalho de Melo
  2008-06-05  6:51 ` Gerrit Renker
  2008-06-05  7:33 ` Gerrit Renker
  7 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2008-06-04 16:54 UTC (permalink / raw)
  To: dccp

Em Wed, Jun 04, 2008 at 04:55:40PM +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
> |    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.

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.
 
> | 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.

OK.

- Arnaldo

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH - test-tree] DCCP: Increment sequence number on
  2008-06-04  7:37 [PATCH - test-tree] DCCP: Increment sequence number on retransmitted Wei Yongjun
                   ` (5 preceding siblings ...)
  2008-06-04 16:54 ` Arnaldo Carvalho de Melo
@ 2008-06-05  6:51 ` Gerrit Renker
  2008-06-05  7:33 ` Gerrit Renker
  7 siblings, 0 replies; 9+ messages in thread
From: Gerrit Renker @ 2008-06-05  6:51 UTC (permalink / raw)
  To: dccp

| 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH - test-tree] DCCP: Increment sequence number on
  2008-06-04  7:37 [PATCH - test-tree] DCCP: Increment sequence number on retransmitted Wei Yongjun
                   ` (6 preceding siblings ...)
  2008-06-05  6:51 ` Gerrit Renker
@ 2008-06-05  7:33 ` Gerrit Renker
  7 siblings, 0 replies; 9+ messages in thread
From: Gerrit Renker @ 2008-06-05  7:33 UTC (permalink / raw)
  To: dccp

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;h\x1a9c8cc01caf3da8e65d82b5d47d377bafbd51bd
	 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2008-06-05  7:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-06-04  7:37 [PATCH - test-tree] DCCP: Increment sequence number on retransmitted Wei Yongjun
2008-06-04  8:09 ` [PATCH - test-tree] DCCP: Increment sequence number on Gerrit Renker
2008-06-04 13:21 ` Arnaldo Carvalho de Melo
2008-06-04 13:38 ` Gerrit Renker
2008-06-04 15:44 ` Arnaldo Carvalho de Melo
2008-06-04 15:55 ` Gerrit Renker
2008-06-04 16:54 ` Arnaldo Carvalho de Melo
2008-06-05  6:51 ` Gerrit Renker
2008-06-05  7:33 ` Gerrit Renker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox