Re: [PATCH] sctp: Fix broken RTO-doubling for data retransmits

[Wei Yongjun <yjwei@cn.fujitsu.com>]

Vlad Yasevich wrote:
> Commit faee47cdbfe8d74a1573c2f81ea6dbb08d735be6
> (sctp: Fix the RTO-doubling on idle-link heartbeats)
> broke the RTO doubling for data retransmits.  If the
> heartbeat was sent before the data T3-rtx time, the
> the RTO will not double upon the T3-rtx expiration.
> Distingish between the operations by passing an argument
> to the function.
>   

Hi Vlad

I think we should talk about what the SCTP_CMD_TRANSPORT_RESET and
SCTP_CMD_HB_TIMER_UPDATE command do.

SCTP_CMD_TRANSPORT_RESET
--> sctp_cmd_transport_reset()
--> sctp_transport_lower_cwnd()
... transport->cwnd = XXX if timer expire
... partial_bytes_acked = 0;
--> sctp_do_8_2_transport_strike()
... asoc->overall_error_count++;
... transport->rto = XXX if not send HB

SCTP_CMD_HB_TIMER_UPDATE
--> sctp_cmd_hb_timer_update()
... mod_timer(&t->hb_timer, sctp_transport_timeout(t))

In sctp_sf_do_prm_requestheartbeat(), it used
SCTP_CMD_TRANSPORT_RESET command, not SCTP_CMD_HB_TIMER_UPDATE,
so if user request to send HB, the partial_bytes_acked will be clear,
and the overall_error_count be increased. And the heartbeat primitive
may not send HB, but only mark the transport state to SCTP_INACTIVE.

We user request to send HB immediately:
- the HB timer will be update, so HB will expires after
(HB_INTERVAL + RTO +/- 50% * RTO).
- the overall_error_count should not be increased. Because this is not
a idle-link heartbeat.
- the cwnd and partial_bytes_acked should not be changed.

Is this correct?

And after the patch I sent, used SCTP_CMD_HB_TIMER_UPDATE instead of
SCTP_CMD_TRANSPORT_RESET command. The action of idle-link heartbeat
and user initiated heartbeat are like this.

idle-link heartbeat:

Endpoint A                      Endpoint B
   |
   | idle
   |
HEARTBEAT    ----------->
   | lower cwnd
   | overall_error_count++
   | RTO = RTO * 2
   | expires HB_INTERVAL + RTO +/- 50% * RTO
HEARTBEAT    ----------->
   | lower cwnd
   | overall_error_count++
   | RTO = RTO * 2
   | expires HB_INTERVAL + RTO +/- 50% * RTO

user initiated heartbeat

Endpoint A                      Endpoint B
   |
   |
   |
HEARTBEAT    ----------->     (*) first HB
   |
   | reset hb_timer, not update RTO
   |
HEARTBEAT    ----------->
   | lower cwnd
   | overall_error_count++
   | RTO = RTO * 2
   | expires HB_INTERVAL + RTO +/- 50% * RTO


The different between two is that only reset the HB timer, the
other increased overall_error_count and lower cwnd. I agree with
reset the HB timer.

Regards.

> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> ---
>  net/sctp/sm_sideeffect.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 0146cfb..4ac6163 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -434,7 +434,8 @@ sctp_timer_event_t *sctp_timer_events[SCTP_NUM_TIMEOUT_TYPES] = {
>   *
>   */
>  static void sctp_do_8_2_transport_strike(struct sctp_association *asoc,
> -					 struct sctp_transport *transport)
> +					 struct sctp_transport *transport,
> +					 int is_hb)
>  {
>  	/* The check for association's overall error counter exceeding the
>  	 * threshold is done in the state function.
> @@ -466,7 +467,7 @@ static void sctp_do_8_2_transport_strike(struct sctp_association *asoc,
>  	 * The first unacknowleged HB triggers it.  We do this with a flag
>  	 * that indicates that we have an outstanding HB.
>  	 */
> -	if (transport->hb_sent) {
> +	if (!is_hb || transport->hb_sent) {
>  		transport->last_rto = transport->rto;
>  		transport->rto = min((transport->rto * 2), transport->asoc->rto_max);
>  	}
> @@ -667,7 +668,7 @@ static void sctp_cmd_transport_reset(sctp_cmd_seq_t *cmds,
>  	sctp_transport_lower_cwnd(t, SCTP_LOWER_CWND_INACTIVE);
>  
>  	/* Mark one strike against a transport.  */
> -	sctp_do_8_2_transport_strike(asoc, t);
> +	sctp_do_8_2_transport_strike(asoc, t, 1);
>  
>  	t->hb_sent = 1;
>  }
> @@ -1459,7 +1460,8 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
>  
>  		case SCTP_CMD_STRIKE:
>  			/* Mark one strike against a transport.  */
> -			sctp_do_8_2_transport_strike(asoc, cmd->obj.transport);
> +			sctp_do_8_2_transport_strike(asoc, cmd->obj.transport,
> +						    0);
>  			break;
>  
>  		case SCTP_CMD_TRANSPORT_RESET:
>