On Mon, Jul 2, 2018 at 2:39 PM, Lawrence Brakmo <bra...@fb.com> wrote: > > DCTCP depends on the CA_EVENT_NON_DELAYED_ACK and CA_EVENT_DELAYED_ACK > notifications to keep track if it needs to send an ACK for packets that > were received with a particular ECN state but whose ACK was delayed. > > Under some circumstances, for example when a delayed ACK is sent with a > data packet, DCTCP state was not being updated due to a lack of > notification that the previously delayed ACK was sent. As a result, it > would sometimes send a duplicate ACK when a new data packet arrived. > > This patch insures that DCTCP's state is correctly updated so it will > not send the duplicate ACK. Sorry to chime-in late here (lame excuse: IETF deadline)
IIRC this issue would exist prior to 4.11 kernel. While it'd be good to fix that, it's not clear which patch introduced the regression between 4.11 and 4.16? I assume you tested Eric's most recent quickack fix. In terms of the fix itself, it seems odd the tcp_send_ack() call in DCTCP generates NON_DELAYED_ACK event to toggle DCTCP's delayed_ack_reserved bit. Shouldn't the fix to have DCTCP send the "prior" ACK w/o cancelling delayed-ACK and mis-recording that it's cancelled, because that prior-ACK really is a supplementary old ACK. But it's still unclear how this bug introduces the regression 4.11 - 4.16 > > Improved based on comments from Neal Cardwell <ncardw...@google.com>. > > Signed-off-by: Lawrence Brakmo <bra...@fb.com> > --- > net/ipv4/tcp_output.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index f8f6129160dd..acefb64e8280 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -172,6 +172,8 @@ static inline void tcp_event_ack_sent(struct sock *sk, > unsigned int pkts) > __sock_put(sk); > } > tcp_dec_quickack_mode(sk, pkts); > + if (inet_csk_ack_scheduled(sk)) > + tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK); > inet_csk_clear_xmit_timer(sk, ICSK_TIME_DACK); > } > > @@ -3567,8 +3569,6 @@ void tcp_send_ack(struct sock *sk) > if (sk->sk_state == TCP_CLOSE) > return; > > - tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK); > - > /* We are not putting this on the write queue, so > * tcp_transmit_skb() will set the ownership to this > * sock. > -- > 2.17.1 >
