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

On Wed, Mar 3, 2021 at 4:07 PM Jakub Kicinski [off-list ref] wrote:

On Wed, 3 Mar 2021 13:35:53 -0800 Alexander Duyck wrote:

quoted

On Tue, Mar 2, 2021 at 1:37 PM 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).

Alternative is that drivers unable to TX complete their skbs in a
reasonable time should call skb_orphan()
 to avoid skb_unclone() penalties (and this skb_still_in_host_queue() issue)

If you really want to play and delay TX completions, maybe provide a
way to disable skb_still_in_host_queue() globally,
using a static key ?

The problem as I see it is that the original fclone isn't what we sent
out on the wire and that is confusing things. What we sent was a SYN
with data, but what we have now is just a data frame that hasn't been
put out on the wire yet.

Not sure I understand why it's the key distinction here. Is it
re-transmitting part of the frame or having different flags?
Is re-transmit of half of a GSO skb also considered not the same?

The difference in my mind is the flags. So specifically the clone of
the syn_data frame in the case of the TCP fast open isn't actually a
clone of the sent frame. Instead we end up modifying the flags so that
it becomes the first data frame. We already have the SYN sitting in
the retransmit queue before we send the SYN w/ data frame. In addition
the SYN packet in the retransmit queue has a reference count of 1 so
it is not encumbered by the fclone reference count check so it could
theoretically be retransmitted immediately, it is just the data packet
that is being held.

If we replay a GSO frame we will get the same frames all over again.
In the case of a TCP fast open syn_data packet that isn't the case.
The first time out it is one packet, the second time it is two.

To me the distinction is that the receiver has implicitly asked
us for the re-transmission. If it was requested by SACK we should
ignore "in_queue" for the first transmission as well, even if the
skb state is identical.

In my mind the distinction is the fact that what we have in the
retransmit queue is 2 frames, a SYN and a data. Whereas what we have
put on the wire is SYN w/ data.

quoted

I wonder if we couldn't get away with doing something like adding a
fourth option of SKB_FCLONE_MODIFIED that we could apply to fastopen
skbs? That would keep the skb_still_in_host queue from triggering as
we would be changing the state from SKB_FCLONE_ORIG to
SKB_FCLONE_MODIFIED for the skb we store in the retransmit queue. In
addition if we have to clone it again and the fclone reference count
is 1 we could reset it back to SKB_FCLONE_ORIG.

The unused value of fclone was tempting me as well :)

AFAICT we have at least these options:

1 - don't use a fclone skb [v1]

2 - mark the fclone as "special" at Tx to escape the "in queue" check

This is what I had in mind. Basically just add a function to
transition from SKB_FCLONE_ORIG to SKB_FCLONE_MODIFIED/DIRTY when the
clone no longer represents what is actually on the wire.

The added benefit is that we can potentially restore it to
SKB_FCLONE_ORIG if we clone it again assuming the reference count fell
back to 1.

3 - indicate to retansmit that we're sure initial tx is out [v2]

4 - same as above but with a bool / flag instead of negative seg

5 - use the fclone bits but mark them at Rx when we see a rtx request

6 - check the skb state in retransmit to match the TFO case (IIUC
    Yuchung's suggestion)

#5 is my favorite but I didn't know how to extend it to fast
re-transmits so I just stuck to the suggestion from the ML :)

WDYT? Eric, Yuchung?