2013-10-14 13:33:48

by Chang Xiangzhong

Subject: [PATCH] net: sctp: fix a cacc_saw_newack missetting issue

For for each TSN t being newly acked (Not only cumulatively,
but also SELECTIVELY) cacc_saw_newack should be set to 1.

Signed-off-by: Xiangzhong Chang <[email protected]>
---
net/sctp/outqueue.c | 42 +++++++++++++++++++++---------------------
1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 94df758..d86032b 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -1398,6 +1398,27 @@ static void sctp_check_transmitted(struct sctp_outq *q,
forward_progress = true;
}

+ if (!tchunk->tsn_gap_acked) {
+ /*
+ * SFR-CACC algorithm:
+ * 2) If the SACK contains gap acks
+ * and the flag CHANGEOVER_ACTIVE is
+ * set the receiver of the SACK MUST
+ * take the following action:
+ *
+ * B) For each TSN t being acked that
+ * has not been acked in any SACK so
+ * far, set cacc_saw_newack to 1 for
+ * the destination that the TSN was
+ * sent to.
+ */
+ if (transport &&
+ sack->num_gap_ack_blocks &&
+ q->asoc->peer.primary_path->cacc.
+ changeover_active)
+ transport->cacc.cacc_saw_newack = 1;
+ }
+
if (TSN_lte(tsn, sack_ctsn)) {
/* RFC 2960 6.3.2 Retransmission Timer Rules
*
@@ -1411,27 +1432,6 @@ static void sctp_check_transmitted(struct sctp_outq *q,
restart_timer = 1;
forward_progress = true;

- if (!tchunk->tsn_gap_acked) {
- /*
- * SFR-CACC algorithm:
- * 2) If the SACK contains gap acks
- * and the flag CHANGEOVER_ACTIVE is
- * set the receiver of the SACK MUST
- * take the following action:
- *
- * B) For each TSN t being acked that
- * has not been acked in any SACK so
- * far, set cacc_saw_newack to 1 for
- * the destination that the TSN was
- * sent to.
- */
- if (transport &&
- sack->num_gap_ack_blocks &&
- q->asoc->peer.primary_path->cacc.
- changeover_active)
- transport->cacc.cacc_saw_newack
- = 1;
- }

list_add_tail(&tchunk->transmitted_list,
&q->sacked);
--
1.7.9.5

2013-10-15 14:12:03

by Vlad Yasevich

Subject: Re: [PATCH] net: sctp: fix a cacc_saw_newack missetting issue

On 10/14/2013 09:33 AM, Chang Xiangzhong wrote:
> For for each TSN t being newly acked (Not only cumulatively,
> but also SELECTIVELY) cacc_saw_newack should be set to 1.
>
> Signed-off-by: Xiangzhong Chang <[email protected]>
> ---
> net/sctp/outqueue.c | 42 +++++++++++++++++++++---------------------
> 1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 94df758..d86032b 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -1398,6 +1398,27 @@ static void sctp_check_transmitted(struct sctp_outq *q,
> forward_progress = true;
> }
>
> + if (!tchunk->tsn_gap_acked) {

You can remove this test since the block just above already performs
it. Just fold this code into the block above.

-vlad

> + /*
> + * SFR-CACC algorithm:
> + * 2) If the SACK contains gap acks
> + * and the flag CHANGEOVER_ACTIVE is
> + * set the receiver of the SACK MUST
> + * take the following action:
> + *
> + * B) For each TSN t being acked that
> + * has not been acked in any SACK so
> + * far, set cacc_saw_newack to 1 for
> + * the destination that the TSN was
> + * sent to.
> + */
> + if (transport &&
> + sack->num_gap_ack_blocks &&
> + q->asoc->peer.primary_path->cacc.
> + changeover_active)
> + transport->cacc.cacc_saw_newack = 1;
> + }
> +
> if (TSN_lte(tsn, sack_ctsn)) {
> /* RFC 2960 6.3.2 Retransmission Timer Rules
> *
> @@ -1411,27 +1432,6 @@ static void sctp_check_transmitted(struct sctp_outq *q,
> restart_timer = 1;
> forward_progress = true;
>
> - if (!tchunk->tsn_gap_acked) {
> - /*
> - * SFR-CACC algorithm:
> - * 2) If the SACK contains gap acks
> - * and the flag CHANGEOVER_ACTIVE is
> - * set the receiver of the SACK MUST
> - * take the following action:
> - *
> - * B) For each TSN t being acked that
> - * has not been acked in any SACK so
> - * far, set cacc_saw_newack to 1 for
> - * the destination that the TSN was
> - * sent to.
> - */
> - if (transport &&
> - sack->num_gap_ack_blocks &&
> - q->asoc->peer.primary_path->cacc.
> - changeover_active)
> - transport->cacc.cacc_saw_newack
> - = 1;
> - }
>
> list_add_tail(&tchunk->transmitted_list,
> &q->sacked);
>

2013-10-15 14:27:12

by Chang Xiangzhong

Subject: Re: [PATCH] net: sctp: fix a cacc_saw_newack missetting issue

On 10/15/2013 04:11 PM, Vlad Yasevich wrote:
> On 10/14/2013 09:33 AM, Chang Xiangzhong wrote:
>> For for each TSN t being newly acked (Not only cumulatively,
>> but also SELECTIVELY) cacc_saw_newack should be set to 1.
>>
>> Signed-off-by: Xiangzhong Chang <[email protected]>
>> ---
>> net/sctp/outqueue.c | 42 +++++++++++++++++++++---------------------
>> 1 file changed, 21 insertions(+), 21 deletions(-)
>>
>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>> index 94df758..d86032b 100644
>> --- a/net/sctp/outqueue.c
>> +++ b/net/sctp/outqueue.c
>> @@ -1398,6 +1398,27 @@ static void sctp_check_transmitted(struct
>> sctp_outq *q,
>> forward_progress = true;
>> }
>>
>> + if (!tchunk->tsn_gap_acked) {
>
> You can remove this test since the block just above already performs
> it. Just fold this code into the block above.
>
> -vlad
>
Sorry, I'm not sure if I fully understand you. There are code blocks
which checking the tchunk->tsn_gap_acked. In addition, they check other
states as well.
>> + /*
>> + * SFR-CACC algorithm:
>> + * 2) If the SACK contains gap acks
>> + * and the flag CHANGEOVER_ACTIVE is
>> + * set the receiver of the SACK MUST
>> + * take the following action:
>> + *
>> + * B) For each TSN t being acked that
>> + * has not been acked in any SACK so
>> + * far, set cacc_saw_newack to 1 for
>> + * the destination that the TSN was
>> + * sent to.
>> + */
>> + if (transport &&
>> + sack->num_gap_ack_blocks &&
>> + q->asoc->peer.primary_path->cacc.
>> + changeover_active)
>> + transport->cacc.cacc_saw_newack = 1;
>> + }
>> +
>> if (TSN_lte(tsn, sack_ctsn)) {
>> /* RFC 2960 6.3.2 Retransmission Timer Rules
>> *
>> @@ -1411,27 +1432,6 @@ static void sctp_check_transmitted(struct
>> sctp_outq *q,
>> restart_timer = 1;
>> forward_progress = true;
>>
>> - if (!tchunk->tsn_gap_acked) {
>> - /*
>> - * SFR-CACC algorithm:
>> - * 2) If the SACK contains gap acks
>> - * and the flag CHANGEOVER_ACTIVE is
>> - * set the receiver of the SACK MUST
>> - * take the following action:
>> - *
>> - * B) For each TSN t being acked that
>> - * has not been acked in any SACK so
>> - * far, set cacc_saw_newack to 1 for
>> - * the destination that the TSN was
>> - * sent to.
>> - */
>> - if (transport &&
>> - sack->num_gap_ack_blocks &&
>> - q->asoc->peer.primary_path->cacc.
>> - changeover_active)
>> - transport->cacc.cacc_saw_newack
>> - = 1;
>> - }
>>
>> list_add_tail(&tchunk->transmitted_list,
>> &q->sacked);
>>
>

2013-10-15 14:34:12

by Vlad Yasevich

Subject: Re: [PATCH] net: sctp: fix a cacc_saw_newack missetting issue

On 10/15/2013 10:27 AM, Chang wrote:
>
> On 10/15/2013 04:11 PM, Vlad Yasevich wrote:
>> On 10/14/2013 09:33 AM, Chang Xiangzhong wrote:
>>> For for each TSN t being newly acked (Not only cumulatively,
>>> but also SELECTIVELY) cacc_saw_newack should be set to 1.
>>>
>>> Signed-off-by: Xiangzhong Chang <[email protected]>
>>> ---
>>> net/sctp/outqueue.c | 42 +++++++++++++++++++++---------------------
>>> 1 file changed, 21 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>>> index 94df758..d86032b 100644
>>> --- a/net/sctp/outqueue.c
>>> +++ b/net/sctp/outqueue.c
>>> @@ -1398,6 +1398,27 @@ static void sctp_check_transmitted(struct
>>> sctp_outq *q,
>>> forward_progress = true;
>>> }
>>>
>>> + if (!tchunk->tsn_gap_acked) {
>>
>> You can remove this test since the block just above already performs
>> it. Just fold this code into the block above.
>>
>> -vlad
>>
> Sorry, I'm not sure if I fully understand you. There are code blocks
> which checking the tchunk->tsn_gap_acked. In addition, they check other
> states as well.

The flow is:

if (sctp_acked(sack, tsn)) {
...
if (transport) {
....
}

if (!tchunk->tsn_gap_acked) {
....
}

if (TSN_lte(tsn, sack_ctsn)) {
....
/* SFR-CACC ...
}

Since you are moving this up, you can simply re-use
the if (!tchunk->tsn_gap_acked) immediately above.

>>> + /*
>>> + * SFR-CACC algorithm:
>>> + * 2) If the SACK contains gap acks
>>> + * and the flag CHANGEOVER_ACTIVE is
>>> + * set the receiver of the SACK MUST
>>> + * take the following action:
>>> + *
>>> + * B) For each TSN t being acked that
>>> + * has not been acked in any SACK so
>>> + * far, set cacc_saw_newack to 1 for
>>> + * the destination that the TSN was
>>> + * sent to.
>>> + */
>>> + if (transport &&
>>> + sack->num_gap_ack_blocks &&
>>> + q->asoc->peer.primary_path->cacc.
>>> + changeover_active)
>>> + transport->cacc.cacc_saw_newack = 1;
^^^^

Don't need that many spaces...

-vlad
>>> + }
>>> +
>>> if (TSN_lte(tsn, sack_ctsn)) {
>>> /* RFC 2960 6.3.2 Retransmission Timer Rules
>>> *
>>> @@ -1411,27 +1432,6 @@ static void sctp_check_transmitted(struct
>>> sctp_outq *q,
>>> restart_timer = 1;
>>> forward_progress = true;
>>>
>>> - if (!tchunk->tsn_gap_acked) {
>>> - /*
>>> - * SFR-CACC algorithm:
>>> - * 2) If the SACK contains gap acks
>>> - * and the flag CHANGEOVER_ACTIVE is
>>> - * set the receiver of the SACK MUST
>>> - * take the following action:
>>> - *
>>> - * B) For each TSN t being acked that
>>> - * has not been acked in any SACK so
>>> - * far, set cacc_saw_newack to 1 for
>>> - * the destination that the TSN was
>>> - * sent to.
>>> - */
>>> - if (transport &&
>>> - sack->num_gap_ack_blocks &&
>>> - q->asoc->peer.primary_path->cacc.
>>> - changeover_active)
>>> - transport->cacc.cacc_saw_newack
>>> - = 1;
>>> - }
>>>
>>> list_add_tail(&tchunk->transmitted_list,
>>> &q->sacked);
>>>
>>
>

2013-10-15 15:13:37

by Chang Xiangzhong

Subject: Re: [PATCH] net: sctp: fix a cacc_saw_newack missetting issue

Thanks, I've got it and will submit a new patch later.
On 10/15/2013 04:34 PM, Vlad Yasevich wrote:
> On 10/15/2013 10:27 AM, Chang wrote:
>>
>> On 10/15/2013 04:11 PM, Vlad Yasevich wrote:
>>> On 10/14/2013 09:33 AM, Chang Xiangzhong wrote:
>>>> For for each TSN t being newly acked (Not only cumulatively,
>>>> but also SELECTIVELY) cacc_saw_newack should be set to 1.
>>>>
>>>> Signed-off-by: Xiangzhong Chang <[email protected]>
>>>> ---
>>>> net/sctp/outqueue.c | 42
>>>> +++++++++++++++++++++---------------------
>>>> 1 file changed, 21 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>>>> index 94df758..d86032b 100644
>>>> --- a/net/sctp/outqueue.c
>>>> +++ b/net/sctp/outqueue.c
>>>> @@ -1398,6 +1398,27 @@ static void sctp_check_transmitted(struct
>>>> sctp_outq *q,
>>>> forward_progress = true;
>>>> }
>>>>
>>>> + if (!tchunk->tsn_gap_acked) {
>>>
>>> You can remove this test since the block just above already performs
>>> it. Just fold this code into the block above.
>>>
>>> -vlad
>>>
>> Sorry, I'm not sure if I fully understand you. There are code blocks
>> which checking the tchunk->tsn_gap_acked. In addition, they check other
>> states as well.
>
> The flow is:
>
> if (sctp_acked(sack, tsn)) {
> ...
> if (transport) {
> ....
> }
>
> if (!tchunk->tsn_gap_acked) {
> ....
> }
>
> if (TSN_lte(tsn, sack_ctsn)) {
> ....
> /* SFR-CACC ...
> }
>
> Since you are moving this up, you can simply re-use
> the if (!tchunk->tsn_gap_acked) immediately above.
>
>>>> + /*
>>>> + * SFR-CACC algorithm:
>>>> + * 2) If the SACK contains gap acks
>>>> + * and the flag CHANGEOVER_ACTIVE is
>>>> + * set the receiver of the SACK MUST
>>>> + * take the following action:
>>>> + *
>>>> + * B) For each TSN t being acked that
>>>> + * has not been acked in any SACK so
>>>> + * far, set cacc_saw_newack to 1 for
>>>> + * the destination that the TSN was
>>>> + * sent to.
>>>> + */
>>>> + if (transport &&
>>>> + sack->num_gap_ack_blocks &&
>>>> + q->asoc->peer.primary_path->cacc.
>>>> + changeover_active)
>>>> + transport->cacc.cacc_saw_newack = 1;
> ^^^^
>
> Don't need that many spaces...
>
> -vlad
>>>> + }
>>>> +
>>>> if (TSN_lte(tsn, sack_ctsn)) {
>>>> /* RFC 2960 6.3.2 Retransmission Timer Rules
>>>> *
>>>> @@ -1411,27 +1432,6 @@ static void sctp_check_transmitted(struct
>>>> sctp_outq *q,
>>>> restart_timer = 1;
>>>> forward_progress = true;
>>>>
>>>> - if (!tchunk->tsn_gap_acked) {
>>>> - /*
>>>> - * SFR-CACC algorithm:
>>>> - * 2) If the SACK contains gap acks
>>>> - * and the flag CHANGEOVER_ACTIVE is
>>>> - * set the receiver of the SACK MUST
>>>> - * take the following action:
>>>> - *
>>>> - * B) For each TSN t being acked that
>>>> - * has not been acked in any SACK so
>>>> - * far, set cacc_saw_newack to 1 for
>>>> - * the destination that the TSN was
>>>> - * sent to.
>>>> - */
>>>> - if (transport &&
>>>> - sack->num_gap_ack_blocks &&
>>>> - q->asoc->peer.primary_path->cacc.
>>>> - changeover_active)
>>>> - transport->cacc.cacc_saw_newack
>>>> - = 1;
>>>> - }
>>>>
>>>> list_add_tail(&tchunk->transmitted_list,
>>>> &q->sacked);
>>>>
>>>
>>
>