sctp: fix heartbeat timer

[tuexen]

Summary

Adding the +/-50%RTO jitter applies only to paths, which are not potentially failed and which are not unconfirmed and not idle.

[albin.hellqvist_ericsson.com]

sys/netinet/sctputil.c

2292–2293

Should SCTP_ADDR_REACHABLE be checked here?

This might be enough?

if ((net->dest_state & (SCTP_ADDR_UNCONFIRMED | SCTP_ADDR_PF)) == 0) {

In that way, we don't add the jitter and heartbeat interval if the path is unconfirmed or potentially failed.

2295–2304

I might be mistaken, but before we had (assuming that RTO > 1):
to_ticks == [RTO/2, 3*RTO/2]
or
to_ticks == [RTO/2, 3*RTO/2] + HB-interval

And now we have (assuming that RTO > 1):
to_ticks == RTO
or
to_ticks == [RTO/2, RTO] + HB-interval

And I think that we would want this (assuming that RTO > 1)?
to_ticks == RTO
or
to_ticks == [RTO/2, 3*RTO/2] + HB-interval

I think the switch case could be easier to understand with a new variable called rto, like in this example:

uint32_t to_ticks;
uint32_t rto, rndval, jitter;

...

if (net->RTO == 0) {
	rto = stcb->asoc.initial_rto;
} else {
	rto = net->RTO;
}

// to_ticks == RTO
to_ticks = rto;

if ((net->dest_state & (SCTP_ADDR_UNCONFIRMED | SCTP_ADDR_PF)) == 0) {
	if (rto > 1) {
		rndval = sctp_select_initial_TSN(&inp->sctp_ep);
		
		// jitter == [0, RTO]
		jitter = rndval % rto;		

		// to_ticks == RTO/2
		to_ticks >>= 1;
		
		// Haven't calculated how this if statement could end up.
		if (jitter < (UINT32_MAX - to_ticks)) {
			// to_ticks == [RTO/2, 3*RTO/2]
			to_ticks += jitter;
		} else {
			to_ticks = UINT32_MAX;
		}
	}
	
	// Haven't calculated how this if statement could end up.
	if (net->heart_beat_delay < (UINT32_MAX - to_ticks)) {
		to_ticks += net->heart_beat_delay;
	} else {
		to_ticks = UINT32_MAX;
	}
}

...

[tuexen]

Fix computation of jitter.

sys/netinet/sctputil.c

2292–2293

Path Verification contains:

In each RTO, a probe MAY be sent on an active UNCONFIRMED path in
an attempt to move it to the CONFIRMED state. If during this probing the
path becomes inactive, this rate is lowered to the normal HEARTBEAT rate.

I think this is captured exactly with:

if (!((net->dest_state & SCTP_ADDR_UNCONFIRMED) &&
      (net->dest_state & SCTP_ADDR_REACHABLE)) &&
    ((net->dest_state & SCTP_ADDR_PF) == 0)) {

2295–2304

You are correct. Fixed in the latest patch.

[albin.hellqvist_ericsson.com]

Nice!

[tuexen]

In D45107#1029978, @albin.hellqvist_ericsson.com wrote:

Nice!

Thanks for the review!

[tuexen]

Committed in 5120ea0d8871.