On 7/2/18, 4:50 PM, "Yuchung Cheng" <ych...@google.com> wrote:
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
Feedback is always appreciated! This issue is also present in 4.11 (that is
where I discovered). I think the bug was introduces much earlier.
Yes, I tested with Eric's quickack fix, it did not fix either of the two issues
that are fixed with this patch set.
As I mentioned earlier, the bug was introduced before 4.11. I am not sure I
understand your comments. Yes, at some level it would make sense to change the
delayed_ack_reserved bit directly, but we would still need to do it whenever we
send the ACK, so I do not think it can be helped. Please clarify if I
misunderstood your comment.
>
> 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
>
