From 4e71409fe1305cde4b9b341247ba658d8d24f4d0 Mon Sep 17 00:00:00 2001 From: Simon Wilkinson Date: Mon, 11 Oct 2010 13:14:02 -0400 Subject: [PATCH] Rx: Reject out of order ACK packets Our RX implementation virtually guarantees that we will see out of order ACK packets, even on well behaved networks, as we send acks simultaneously from multiple threads. Currently we only reject out-of-order ACKS which change the window position (so a window that advances, can never go back). However, we fail to deal with the explicit acknowledgement portion of the ACK packet in the same way... For example, if we have a packet A that acknowledges packets 1 and 2, and then a packet B acknowledging 1,2,3 and 4. If B arrives before A, then we mark 1, 2, 3, 4 as acknowledged, and then treat the arrival of A as nAcking 3 and 4. This has the same effect as an explicitly stated nack, triggers an early and unnecessary resend and may, in some situations, cause the call to go into congestion avoidance. We can solve this using the previousPacket field of the ACK. This indicates the last packet seen by the peer. In the same way as firstPacket, this should never go backwards, and so can be used to detect out of order acknowledgements, and reject them. Change-Id: I9ad850872a1a62050e774c911302a65bb8a59525 Reviewed-on: http://gerrit.openafs.org/2958 Tested-by: Derrick Brashear Reviewed-by: Derrick Brashear --- src/rx/rx.c | 8 +++++++- src/rx/rx.h | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/rx/rx.c b/src/rx/rx.c index ff89d7edb..794834a7c 100644 --- a/src/rx/rx.c +++ b/src/rx/rx.c @@ -3939,6 +3939,7 @@ rxi_ReceiveAckPacket(struct rx_call *call, struct rx_packet *np, struct rx_peer *peer = conn->peer; struct clock now; /* Current time, for RTT calculations */ afs_uint32 first; + afs_uint32 prev; afs_uint32 serial; /* because there are CM's that are bogus, sending weird values for this. */ afs_uint32 skew = 0; @@ -3961,15 +3962,19 @@ rxi_ReceiveAckPacket(struct rx_call *call, struct rx_packet *np, /* depends on ack packet struct */ nAcks = MIN((unsigned)nbytes, (unsigned)ap->nAcks); first = ntohl(ap->firstPacket); + prev = ntohl(ap->previousPacket); serial = ntohl(ap->serial); /* temporarily disabled -- needs to degrade over time * skew = ntohs(ap->maxSkew); */ /* Ignore ack packets received out of order */ - if (first < call->tfirst) { + if (first < call->tfirst || + (first == call->tfirst && prev < call->tprev)) { return np; } + call->tprev = prev; + if (np->header.flags & RX_SLOW_START_OK) { call->flags |= RX_CALL_SLOW_START_OK; } @@ -5043,6 +5048,7 @@ rxi_ResetCall(struct rx_call *call, int newcall) call->nHardAcks = 0; call->tfirst = call->rnext = call->tnext = 1; + call->tprev = 0; call->rprev = 0; call->lastAcked = 0; call->localStatus = call->remoteStatus = 0; diff --git a/src/rx/rx.h b/src/rx/rx.h index 870499084..2c05d30a9 100644 --- a/src/rx/rx.h +++ b/src/rx/rx.h @@ -518,6 +518,7 @@ struct rx_call { afs_uint32 rwind; /* The receive window: the peer must not send packets with sequence numbers >= rnext+rwind */ afs_uint32 tfirst; /* First unacknowledged transmit packet number */ afs_uint32 tnext; /* Next transmit sequence number to use */ + afs_uint32 tprev; /* Last packet that we saw an ack for */ u_short twind; /* The transmit window: we cannot assign a sequence number to a packet >= tfirst + twind */ u_short cwind; /* The congestion window */ u_short nSoftAcked; /* Number soft acked transmit packets */ -- 2.39.5