[PATCH] sctp: Try to encourage SACK bundling with DATA.

[PATCH] sctp: Try to encourage SACK bundling with DATA.

[Vlad Yasevich]

If the association has a SACK timer pending and now DATA queued
to be send, we'll try to bundle the SACK with the next application send.
As such, try encourage bundling by accounting for SACK in the size
of the first chunk fragment.

Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
---
 net/sctp/chunk.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index 1748ef9..05b532e 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -196,6 +196,15 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
 	whole = 0;
 	first_len = max;
 
+	/* Check to see if we have a pending SACK and try to let it be bundled
+	 * with this message.  Do this if we don't have any data queued already.
+	 * To check that, look at out_qlen and retransmit list.
+	 */
+	if (timer_pending(&asoc->timers[SCTP_EVENT_TIMEOUT_SACK]) &&
+	    asoc->outqueue.out_qlen = 0 &&
+	    list_empty(&asoc->outqueue.retransmit))
+		first_len -= WORD_ROUND(sizeof(sctp_sack_chunk_t));
+
 	/* Encourage Cookie-ECHO bundling. */
 	if (asoc->state < SCTP_STATE_COOKIE_ECHOED) {
 		whole = msg_len / (max - SCTP_ARBITRARY_COOKIE_ECHO_LEN);
-- 
1.5.4.3

Re: [PATCH] sctp: Try to encourage SACK bundling with DATA.

[Wei Yongjun]

Vlad Yasevich wrote:
> If the association has a SACK timer pending and now DATA queued
> to be send, we'll try to bundle the SACK with the next application send.
> As such, try encourage bundling by accounting for SACK in the size
> of the first chunk fragment.
>
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> ---
>  net/sctp/chunk.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
> index 1748ef9..05b532e 100644
> --- a/net/sctp/chunk.c
> +++ b/net/sctp/chunk.c
> @@ -196,6 +196,15 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>  	whole = 0;
>  	first_len = max;
>  
> +	/* Check to see if we have a pending SACK and try to let it be bundled
> +	 * with this message.  Do this if we don't have any data queued already.
> +	 * To check that, look at out_qlen and retransmit list.
> +	 */
> +	if (timer_pending(&asoc->timers[SCTP_EVENT_TIMEOUT_SACK]) &&
> +	    asoc->outqueue.out_qlen = 0 &&
> +	    list_empty(&asoc->outqueue.retransmit))
> +		first_len -= WORD_ROUND(sizeof(sctp_sack_chunk_t));
> +
>   

Maybe we should check the first_len < WORD_ROUND(sizeof(sctp_sack_chunk_t)).
Some time when first_len = asoc->frag_point - AUTH_CHUNK - SACK_CHUNK, if
the user set the frag_point to a low value such as 40, the first_len will
be negative value.
The same problem exists when we hold the room for AUTH chunk.

I have checked that asoc->frag_point can be set between 8 and
SCTP_MAX_CHUNK_LEN.



>  	/* Encourage Cookie-ECHO bundling. */
>  	if (asoc->state < SCTP_STATE_COOKIE_ECHOED) {
>  		whole = msg_len / (max - SCTP_ARBITRARY_COOKIE_ECHO_LEN);
>   

Re: [PATCH] sctp: Try to encourage SACK bundling with DATA.

[Vlad Yasevich]

Wei Yongjun wrote:
> Vlad Yasevich wrote:
>> If the association has a SACK timer pending and now DATA queued
>> to be send, we'll try to bundle the SACK with the next application send.
>> As such, try encourage bundling by accounting for SACK in the size
>> of the first chunk fragment.
>>
>> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
>> ---
>>  net/sctp/chunk.c |    9 +++++++++
>>  1 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
>> index 1748ef9..05b532e 100644
>> --- a/net/sctp/chunk.c
>> +++ b/net/sctp/chunk.c
>> @@ -196,6 +196,15 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>>  	whole = 0;
>>  	first_len = max;
>>  
>> +	/* Check to see if we have a pending SACK and try to let it be bundled
>> +	 * with this message.  Do this if we don't have any data queued already.
>> +	 * To check that, look at out_qlen and retransmit list.
>> +	 */
>> +	if (timer_pending(&asoc->timers[SCTP_EVENT_TIMEOUT_SACK]) &&
>> +	    asoc->outqueue.out_qlen = 0 &&
>> +	    list_empty(&asoc->outqueue.retransmit))
>> +		first_len -= WORD_ROUND(sizeof(sctp_sack_chunk_t));
>> +
>>   
> 
> Maybe we should check the first_len < WORD_ROUND(sizeof(sctp_sack_chunk_t)).
> Some time when first_len = asoc->frag_point - AUTH_CHUNK - SACK_CHUNK, if
> the user set the frag_point to a low value such as 40, the first_len will
> be negative value.
> The same problem exists when we hold the room for AUTH chunk.
> 
> I have checked that asoc->frag_point can be set between 8 and
> SCTP_MAX_CHUNK_LEN.
> 

Good point.  Will do.

-vlad

> 
> 
>>  	/* Encourage Cookie-ECHO bundling. */
>>  	if (asoc->state < SCTP_STATE_COOKIE_ECHOED) {
>>  		whole = msg_len / (max - SCTP_ARBITRARY_COOKIE_ECHO_LEN);
>>   
> 
>