Re: sctp: move chunk from retransmit queue to abandoned list

[Vlad Yasevich <vladislav.yasevich@hp.com>]

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