* [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
@ 2013-11-22 7:49 ` Chang Xiangzhong
0 siblings, 0 replies; 16+ messages in thread
From: Chang Xiangzhong @ 2013-11-22 7:49 UTC (permalink / raw)
To: vyasevich, nhorman, davem
Cc: linux-sctp, netdev, linux-kernel, Chang Xiangzhong
tsn_gap_acked is an important state flag in chunk, which indicates if the
chunk has been acked in gap reports before. SFR-CACC algorithm depends on this
variable. So set this at the end of each iteration, otherwise the SFR-CACC
algorithm would never be toggled.
Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
---
net/sctp/outqueue.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 1b494fa..bff828c 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -1396,7 +1396,6 @@ static void sctp_check_transmitted(struct sctp_outq *q,
* while DATA was outstanding).
*/
if (!tchunk->tsn_gap_acked) {
- tchunk->tsn_gap_acked = 1;
if (TSN_lt(*highest_new_tsn_in_sack, tsn))
*highest_new_tsn_in_sack = tsn;
bytes_acked += sctp_data_size(tchunk);
@@ -1460,6 +1459,8 @@ static void sctp_check_transmitted(struct sctp_outq *q,
*/
list_add_tail(lchunk, &tlist);
}
+
+ tchunk->tsn_gap_acked = 1;
} else {
if (tchunk->tsn_gap_acked) {
pr_debug("%s: receiver reneged on data TSN:0x%x\n",
--
1.8.4.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
2013-11-22 7:49 ` Chang Xiangzhong
@ 2013-11-22 12:10 ` Neil Horman
-1 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2013-11-22 12:10 UTC (permalink / raw)
To: Chang Xiangzhong; +Cc: vyasevich, davem, linux-sctp, netdev, linux-kernel
On Fri, Nov 22, 2013 at 08:49:49AM +0100, Chang Xiangzhong wrote:
> tsn_gap_acked is an important state flag in chunk, which indicates if the
> chunk has been acked in gap reports before. SFR-CACC algorithm depends on this
> variable. So set this at the end of each iteration, otherwise the SFR-CACC
> algorithm would never be toggled.
>
> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
> ---
> net/sctp/outqueue.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 1b494fa..bff828c 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -1396,7 +1396,6 @@ static void sctp_check_transmitted(struct sctp_outq *q,
> * while DATA was outstanding).
> */
> if (!tchunk->tsn_gap_acked) {
> - tchunk->tsn_gap_acked = 1;
> if (TSN_lt(*highest_new_tsn_in_sack, tsn))
> *highest_new_tsn_in_sack = tsn;
> bytes_acked += sctp_data_size(tchunk);
> @@ -1460,6 +1459,8 @@ static void sctp_check_transmitted(struct sctp_outq *q,
> */
> list_add_tail(lchunk, &tlist);
> }
> +
> + tchunk->tsn_gap_acked = 1;
> } else {
> if (tchunk->tsn_gap_acked) {
> pr_debug("%s: receiver reneged on data TSN:0x%x\n",
> --
> 1.8.4.3
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
@ 2013-11-22 12:10 ` Neil Horman
0 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2013-11-22 12:10 UTC (permalink / raw)
To: Chang Xiangzhong; +Cc: vyasevich, davem, linux-sctp, netdev, linux-kernel
On Fri, Nov 22, 2013 at 08:49:49AM +0100, Chang Xiangzhong wrote:
> tsn_gap_acked is an important state flag in chunk, which indicates if the
> chunk has been acked in gap reports before. SFR-CACC algorithm depends on this
> variable. So set this at the end of each iteration, otherwise the SFR-CACC
> algorithm would never be toggled.
>
> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
> ---
> net/sctp/outqueue.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 1b494fa..bff828c 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -1396,7 +1396,6 @@ static void sctp_check_transmitted(struct sctp_outq *q,
> * while DATA was outstanding).
> */
> if (!tchunk->tsn_gap_acked) {
> - tchunk->tsn_gap_acked = 1;
> if (TSN_lt(*highest_new_tsn_in_sack, tsn))
> *highest_new_tsn_in_sack = tsn;
> bytes_acked += sctp_data_size(tchunk);
> @@ -1460,6 +1459,8 @@ static void sctp_check_transmitted(struct sctp_outq *q,
> */
> list_add_tail(lchunk, &tlist);
> }
> +
> + tchunk->tsn_gap_acked = 1;
> } else {
> if (tchunk->tsn_gap_acked) {
> pr_debug("%s: receiver reneged on data TSN:0x%x\n",
> --
> 1.8.4.3
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
2013-11-22 7:49 ` Chang Xiangzhong
@ 2013-11-22 14:27 ` Vlad Yasevich
-1 siblings, 0 replies; 16+ messages in thread
From: Vlad Yasevich @ 2013-11-22 14:27 UTC (permalink / raw)
To: Chang Xiangzhong, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
On 11/22/2013 02:49 AM, Chang Xiangzhong wrote:
> tsn_gap_acked is an important state flag in chunk, which indicates if the
> chunk has been acked in gap reports before.
Actually, this bit indicates simply that the chunk has been acked. It
doesn't state whether it's been acked in a gap report or via cumulative tsn.
> SFR-CACC algorithm depends on this
> variable. So set this at the end of each iteration, otherwise the SFR-CACC
> algorithm would never be toggled.
>
> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
> ---
> net/sctp/outqueue.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 1b494fa..bff828c 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -1396,7 +1396,6 @@ static void sctp_check_transmitted(struct sctp_outq *q,
> * while DATA was outstanding).
> */
> if (!tchunk->tsn_gap_acked) {
> - tchunk->tsn_gap_acked = 1;
> if (TSN_lt(*highest_new_tsn_in_sack, tsn))
> *highest_new_tsn_in_sack = tsn;
> bytes_acked += sctp_data_size(tchunk);
> @@ -1460,6 +1459,8 @@ static void sctp_check_transmitted(struct sctp_outq *q,
> */
> list_add_tail(lchunk, &tlist);
> }
> +
> + tchunk->tsn_gap_acked = 1;
The current code will set the state if it hasn't been set yet. Why is
this needed?
Now there is an issue with tracking highest_new_tsn_in_sack. The spec
is a little vague on this, but the highest_new_tsn_in_sack only supposed
to track tsns that have not been resent. If a tsn has been reneged, and
then sent again, it is not considered 'new' thus should not count
toward highest_new_tsn_in_sack. We currently do not track this right.
-vlad
-vlad
> } else {
> if (tchunk->tsn_gap_acked) {
> pr_debug("%s: receiver reneged on data TSN:0x%x\n",
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
@ 2013-11-22 14:27 ` Vlad Yasevich
0 siblings, 0 replies; 16+ messages in thread
From: Vlad Yasevich @ 2013-11-22 14:27 UTC (permalink / raw)
To: Chang Xiangzhong, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
On 11/22/2013 02:49 AM, Chang Xiangzhong wrote:
> tsn_gap_acked is an important state flag in chunk, which indicates if the
> chunk has been acked in gap reports before.
Actually, this bit indicates simply that the chunk has been acked. It
doesn't state whether it's been acked in a gap report or via cumulative tsn.
> SFR-CACC algorithm depends on this
> variable. So set this at the end of each iteration, otherwise the SFR-CACC
> algorithm would never be toggled.
>
> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
> ---
> net/sctp/outqueue.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 1b494fa..bff828c 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -1396,7 +1396,6 @@ static void sctp_check_transmitted(struct sctp_outq *q,
> * while DATA was outstanding).
> */
> if (!tchunk->tsn_gap_acked) {
> - tchunk->tsn_gap_acked = 1;
> if (TSN_lt(*highest_new_tsn_in_sack, tsn))
> *highest_new_tsn_in_sack = tsn;
> bytes_acked += sctp_data_size(tchunk);
> @@ -1460,6 +1459,8 @@ static void sctp_check_transmitted(struct sctp_outq *q,
> */
> list_add_tail(lchunk, &tlist);
> }
> +
> + tchunk->tsn_gap_acked = 1;
The current code will set the state if it hasn't been set yet. Why is
this needed?
Now there is an issue with tracking highest_new_tsn_in_sack. The spec
is a little vague on this, but the highest_new_tsn_in_sack only supposed
to track tsns that have not been resent. If a tsn has been reneged, and
then sent again, it is not considered 'new' thus should not count
toward highest_new_tsn_in_sack. We currently do not track this right.
-vlad
-vlad
> } else {
> if (tchunk->tsn_gap_acked) {
> pr_debug("%s: receiver reneged on data TSN:0x%x\n",
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
2013-11-22 14:27 ` Vlad Yasevich
@ 2013-11-22 19:24 ` Chang
-1 siblings, 0 replies; 16+ messages in thread
From: Chang @ 2013-11-22 19:24 UTC (permalink / raw)
To: Vlad Yasevich, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
On 11/22/2013 03:27 PM, Vlad Yasevich wrote:
> On 11/22/2013 02:49 AM, Chang Xiangzhong wrote:
>> tsn_gap_acked is an important state flag in chunk, which indicates if the
>> chunk has been acked in gap reports before.
> Actually, this bit indicates simply that the chunk has been acked. It
> doesn't state whether it's been acked in a gap report or via cumulative tsn.
Thanks for pointing this out. Sorry for not having made that clear.
>
>> SFR-CACC algorithm depends on this
>> variable. So set this at the end of each iteration, otherwise the SFR-CACC
>> algorithm would never be toggled.
>>
>> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
>> ---
>> net/sctp/outqueue.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>> index 1b494fa..bff828c 100644
>> --- a/net/sctp/outqueue.c
>> +++ b/net/sctp/outqueue.c
>> @@ -1396,7 +1396,6 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>> * while DATA was outstanding).
>> */
>> if (!tchunk->tsn_gap_acked) {
>> - tchunk->tsn_gap_acked = 1;
>> if (TSN_lt(*highest_new_tsn_in_sack, tsn))
>> *highest_new_tsn_in_sack = tsn;
>> bytes_acked += sctp_data_size(tchunk);
>> @@ -1460,6 +1459,8 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>> */
>> list_add_tail(lchunk, &tlist);
>> }
>> +
>> + tchunk->tsn_gap_acked = 1;
> The current code will set the state if it hasn't been set yet. Why is
> this needed?
Because in line 1420 ~ 1440 The SFR-CACC algorithms use tsn_gap_acked.
That "if block" would never be triggered because the varaiable's been
set. That's why I move the "state changing sentence" to the end of the
iteration.
> Now there is an issue with tracking highest_new_tsn_in_sack. The spec
> is a little vague on this, but the highest_new_tsn_in_sack only supposed
> to track tsns that have not been resent. If a tsn has been reneged, and
> then sent again, it is not considered 'new' thus should not count
> toward highest_new_tsn_in_sack. We currently do not track this right.
I'll try to figure this out.
> -vlad
>
> -vlad
>
>> } else {
>> if (tchunk->tsn_gap_acked) {
>> pr_debug("%s: receiver reneged on data TSN:0x%x\n",
>>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
@ 2013-11-22 19:24 ` Chang
0 siblings, 0 replies; 16+ messages in thread
From: Chang @ 2013-11-22 19:24 UTC (permalink / raw)
To: Vlad Yasevich, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
On 11/22/2013 03:27 PM, Vlad Yasevich wrote:
> On 11/22/2013 02:49 AM, Chang Xiangzhong wrote:
>> tsn_gap_acked is an important state flag in chunk, which indicates if the
>> chunk has been acked in gap reports before.
> Actually, this bit indicates simply that the chunk has been acked. It
> doesn't state whether it's been acked in a gap report or via cumulative tsn.
Thanks for pointing this out. Sorry for not having made that clear.
>
>> SFR-CACC algorithm depends on this
>> variable. So set this at the end of each iteration, otherwise the SFR-CACC
>> algorithm would never be toggled.
>>
>> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
>> ---
>> net/sctp/outqueue.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>> index 1b494fa..bff828c 100644
>> --- a/net/sctp/outqueue.c
>> +++ b/net/sctp/outqueue.c
>> @@ -1396,7 +1396,6 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>> * while DATA was outstanding).
>> */
>> if (!tchunk->tsn_gap_acked) {
>> - tchunk->tsn_gap_acked = 1;
>> if (TSN_lt(*highest_new_tsn_in_sack, tsn))
>> *highest_new_tsn_in_sack = tsn;
>> bytes_acked += sctp_data_size(tchunk);
>> @@ -1460,6 +1459,8 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>> */
>> list_add_tail(lchunk, &tlist);
>> }
>> +
>> + tchunk->tsn_gap_acked = 1;
> The current code will set the state if it hasn't been set yet. Why is
> this needed?
Because in line 1420 ~ 1440 The SFR-CACC algorithms use tsn_gap_acked.
That "if block" would never be triggered because the varaiable's been
set. That's why I move the "state changing sentence" to the end of the
iteration.
> Now there is an issue with tracking highest_new_tsn_in_sack. The spec
> is a little vague on this, but the highest_new_tsn_in_sack only supposed
> to track tsns that have not been resent. If a tsn has been reneged, and
> then sent again, it is not considered 'new' thus should not count
> toward highest_new_tsn_in_sack. We currently do not track this right.
I'll try to figure this out.
> -vlad
>
> -vlad
>
>> } else {
>> if (tchunk->tsn_gap_acked) {
>> pr_debug("%s: receiver reneged on data TSN:0x%x\n",
>>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
2013-11-22 19:24 ` Chang
@ 2013-11-22 22:48 ` Vlad Yasevich
-1 siblings, 0 replies; 16+ messages in thread
From: Vlad Yasevich @ 2013-11-22 22:48 UTC (permalink / raw)
To: Chang, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
On 11/22/2013 02:24 PM, Chang wrote:
>
> On 11/22/2013 03:27 PM, Vlad Yasevich wrote:
>> On 11/22/2013 02:49 AM, Chang Xiangzhong wrote:
>>> tsn_gap_acked is an important state flag in chunk, which indicates if
>>> the
>>> chunk has been acked in gap reports before.
>> Actually, this bit indicates simply that the chunk has been acked. It
>> doesn't state whether it's been acked in a gap report or via
>> cumulative tsn.
> Thanks for pointing this out. Sorry for not having made that clear.
>>
>>> SFR-CACC algorithm depends on this
>>> variable. So set this at the end of each iteration, otherwise the
>>> SFR-CACC
>>> algorithm would never be toggled.
>>>
>>> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
>>> ---
>>> net/sctp/outqueue.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>>> index 1b494fa..bff828c 100644
>>> --- a/net/sctp/outqueue.c
>>> +++ b/net/sctp/outqueue.c
>>> @@ -1396,7 +1396,6 @@ static void sctp_check_transmitted(struct
>>> sctp_outq *q,
>>> * while DATA was outstanding).
>>> */
>>> if (!tchunk->tsn_gap_acked) {
>>> - tchunk->tsn_gap_acked = 1;
>>> if (TSN_lt(*highest_new_tsn_in_sack, tsn))
>>> *highest_new_tsn_in_sack = tsn;
>>> bytes_acked += sctp_data_size(tchunk);
>>> @@ -1460,6 +1459,8 @@ static void sctp_check_transmitted(struct
>>> sctp_outq *q,
>>> */
>>> list_add_tail(lchunk, &tlist);
>>> }
>>> +
>>> + tchunk->tsn_gap_acked = 1;
>> The current code will set the state if it hasn't been set yet. Why is
>> this needed?
> Because in line 1420 ~ 1440 The SFR-CACC algorithms use tsn_gap_acked.
> That "if block" would never be triggered because the varaiable's been
> set. That's why I move the "state changing sentence" to the end of the
> iteration.
>> Now there is an issue with tracking highest_new_tsn_in_sack. The spec
>> is a little vague on this, but the highest_new_tsn_in_sack only supposed
>> to track tsns that have not been resent. If a tsn has been reneged, and
>> then sent again, it is not considered 'new' thus should not count
>> toward highest_new_tsn_in_sack. We currently do not track this right.
> I'll try to figure this out.
So the solution that's been proposed before is to move the CACC block up
and merge it with the !tsn_gap_acked block. This requires additional
change though to only mark highest_new_tsn if the chunk has not been
retransmitted (which you've added in a prior patch).
-vlad
>> -vlad
>>
>> -vlad
>>
>>> } else {
>>> if (tchunk->tsn_gap_acked) {
>>> pr_debug("%s: receiver reneged on data TSN:0x%x\n",
>>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
@ 2013-11-22 22:48 ` Vlad Yasevich
0 siblings, 0 replies; 16+ messages in thread
From: Vlad Yasevich @ 2013-11-22 22:48 UTC (permalink / raw)
To: Chang, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
On 11/22/2013 02:24 PM, Chang wrote:
>
> On 11/22/2013 03:27 PM, Vlad Yasevich wrote:
>> On 11/22/2013 02:49 AM, Chang Xiangzhong wrote:
>>> tsn_gap_acked is an important state flag in chunk, which indicates if
>>> the
>>> chunk has been acked in gap reports before.
>> Actually, this bit indicates simply that the chunk has been acked. It
>> doesn't state whether it's been acked in a gap report or via
>> cumulative tsn.
> Thanks for pointing this out. Sorry for not having made that clear.
>>
>>> SFR-CACC algorithm depends on this
>>> variable. So set this at the end of each iteration, otherwise the
>>> SFR-CACC
>>> algorithm would never be toggled.
>>>
>>> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
>>> ---
>>> net/sctp/outqueue.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>>> index 1b494fa..bff828c 100644
>>> --- a/net/sctp/outqueue.c
>>> +++ b/net/sctp/outqueue.c
>>> @@ -1396,7 +1396,6 @@ static void sctp_check_transmitted(struct
>>> sctp_outq *q,
>>> * while DATA was outstanding).
>>> */
>>> if (!tchunk->tsn_gap_acked) {
>>> - tchunk->tsn_gap_acked = 1;
>>> if (TSN_lt(*highest_new_tsn_in_sack, tsn))
>>> *highest_new_tsn_in_sack = tsn;
>>> bytes_acked += sctp_data_size(tchunk);
>>> @@ -1460,6 +1459,8 @@ static void sctp_check_transmitted(struct
>>> sctp_outq *q,
>>> */
>>> list_add_tail(lchunk, &tlist);
>>> }
>>> +
>>> + tchunk->tsn_gap_acked = 1;
>> The current code will set the state if it hasn't been set yet. Why is
>> this needed?
> Because in line 1420 ~ 1440 The SFR-CACC algorithms use tsn_gap_acked.
> That "if block" would never be triggered because the varaiable's been
> set. That's why I move the "state changing sentence" to the end of the
> iteration.
>> Now there is an issue with tracking highest_new_tsn_in_sack. The spec
>> is a little vague on this, but the highest_new_tsn_in_sack only supposed
>> to track tsns that have not been resent. If a tsn has been reneged, and
>> then sent again, it is not considered 'new' thus should not count
>> toward highest_new_tsn_in_sack. We currently do not track this right.
> I'll try to figure this out.
So the solution that's been proposed before is to move the CACC block up
and merge it with the !tsn_gap_acked block. This requires additional
change though to only mark highest_new_tsn if the chunk has not been
retransmitted (which you've added in a prior patch).
-vlad
>> -vlad
>>
>> -vlad
>>
>>> } else {
>>> if (tchunk->tsn_gap_acked) {
>>> pr_debug("%s: receiver reneged on data TSN:0x%x\n",
>>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
2013-11-22 22:48 ` Vlad Yasevich
@ 2013-11-23 8:22 ` Chang
-1 siblings, 0 replies; 16+ messages in thread
From: Chang @ 2013-11-23 8:22 UTC (permalink / raw)
To: Vlad Yasevich, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
On 11/22/2013 11:48 PM, Vlad Yasevich wrote:
> On 11/22/2013 02:24 PM, Chang wrote:
>> On 11/22/2013 03:27 PM, Vlad Yasevich wrote:
>>> On 11/22/2013 02:49 AM, Chang Xiangzhong wrote:
>>>> tsn_gap_acked is an important state flag in chunk, which indicates if
>>>> the
>>>> chunk has been acked in gap reports before.
>>> Actually, this bit indicates simply that the chunk has been acked. It
>>> doesn't state whether it's been acked in a gap report or via
>>> cumulative tsn.
>> Thanks for pointing this out. Sorry for not having made that clear.
>>>> SFR-CACC algorithm depends on this
>>>> variable. So set this at the end of each iteration, otherwise the
>>>> SFR-CACC
>>>> algorithm would never be toggled.
>>>>
>>>> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
>>>> ---
>>>> net/sctp/outqueue.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>>>> index 1b494fa..bff828c 100644
>>>> --- a/net/sctp/outqueue.c
>>>> +++ b/net/sctp/outqueue.c
>>>> @@ -1396,7 +1396,6 @@ static void sctp_check_transmitted(struct
>>>> sctp_outq *q,
>>>> * while DATA was outstanding).
>>>> */
>>>> if (!tchunk->tsn_gap_acked) {
>>>> - tchunk->tsn_gap_acked = 1;
>>>> if (TSN_lt(*highest_new_tsn_in_sack, tsn))
>>>> *highest_new_tsn_in_sack = tsn;
>>>> bytes_acked += sctp_data_size(tchunk);
>>>> @@ -1460,6 +1459,8 @@ static void sctp_check_transmitted(struct
>>>> sctp_outq *q,
>>>> */
>>>> list_add_tail(lchunk, &tlist);
>>>> }
>>>> +
>>>> + tchunk->tsn_gap_acked = 1;
>>> The current code will set the state if it hasn't been set yet. Why is
>>> this needed?
>> Because in line 1420 ~ 1440 The SFR-CACC algorithms use tsn_gap_acked.
>> That "if block" would never be triggered because the varaiable's been
>> set. That's why I move the "state changing sentence" to the end of the
>> iteration.
>>> Now there is an issue with tracking highest_new_tsn_in_sack. The spec
>>> is a little vague on this, but the highest_new_tsn_in_sack only supposed
>>> to track tsns that have not been resent. If a tsn has been reneged, and
>>> then sent again, it is not considered 'new' thus should not count
>>> toward highest_new_tsn_in_sack. We currently do not track this right.
>> I'll try to figure this out.
> So the solution that's been proposed before is to move the CACC block up
> and merge it with the !tsn_gap_acked block. This requires additional
> change though to only mark highest_new_tsn if the chunk has not been
> retransmitted (which you've added in a prior patch).
>
> -vlad
Yeah, I've mixed that up with a prior patch. How was the previous patch?
Was it approved?
-chang
>>> -vlad
>>>
>>> -vlad
>>>
>>>> } else {
>>>> if (tchunk->tsn_gap_acked) {
>>>> pr_debug("%s: receiver reneged on data TSN:0x%x\n",
>>>>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
@ 2013-11-23 8:22 ` Chang
0 siblings, 0 replies; 16+ messages in thread
From: Chang @ 2013-11-23 8:22 UTC (permalink / raw)
To: Vlad Yasevich, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
On 11/22/2013 11:48 PM, Vlad Yasevich wrote:
> On 11/22/2013 02:24 PM, Chang wrote:
>> On 11/22/2013 03:27 PM, Vlad Yasevich wrote:
>>> On 11/22/2013 02:49 AM, Chang Xiangzhong wrote:
>>>> tsn_gap_acked is an important state flag in chunk, which indicates if
>>>> the
>>>> chunk has been acked in gap reports before.
>>> Actually, this bit indicates simply that the chunk has been acked. It
>>> doesn't state whether it's been acked in a gap report or via
>>> cumulative tsn.
>> Thanks for pointing this out. Sorry for not having made that clear.
>>>> SFR-CACC algorithm depends on this
>>>> variable. So set this at the end of each iteration, otherwise the
>>>> SFR-CACC
>>>> algorithm would never be toggled.
>>>>
>>>> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
>>>> ---
>>>> net/sctp/outqueue.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>>>> index 1b494fa..bff828c 100644
>>>> --- a/net/sctp/outqueue.c
>>>> +++ b/net/sctp/outqueue.c
>>>> @@ -1396,7 +1396,6 @@ static void sctp_check_transmitted(struct
>>>> sctp_outq *q,
>>>> * while DATA was outstanding).
>>>> */
>>>> if (!tchunk->tsn_gap_acked) {
>>>> - tchunk->tsn_gap_acked = 1;
>>>> if (TSN_lt(*highest_new_tsn_in_sack, tsn))
>>>> *highest_new_tsn_in_sack = tsn;
>>>> bytes_acked += sctp_data_size(tchunk);
>>>> @@ -1460,6 +1459,8 @@ static void sctp_check_transmitted(struct
>>>> sctp_outq *q,
>>>> */
>>>> list_add_tail(lchunk, &tlist);
>>>> }
>>>> +
>>>> + tchunk->tsn_gap_acked = 1;
>>> The current code will set the state if it hasn't been set yet. Why is
>>> this needed?
>> Because in line 1420 ~ 1440 The SFR-CACC algorithms use tsn_gap_acked.
>> That "if block" would never be triggered because the varaiable's been
>> set. That's why I move the "state changing sentence" to the end of the
>> iteration.
>>> Now there is an issue with tracking highest_new_tsn_in_sack. The spec
>>> is a little vague on this, but the highest_new_tsn_in_sack only supposed
>>> to track tsns that have not been resent. If a tsn has been reneged, and
>>> then sent again, it is not considered 'new' thus should not count
>>> toward highest_new_tsn_in_sack. We currently do not track this right.
>> I'll try to figure this out.
> So the solution that's been proposed before is to move the CACC block up
> and merge it with the !tsn_gap_acked block. This requires additional
> change though to only mark highest_new_tsn if the chunk has not been
> retransmitted (which you've added in a prior patch).
>
> -vlad
Yeah, I've mixed that up with a prior patch. How was the previous patch?
Was it approved?
-chang
>>> -vlad
>>>
>>> -vlad
>>>
>>>> } else {
>>>> if (tchunk->tsn_gap_acked) {
>>>> pr_debug("%s: receiver reneged on data TSN:0x%x\n",
>>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
2013-11-22 22:48 ` Vlad Yasevich
@ 2013-11-23 11:14 ` Chang
-1 siblings, 0 replies; 16+ messages in thread
From: Chang @ 2013-11-23 11:14 UTC (permalink / raw)
To: Vlad Yasevich, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
Hi,
Could you please why a **reneged** newly acked TSN doesn't qualify the
highest_new_tsn? What's the wrongs of doing that?
I've been thinking a few scenarios, but I couldn't figure out what's
wrong with that.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
@ 2013-11-23 11:14 ` Chang
0 siblings, 0 replies; 16+ messages in thread
From: Chang @ 2013-11-23 11:14 UTC (permalink / raw)
To: Vlad Yasevich, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
Hi,
Could you please why a **reneged** newly acked TSN doesn't qualify the
highest_new_tsn? What's the wrongs of doing that?
I've been thinking a few scenarios, but I couldn't figure out what's
wrong with that.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
2013-11-23 11:14 ` Chang
@ 2013-11-23 19:37 ` Vlad Yasevich
-1 siblings, 0 replies; 16+ messages in thread
From: Vlad Yasevich @ 2013-11-23 19:37 UTC (permalink / raw)
To: Chang, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
On 11/23/2013 06:14 AM, Chang wrote:
> Hi,
> Could you please why a **reneged** newly acked TSN doesn't qualify the
> highest_new_tsn? What's the wrongs of doing that?
>
> I've been thinking a few scenarios, but I couldn't figure out what's
> wrong with that.
>
The spec is a bit conflicting on this topic. Here is what it says
Section 7.2.4
Miss indications SHOULD follow the HTNA (Highest TSN Newly
Acknowledged) algorithm. For each incoming SACK, miss indications
are incremented only for missing TSNs prior to the highest TSN newly
acknowledged in the SACK. A newly acknowledged DATA chunk is one not
previously acknowledged in a SACK.
But section 6.2.1 says:
iii) If the SACK is missing a TSN that was previously acknowledged
via a Gap Ack Block (e.g., the data receiver reneged on the
data), then consider the corresponding DATA that might be
possibly missing: Count one miss indication towards Fast
Retransmit as described in Section 7.2.4, and if no
retransmit timer is running for the destination address to
which the DATA chunk was originally transmitted, then T3-rtx
is started for that destination address.
So, the question becomes does the reneged tsn update HTNA counter? It
has been acked by a previous SACK, but 6.2.1 says to treat as missing.
The more I look at this the more I think we should continue doing what
we are doing which is following section 6.2.1.
-vlad
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] net: sctp: set chunk->tsn_gap_acked at the end of cycle
@ 2013-11-23 19:37 ` Vlad Yasevich
0 siblings, 0 replies; 16+ messages in thread
From: Vlad Yasevich @ 2013-11-23 19:37 UTC (permalink / raw)
To: Chang, nhorman, davem; +Cc: linux-sctp, netdev, linux-kernel
On 11/23/2013 06:14 AM, Chang wrote:
> Hi,
> Could you please why a **reneged** newly acked TSN doesn't qualify the
> highest_new_tsn? What's the wrongs of doing that?
>
> I've been thinking a few scenarios, but I couldn't figure out what's
> wrong with that.
>
The spec is a bit conflicting on this topic. Here is what it says
Section 7.2.4
Miss indications SHOULD follow the HTNA (Highest TSN Newly
Acknowledged) algorithm. For each incoming SACK, miss indications
are incremented only for missing TSNs prior to the highest TSN newly
acknowledged in the SACK. A newly acknowledged DATA chunk is one not
previously acknowledged in a SACK.
But section 6.2.1 says:
iii) If the SACK is missing a TSN that was previously acknowledged
via a Gap Ack Block (e.g., the data receiver reneged on the
data), then consider the corresponding DATA that might be
possibly missing: Count one miss indication towards Fast
Retransmit as described in Section 7.2.4, and if no
retransmit timer is running for the destination address to
which the DATA chunk was originally transmitted, then T3-rtx
is started for that destination address.
So, the question becomes does the reneged tsn update HTNA counter? It
has been acked by a previous SACK, but 6.2.1 says to treat as missing.
The more I look at this the more I think we should continue doing what
we are doing which is following section 6.2.1.
-vlad
^ permalink raw reply [flat|nested] 16+ messages in thread