Re: [PATCH net] net: tcp: don't allocate fast clones for fastopen SYN

On Tue, 2 Mar 2021 12:52:14 -0800 Yuchung Cheng wrote:

On Tue, Mar 2, 2021 at 11:58 AM Eric Dumazet [off-list ref] wrote:

quoted

On Tue, Mar 2, 2021 at 7:08 AM Jakub Kicinski [off-list ref] wrote:  

quoted

When receiver does not accept TCP Fast Open it will only ack
the SYN, and not the data. We detect this and immediately queue
the data for (re)transmission in tcp_rcv_fastopen_synack().

In DC networks with very low RTT and without RFS the SYN-ACK
may arrive before NIC driver reported Tx completion on
the original SYN. In which case skb_still_in_host_queue()
returns true and sender will need to wait for the retransmission
timer to fire milliseconds later.

Revert back to non-fast clone skbs, this way
skb_still_in_host_queue() won't prevent the recovery flow
from completing.

Suggested-by: Eric Dumazet <edumazet@google.com>
Fixes: 355a901e6cf1 ("tcp: make connect() mem charging friendly")  

Hmmm, not sure if this Fixes: tag makes sense.

Really, if we delay TX completions by say 10 ms, other parts of the
stack will misbehave anyway.

Also, backporting this patch up to linux-3.19 is going to be tricky.

The real issue here is that skb_still_in_host_queue() can give a false positive.

I have mixed feelings here, as you can read my answer :/

Maybe skb_still_in_host_queue() signal should not be used when a part
of the SKB has been received/acknowledged by the remote peer
(in this case the SYN part).  

Thank you Eric and Jakub for working on the TFO issue.

I like this option the most because it's more generic and easy to understand. 

I'm assuming by "this" you mean v1 - as far as understanding goes we
can polish v2. I think Eric just shared a quick example, but perhaps 
we could add an explicit bool to __tcp_retransmit_skb() called expl_req
or such to signify that the rtx was explicitly requested by the
receiver and therefore we can skip the "in queue" check? Or add some
flags to the call?

If everyone is okay with targeting -next the new argument won't be an
issue.

Is it easy to implement by checking snd_una etc?

That is to detect TFO retransmits in __tcp_retransmit_skb() by matching
the connections state, like acked == 1?