* Re: sctp: move chunk from retransmit queue to abandoned list
2010-01-15 3:47 sctp: move chunk from retransmit queue to abandoned list Wei Yongjun
@ 2010-01-15 15:31 ` Vlad Yasevich
2010-01-18 4:51 ` Wei Yongjun
2010-01-19 19:50 ` Vlad Yasevich
2 siblings, 0 replies; 4+ messages in thread
From: Vlad Yasevich @ 2010-01-15 15:31 UTC (permalink / raw)
To: linux-sctp
Wei Yongjun wrote:
> If there is still data waiting to retransmit and remain in
> retransmit queue, while doing the next retransmit, if the
> chunk is abandoned, we should move it to abandoned list.
I think it might be better to move this to sctp_outq_flush_rtx().
Then we can reduce this to just the sctp_chunk_abandoned() check.
Remember, retransmit_mark is not executed that often. Also, if
we have move then one chunk on the retransmit queue, some part of
the queue may expire without us ever running retransmit_mark() again.
However, when we flush the queue, we should check for timed out chunks
just like when we flush the outqueue.
-vlad
>
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> ---
> net/sctp/outqueue.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 229690f..8092032 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -390,6 +390,18 @@ void sctp_retransmit_mark(struct sctp_outq *q,
> struct list_head *lchunk, *ltemp;
> struct sctp_chunk *chunk;
>
> + /* Walk through the retransmit queue */
> + list_for_each_safe(lchunk, ltemp, &q->retransmit) {
> + chunk = list_entry(lchunk, struct sctp_chunk,
> + transmitted_list);
> +
> + /* If the chunk is abandoned, move it to abandoned list. */
> + if (sctp_chunk_abandoned(chunk)) {
> + list_del_init(lchunk);
> + sctp_insert_list(&q->abandoned, lchunk);
> + }
> + }
> +
> /* Walk through the specified transmitted queue. */
> list_for_each_safe(lchunk, ltemp, &transport->transmitted) {
> chunk = list_entry(lchunk, struct sctp_chunk,
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: sctp: move chunk from retransmit queue to abandoned list
2010-01-15 3:47 sctp: move chunk from retransmit queue to abandoned list Wei Yongjun
2010-01-15 15:31 ` Vlad Yasevich
@ 2010-01-18 4:51 ` Wei Yongjun
2010-01-19 19:50 ` Vlad Yasevich
2 siblings, 0 replies; 4+ messages in thread
From: Wei Yongjun @ 2010-01-18 4:51 UTC (permalink / raw)
To: linux-sctp
Vlad Yasevich wrote:
> Wei Yongjun wrote:
>
>> If there is still data waiting to retransmit and remain in
>> retransmit queue, while doing the next retransmit, if the
>> chunk is abandoned, we should move it to abandoned list.
>>
>
> I think it might be better to move this to sctp_outq_flush_rtx().
> Then we can reduce this to just the sctp_chunk_abandoned() check.
>
Maybe both place need this check?
Check in sctp_retransmit_mark() can early indicating the retransmit chunk
is abandoned when we do retransmit and send the FORWARD TSN chunk.
And check in sctp_retransmit_mark() only miss the process when new DATA
send is coming and still have retransmit chunk.
If we received SACK, this expired chunks can also be remove to
abandoned list when we do sctp_outq_sack().
Check in sctp_outq_flush_rtx() aim to check only when new DATA is coming
and we send the retransmit chunk before the new DATA.
Just move this code to sctp_outq_flush_rtx() may have other issue. The
FORWARD TSN chunk is sent before we do sctp_outq_flush_rtx(), so the
retransmit chunk will remain in abandoned list until next rtx timeout or
received SACK. If all of the DATA chunks are abandoned, nothing will be
sent in this timeout.
> Remember, retransmit_mark is not executed that often. Also, if
> we have move then one chunk on the retransmit queue, some part of
> the queue may expire without us ever running retransmit_mark() again.
>
>
> However, when we flush the queue, we should check for timed out chunks
> just like when we flush the outqueue.
>
The outqueue does not need to send the FORWARD TSN chunk
because it does not hold an TSN. But retransmit queue need to
send the FORWARD TSN chunk.
[PATCH v2] sctp: move chunk from retransmit queue to abandoned list
If there is still data waiting to retransmit and remain in
retransmit queue, while doing the next retransmit, if the
chunk is abandoned, we should move it to abandoned list.
Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
net/sctp/outqueue.c | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 229690f..d36aea4 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -390,6 +390,18 @@ void sctp_retransmit_mark(struct sctp_outq *q,
struct list_head *lchunk, *ltemp;
struct sctp_chunk *chunk;
+ /* Walk through the retransmit queue */
+ list_for_each_safe(lchunk, ltemp, &q->retransmit) {
+ chunk = list_entry(lchunk, struct sctp_chunk,
+ transmitted_list);
+
+ /* If the chunk is abandoned, move it to abandoned list. */
+ if (sctp_chunk_abandoned(chunk)) {
+ list_del_init(lchunk);
+ sctp_insert_list(&q->abandoned, lchunk);
+ }
+ }
+
/* Walk through the specified transmitted queue. */
list_for_each_safe(lchunk, ltemp, &transport->transmitted) {
chunk = list_entry(lchunk, struct sctp_chunk,
@@ -578,6 +590,12 @@ static int sctp_outq_flush_rtx(struct sctp_outq *q, struct sctp_packet *pkt,
* try to send as much as possible.
*/
list_for_each_entry_safe(chunk, chunk1, lqueue, transmitted_list) {
+ /* If the chunk is abandoned, move it to abandoned list. */
+ if (!rtx_timeout && sctp_chunk_abandoned(chunk)) {
+ list_del_init(&chunk->transmitted_list);
+ sctp_insert_list(&q->abandoned,
+ &chunk->transmitted_list);
+ }
/* Make sure that Gap Acked TSNs are not retransmitted. A
* simple approach is just to move such TSNs out of the
--
1.6.2.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: sctp: move chunk from retransmit queue to abandoned list
2010-01-15 3:47 sctp: move chunk from retransmit queue to abandoned list Wei Yongjun
2010-01-15 15:31 ` Vlad Yasevich
2010-01-18 4:51 ` Wei Yongjun
@ 2010-01-19 19:50 ` Vlad Yasevich
2 siblings, 0 replies; 4+ messages in thread
From: Vlad Yasevich @ 2010-01-19 19:50 UTC (permalink / raw)
To: linux-sctp
Wei Yongjun wrote:
> Vlad Yasevich wrote:
>> Wei Yongjun wrote:
>>
>>> If there is still data waiting to retransmit and remain in
>>> retransmit queue, while doing the next retransmit, if the
>>> chunk is abandoned, we should move it to abandoned list.
>>>
>> I think it might be better to move this to sctp_outq_flush_rtx().
>> Then we can reduce this to just the sctp_chunk_abandoned() check.
>>
> Maybe both place need this check?
>
> Check in sctp_retransmit_mark() can early indicating the retransmit chunk
> is abandoned when we do retransmit and send the FORWARD TSN chunk.
> And check in sctp_retransmit_mark() only miss the process when new DATA
> send is coming and still have retransmit chunk.
>
> If we received SACK, this expired chunks can also be remove to
> abandoned list when we do sctp_outq_sack().
>
> Check in sctp_outq_flush_rtx() aim to check only when new DATA is coming
> and we send the retransmit chunk before the new DATA.
Not only. This will happen on SACKs and timeouts, but I see what you are
trying to catch.
If we have just timeout retransmits and we have more then 1 MTU worth to
retransmit, then it's possible for the retransmit list to hold abandoned
data.
I guess what I don't like is that the code you added has nothing to do
with retransmission. I am OK with the code, but I think it deserves its
own function. It will make it clearer.
-vlad
>
> Just move this code to sctp_outq_flush_rtx() may have other issue. The
> FORWARD TSN chunk is sent before we do sctp_outq_flush_rtx(), so the
> retransmit chunk will remain in abandoned list until next rtx timeout or
> received SACK. If all of the DATA chunks are abandoned, nothing will be
> sent in this timeout.
>
>
>> Remember, retransmit_mark is not executed that often. Also, if
>> we have move then one chunk on the retransmit queue, some part of
>> the queue may expire without us ever running retransmit_mark() again.
>>
>>
>> However, when we flush the queue, we should check for timed out chunks
>> just like when we flush the outqueue.
>>
>
> The outqueue does not need to send the FORWARD TSN chunk
> because it does not hold an TSN. But retransmit queue need to
> send the FORWARD TSN chunk.
>
>
> [PATCH v2] sctp: move chunk from retransmit queue to abandoned list
>
> If there is still data waiting to retransmit and remain in
> retransmit queue, while doing the next retransmit, if the
> chunk is abandoned, we should move it to abandoned list.
>
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> ---
> net/sctp/outqueue.c | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 229690f..d36aea4 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -390,6 +390,18 @@ void sctp_retransmit_mark(struct sctp_outq *q,
> struct list_head *lchunk, *ltemp;
> struct sctp_chunk *chunk;
>
> + /* Walk through the retransmit queue */
> + list_for_each_safe(lchunk, ltemp, &q->retransmit) {
> + chunk = list_entry(lchunk, struct sctp_chunk,
> + transmitted_list);
> +
> + /* If the chunk is abandoned, move it to abandoned list. */
> + if (sctp_chunk_abandoned(chunk)) {
> + list_del_init(lchunk);
> + sctp_insert_list(&q->abandoned, lchunk);
> + }
> + }
> +
> /* Walk through the specified transmitted queue. */
> list_for_each_safe(lchunk, ltemp, &transport->transmitted) {
> chunk = list_entry(lchunk, struct sctp_chunk,
> @@ -578,6 +590,12 @@ static int sctp_outq_flush_rtx(struct sctp_outq *q, struct sctp_packet *pkt,
> * try to send as much as possible.
> */
> list_for_each_entry_safe(chunk, chunk1, lqueue, transmitted_list) {
> + /* If the chunk is abandoned, move it to abandoned list. */
> + if (!rtx_timeout && sctp_chunk_abandoned(chunk)) {
> + list_del_init(&chunk->transmitted_list);
> + sctp_insert_list(&q->abandoned,
> + &chunk->transmitted_list);
> + }
>
> /* Make sure that Gap Acked TSNs are not retransmitted. A
> * simple approach is just to move such TSNs out of the
^ permalink raw reply [flat|nested] 4+ messages in thread