[PATCH] SCTP: Fix DATA retransmit after fast retransmit

[PATCH] SCTP: Fix DATA retransmit after fast retransmit

[Wei Yongjun]

If DATA chunk is resend by fast retransmit, and no SACK is received, 
this DATA chunk can not be send again after T3 timeout.

This is because chunk->sent_at will be update after chunk is 
transmitted, but after fast retransmit, T3 timer will not be reset, so 
sctp_retransmit_mark() can not mark this chunk for retransmit, and will 
never sent again because not T3 timer is running.

You can test this by following data transmit:

Endpoint A                    Endpoint B

              <--------------  DATA1
              <--------------  DATA2
              <--------------  DATA3
              <--------------  DATA4
              <--------------  DATA5
 SACK(1,2)    --------------->
 SACK(1,3)    --------------->
 SACK(1,3,4)  --------------->
 SACK(1,3,4,5)--------------->
              <--------------  DATA2 (Fast retransmit)
 NO SACK is sent, DATA2 will never be send again.


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

--- a/net/sctp/outqueue.c	2008-04-18 09:00:58.000000000 -0400
+++ b/net/sctp/outqueue.c	2008-04-18 09:06:07.000000000 -0400
@@ -425,7 +425,8 @@ void sctp_retransmit_mark(struct sctp_ou
 			 * retransmitting due to T3 timeout.
 			 */
 			if (reason = SCTP_RTXR_T3_RTX &&
-			    (jiffies - chunk->sent_at) < transport->last_rto)
+			    (jiffies - chunk->sent_at) < transport->last_rto &&
+			    !chunk->fast_retransmit)
 				continue;
 
 			/* RFC 2960 6.2.1 Processing a Received SACK

Re: [PATCH] SCTP: Fix DATA retransmit after fast retransmit

[Vlad Yasevich]

Wei Yongjun wrote:
> If DATA chunk is resend by fast retransmit, and no SACK is received, 
> this DATA chunk can not be send again after T3 timeout.
> 
> This is because chunk->sent_at will be update after chunk is 
> transmitted, but after fast retransmit, T3 timer will not be reset, so 
> sctp_retransmit_mark() can not mark this chunk for retransmit, and will 
> never sent again because not T3 timer is running.
> 
> You can test this by following data transmit:
> 
> Endpoint A                    Endpoint B
> 
>              <--------------  DATA1
>              <--------------  DATA2
>              <--------------  DATA3
>              <--------------  DATA4
>              <--------------  DATA5
> SACK(1,2)    --------------->
> SACK(1,3)    --------------->
> SACK(1,3,4)  --------------->
> SACK(1,3,4,5)--------------->
>              <--------------  DATA2 (Fast retransmit)
> NO SACK is sent, DATA2 will never be send again.
> 
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> 
> --- a/net/sctp/outqueue.c    2008-04-18 09:00:58.000000000 -0400
> +++ b/net/sctp/outqueue.c    2008-04-18 09:06:07.000000000 -0400
> @@ -425,7 +425,8 @@ void sctp_retransmit_mark(struct sctp_ou
>              * retransmitting due to T3 timeout.
>              */
>             if (reason = SCTP_RTXR_T3_RTX &&
> -                (jiffies - chunk->sent_at) < transport->last_rto)
> +                (jiffies - chunk->sent_at) < transport->last_rto &&
> +                !chunk->fast_retransmit)
>                 continue;
> 
>             /* RFC 2960 6.2.1 Processing a Received SACK
> 
> 

The problem with this patch is that a fast retransmitted chunk ends up being
retransmitted again too early.

There is text in 4960 that I am really trying to understand the intention of:

   4)  Restart the T3-rtx timer only if the last SACK acknowledged the
       lowest outstanding TSN number sent to that address, or the
       endpoint is retransmitting the first outstanding DATA chunk sent
       to that address.

In particular the send part of the above sentence.

Is the meaning that when fast retransmitting the earliest outstanding DATA,
we should restart T3-RTX?

If that's the case, then when we fast-rtx DATA2 above, we should restart
t3-rtx timer.

-vlad

Re: [PATCH] SCTP: Fix DATA retransmit after fast retransmit

[Wei Yongjun]

Hi Vlad:

Vlad Yasevich wrote:
> Wei Yongjun wrote:
>> If DATA chunk is resend by fast retransmit, and no SACK is received, 
>> this DATA chunk can not be send again after T3 timeout.
>>
>> This is because chunk->sent_at will be update after chunk is 
>> transmitted, but after fast retransmit, T3 timer will not be reset, 
>> so sctp_retransmit_mark() can not mark this chunk for retransmit, and 
>> will never sent again because not T3 timer is running.
>>
>> You can test this by following data transmit:
>>
>> Endpoint A                    Endpoint B
>>
>>              <--------------  DATA1
>>              <--------------  DATA2
>>              <--------------  DATA3
>>              <--------------  DATA4
>>              <--------------  DATA5
>> SACK(1,2)    --------------->
>> SACK(1,3)    --------------->
>> SACK(1,3,4)  --------------->
>> SACK(1,3,4,5)--------------->
>>              <--------------  DATA2 (Fast retransmit)
>> NO SACK is sent, DATA2 will never be send again.
>
> The problem with this patch is that a fast retransmitted chunk ends up 
> being
> retransmitted again too early.
>
> There is text in 4960 that I am really trying to understand the 
> intention of:
>
>   4)  Restart the T3-rtx timer only if the last SACK acknowledged the
>       lowest outstanding TSN number sent to that address, or the
>       endpoint is retransmitting the first outstanding DATA chunk sent
>       to that address.
>
Following is my understand:

1. Restart the T3-rtx timer only if the last SACK acknowledged the 
lowest outstanding TSN number sent to that address. SACK4 as following 
acknowledged the lowest outstanding TSN number.

Endpoint A                    Endpoint B
             <--------------  DATA1 (TSN=1)
             <--------------  DATA2 (TSN=2)
             <--------------  DATA3 (TSN=3)
             <--------------  DATA4 (TSN=4)
             <--------------  DATA5 (TSN=5)
SACK1(1,2)(TSN=0)  --------------->
SACK2(1,3)(TSN=0)  --------------->
SACK3(1,3,4)(TSN=0)--------------->
SACK4(3,4,5)(TSN=1)--------------->  (*)
             <--------------  DATA2 (Fast retransmit)


2. The endpoint is retransmitting the first outstanding DATA chunk sent 
to that address.

Endpoint A                    Endpoint B
             <--------------  DATA1 (TSN=1)
             <--------------  DATA2 (TSN=2)
             <--------------  DATA3 (TSN=3)
             <--------------  DATA4 (TSN=4)
             <--------------  DATA5 (TSN=5)
SACK1(1,2)(TSN=0)    --------------->
SACK2(2,3)(TSN=0)    --------------->
SACK3(2,3,4)(TSN=0)  --------------->
SACK4(2,3,4,5)(TSN=0)--------------->
             <--------------  DATA1 (Fast retransmit)


T3-rtx timer will be restart only in the two case above.

> In particular the send part of the above sentence.
>
> Is the meaning that when fast retransmitting the earliest outstanding 
> DATA,
> we should restart T3-RTX?
>
> If that's the case, then when we fast-rtx DATA2 above, we should restart
> t3-rtx timer.
T3-rtx timer will not be restart, since it the last SACK not 
acknowledged the lowest outstanding TSN number.

So i think check for "!chunk->fast_retransmit" is correct in my patch. 
And fast retransmit also need some check to restart T3-rtx in the two 
case above.

Is it my understand correct?

Wei Yongjun

Re: [PATCH] SCTP: Fix DATA retransmit after fast retransmit

[Vlad Yasevich]

Hi Wei

I am attempting to reproduce this scenario and trying to understand
the packet sequence.

I have tried the following simple scenario and it works correctly:

Endpoint A                       Endpoint B
DATA (TSN = 1)  ------------->
                <-------------   SACK (CTSN = 1)

DATA (TSN = 2)  -- (lost)---->
DATA (TSN = 3)  ------------->
                <-------------   SACK (CTSN = 1, GAP = 3)

DATA (TSN = 4)  ------------->
                <------------    SACK (CTSN = 1, GAP = 3,4)

DATA (TSN = 5)  ------------->
                <------------    SACK (CTSN = 1, GAP = 3,4,5)

DATA (TSN = 2)  -(fast rtx)-->
                <---(lost)----   SACK (CTSN = 5)

DATA (TSN = 2)  -(t3 rtx) --->

The T3_RTX retransmission happens because at the initial sending
of TSN 2 (the one that was lost), the rtx timer was set and
running.  It was never reset since not all outstanding chunks
have been acknowledged.

Can you describe the SACKs in your scenario with CTSN and GAP breakdowns
so I can try to reproduce it.

Thanks
-vlad

Wei Yongjun wrote:
> Hi Vlad:
> 
> Vlad Yasevich wrote:
>> Wei Yongjun wrote:
>>> If DATA chunk is resend by fast retransmit, and no SACK is received, 
>>> this DATA chunk can not be send again after T3 timeout.
>>>
>>> This is because chunk->sent_at will be update after chunk is 
>>> transmitted, but after fast retransmit, T3 timer will not be reset, 
>>> so sctp_retransmit_mark() can not mark this chunk for retransmit, and 
>>> will never sent again because not T3 timer is running.
>>>
>>> You can test this by following data transmit:
>>>
>>> Endpoint A                    Endpoint B
>>>
>>>              <--------------  DATA1
>>>              <--------------  DATA2
>>>              <--------------  DATA3
>>>              <--------------  DATA4
>>>              <--------------  DATA5
>>> SACK(1,2)    --------------->
>>> SACK(1,3)    --------------->
>>> SACK(1,3,4)  --------------->
>>> SACK(1,3,4,5)--------------->
>>>              <--------------  DATA2 (Fast retransmit)
>>> NO SACK is sent, DATA2 will never be send again.
>>
>> The problem with this patch is that a fast retransmitted chunk ends up 
>> being
>> retransmitted again too early.
>>
>> There is text in 4960 that I am really trying to understand the 
>> intention of:
>>
>>   4)  Restart the T3-rtx timer only if the last SACK acknowledged the
>>       lowest outstanding TSN number sent to that address, or the
>>       endpoint is retransmitting the first outstanding DATA chunk sent
>>       to that address.
>>
> Following is my understand:
> 
> 1. Restart the T3-rtx timer only if the last SACK acknowledged the 
> lowest outstanding TSN number sent to that address. SACK4 as following 
> acknowledged the lowest outstanding TSN number.
> 
> Endpoint A                    Endpoint B
>             <--------------  DATA1 (TSN=1)
>             <--------------  DATA2 (TSN=2)
>             <--------------  DATA3 (TSN=3)
>             <--------------  DATA4 (TSN=4)
>             <--------------  DATA5 (TSN=5)
> SACK1(1,2)(TSN=0)  --------------->
> SACK2(1,3)(TSN=0)  --------------->
> SACK3(1,3,4)(TSN=0)--------------->
> SACK4(3,4,5)(TSN=1)--------------->  (*)
>             <--------------  DATA2 (Fast retransmit)
> 
> 
> 2. The endpoint is retransmitting the first outstanding DATA chunk sent 
> to that address.
> 
> Endpoint A                    Endpoint B
>             <--------------  DATA1 (TSN=1)
>             <--------------  DATA2 (TSN=2)
>             <--------------  DATA3 (TSN=3)
>             <--------------  DATA4 (TSN=4)
>             <--------------  DATA5 (TSN=5)
> SACK1(1,2)(TSN=0)    --------------->
> SACK2(2,3)(TSN=0)    --------------->
> SACK3(2,3,4)(TSN=0)  --------------->
> SACK4(2,3,4,5)(TSN=0)--------------->
>             <--------------  DATA1 (Fast retransmit)
> 
> 
> T3-rtx timer will be restart only in the two case above.
> 
>> In particular the send part of the above sentence.
>>
>> Is the meaning that when fast retransmitting the earliest outstanding 
>> DATA,
>> we should restart T3-RTX?
>>
>> If that's the case, then when we fast-rtx DATA2 above, we should restart
>> t3-rtx timer.
> T3-rtx timer will not be restart, since it the last SACK not 
> acknowledged the lowest outstanding TSN number.
> 
> So i think check for "!chunk->fast_retransmit" is correct in my patch. 
> And fast retransmit also need some check to restart T3-rtx in the two 
> case above.
> 
> Is it my understand correct?
> 
> Wei Yongjun
> 

Re: [PATCH] SCTP: Fix DATA retransmit after fast retransmit

[Wei Yongjun]

[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]

Hi Vlad:

The test sequence is the same as yours. The data flow see the following. 
If you can receive attachment, see the tcpdump file in the attachment.

Endpoint A                       Endpoint B
DATA (TSN = 1)  ------------->
DATA (TSN = 2)  -- (lost)---->
DATA (TSN = 3)  ------------->
DATA (TSN = 4)  ------------->
               <-------------   SACK (CTSN = 1)
DATA (TSN = 5)  ------------->
               <-------------   SACK (CTSN = 1, GAP-START = 2, GAP-ENT = 2)
               <------------    SACK (CTSN = 1, GAP-START = 2, GAP-ENT = 3)
               <------------    SACK (CTSN = 1, GAP-START = 2, GAP-ENT = 4)
DATA (TSN = 2)  -(fast rtx)-->
               <---(lost)----   SACK (CTSN = 5)
NO DATA send   --(t3-rtx)----
HEARTBEAT      -(hb rtx1)-->
HEARTBEAT      -(hb rtx1)-->

I do not know what packet generater you used for test, maybe it send too 
fast, I think If you do same delay before send SACK (CTSN = 1, 
GAP=3,4,5), you can find this problem.

Wei Yongjun

Vlad Yasevich wrote:
> Hi Wei
>
> I am attempting to reproduce this scenario and trying to understand
> the packet sequence.
>
> I have tried the following simple scenario and it works correctly:
>
> Endpoint A                       Endpoint B
> DATA (TSN = 1)  ------------->
>                <-------------   SACK (CTSN = 1)
>
> DATA (TSN = 2)  -- (lost)---->
> DATA (TSN = 3)  ------------->
>                <-------------   SACK (CTSN = 1, GAP = 3)
>
> DATA (TSN = 4)  ------------->
>                <------------    SACK (CTSN = 1, GAP = 3,4)
>
> DATA (TSN = 5)  ------------->
>                <------------    SACK (CTSN = 1, GAP = 3,4,5)
>
> DATA (TSN = 2)  -(fast rtx)-->
>                <---(lost)----   SACK (CTSN = 5)
>
> DATA (TSN = 2)  -(t3 rtx) --->
>
> The T3_RTX retransmission happens because at the initial sending
> of TSN 2 (the one that was lost), the rtx timer was set and
> running.  It was never reset since not all outstanding chunks
> have been acknowledged.
>
> Can you describe the SACKs in your scenario with CTSN and GAP breakdowns
> so I can try to reproduce it.




[-- Attachment #2: 1.html.Link0.dump --]
[-- Type: application/octet-stream, Size: 8450 bytes --]

Re: [PATCH] SCTP: Fix DATA retransmit after fast retransmit

[Vlad Yasevich]

Hi Wei

I was able to reproduce the problem as well as understand when the rfc really
wants us to do.

On further analysis we have a lot of problems with fast retransmit.

1.  We really should be restarting the T3 timer any time we fast retransmit the
lowest outstanding TSN.  In your scenario, we would restart the T3-rtx time when
we fast retransmit TSN 2.

2.  We should not change the cnwd every time we enter hit the fast retransmit
event.  We should follow the Fast Recovery algorithm as specified in the spec.

3.  We only be sending 1 MTU worth fast retransmit.  What happens now is if we have
multiple chunks to retransmit, we queue all the chunks on the retransmit queue, fast
retransmit one and call outq_flush() which will send the rest, assuming cwnd allows it.
For an on-the-wire observer, it appears like we fast retransmitted more the 1-mtu worth
of data.

-vlad

Re: [PATCH] SCTP: Fix DATA retransmit after fast retransmit

[Wei Yongjun]

Hi Vlad:

Vlad Yasevich wrote:
> Hi Wei
>
> I was able to reproduce the problem as well as understand when the rfc 
> really
> wants us to do.
>
> On further analysis we have a lot of problems with fast retransmit.
>
> 1.  We really should be restarting the T3 timer any time we fast 
> retransmit the
> lowest outstanding TSN.  In your scenario, we would restart the T3-rtx 
> time when
> we fast retransmit TSN 2.

Does this means that when we do fast retransmit, we would restart the 
T3-rtx time?
If not, fast retransmit TSN1 or TSN5 or the other does not need restart 
the T3-rtx?
 
>
> 2.  We should not change the cnwd every time we enter hit the fast 
> retransmit
> event.  We should follow the Fast Recovery algorithm as specified in 
> the spec.
>
> 3.  We only be sending 1 MTU worth fast retransmit.  What happens now 
> is if we have
> multiple chunks to retransmit, we queue all the chunks on the 
> retransmit queue, fast
> retransmit one and call outq_flush() which will send the rest, 
> assuming cwnd allows it.
> For an on-the-wire observer, it appears like we fast retransmitted 
> more the 1-mtu worth
> of data.
>

Re: [PATCH] SCTP: Fix DATA retransmit after fast retransmit

[Vlad Yasevich]

Wei Yongjun wrote:
> Hi Vlad:
> 
> Vlad Yasevich wrote:
>> Hi Wei
>>
>> I was able to reproduce the problem as well as understand when the rfc 
>> really
>> wants us to do.
>>
>> On further analysis we have a lot of problems with fast retransmit.
>>
>> 1.  We really should be restarting the T3 timer any time we fast 
>> retransmit the
>> lowest outstanding TSN.  In your scenario, we would restart the T3-rtx 
>> time when
>> we fast retransmit TSN 2.
> 
> Does this means that when we do fast retransmit, we would restart the 
> T3-rtx time?

Yes, but only when fast retransmitting the lowest outstanding TSN.
So, let's take the scenario we've been working with.

Endpoint A                       Endpoint B
DATA (TSN = 1)  ------------->
DATA (TSN = 2)  -- (lost)---->
DATA (TSN = 3)  ------------->
DATA (TSN = 4)  ------------->
              <-------------   SACK (CTSN = 1)
                                   At this point, lowest outstanding is 2.

DATA (TSN = 5)  ------------->
              <-------------   SACK (CTSN = 1, GAP-START = 2, GAP-ENT = 2)
              <------------    SACK (CTSN = 1, GAP-START = 2, GAP-ENT = 3)
              <------------    SACK (CTSN = 1, GAP-START = 2, GAP-ENT = 4)

DATA (TSN = 2)  -(fast rtx)-->
    Since we are doing fast rtx for lowest outstanding, restart the T3 timer
    giving this chunk a chance to be acked.

              <---(lost)----   SACK (CTSN = 5) 


We can also extend this scenario a little bit more:

DATA (TSN = 1)  ------------->
DATA (TSN = 2)  -- (lost)---->
DATA (TSN = 3)  ------------->
DATA (TSN = 4)  -- (lost)---->
              <-------------   SACK (CTSN = 1)
                                   At this point, lowest outstanding is 2.

DATA (TSN = 5)  ------------->
              <-------------   SACK (CTSN = 1, GAP-START = 2, GAP-ENT = 2)
              <------------    SACK (CTSN = 1, GAP-START = 2, GAP-ENT = 2, GAP-START = 4, END = 4)
DATA (TSN = 6) -------------->
              <------------    SACK (CTSN = 1, GAP-START = 2, GAP-ENT = 2, GAP-START = 4, END = 5)

DATA (TSN = 2)  -(fast rtx, lost)-->
    Since we are doing fast rtx for lowest outstanding, restart the T3 timer
    giving this chunk a chance to be acked.

DATA (TSN = 7) ------------>
              <-------------   SACK (CTSN = 1, GAP-START = 2, GAP-ENT = 2, GAP-START = 4, END = 6)

DATA (TSN = 4) -(fast rtx) ---->
    At this point since TSN 2 is still the lowest outstanding, we do NOTrestart
    the T3 timer.


> If not, fast retransmit TSN1 or TSN5 or the other does not need restart 
> the T3-rtx?

Does the above example clarify?

-vlad