Message-ID: <2166c3ff-e08d-e89d-4753-01c8bd2d9505@akamai.com>
Date:   Tue, 22 Oct 2019 15:32:54 -0400
From:   Jason Baron <jbaron@...mai.com>
To:     Yuchung Cheng <ycheng@...gle.com>,
        Neal Cardwell <ncardwell@...gle.com>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        Christoph Paasch <cpaasch@...le.com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [net-next] tcp: add TCP_INFO status for failed client TFO



On 10/22/19 2:17 PM, Yuchung Cheng wrote:
> On Mon, Oct 21, 2019 at 7:14 PM Neal Cardwell <ncardwell@...gle.com> wrote:
>>
>> On Mon, Oct 21, 2019 at 5:11 PM Jason Baron <jbaron@...mai.com> wrote:
>>>
>>>
>>>
>>> On 10/21/19 4:36 PM, Eric Dumazet wrote:
>>>> On Mon, Oct 21, 2019 at 12:53 PM Christoph Paasch <cpaasch@...le.com> wrote:
>>>>>
>>>>
>>>>> Actually, longterm I hope we would be able to get rid of the
>>>>> blackhole-detection and fallback heuristics. In a far distant future where
>>>>> these middleboxes have been weeded out ;-)
>>>>>
>>>>> So, do we really want to eternalize this as part of the API in tcp_info ?
>>>>>
>>>>
>>>> A getsockopt() option won't be available for netlink diag (ss command).
>>>>
>>>> So it all depends on plans to use this FASTOPEN information ?
>>>>
>>>
>>> The original proposal I had 4 states of interest:
>>>
>>> First, we are interested in knowing when a socket has TFO set but
>>> actually requires a retransmit of a non-TFO syn to become established.
>>> In this case, we'd be better off not using TFO.
>>>
>>> A second case is when the server immediately rejects the DATA and just
>>> acks the syn (but not the data). Again in that case, we don't want to be
>>> sending syn+data.
>>>
>>> The third case was whether or not we sent a cookie. Perhaps, the server
>>> doesn't have TFO enabled in which case, it really doesn't make make
>>> sense to enable TFO in the first place. Or if one also controls the
>>> server its helpful in understanding if the server is mis-configured. So
>>> that was the motivation I had for the original four states that I
>>> proposed (final state was a catch-all state).
>>>
>>> Yuchung suggested dividing the 3rd case into 2 for - no cookie sent
>>> because of blackhole or no cookie sent because its not in cache. And
>>> then dropping the second state because we already have the
>>> TCPI_OPT_SYN_DATA bit. However, the TCPI_OPT_SYN_DATA may not be set
>>> because we may fallback in tcp_send_syn_data() due to send failure. So
> but sendto would return -1 w/ EINPROGRESS in this case already so the
> application shouldn't expect TCPI_OPT_SYN_DATA?

Ok, but let's say the sk_stream_alloc_skb() fails in
tcp_send_syn_data(), in that case we aren't going to send a TFO cookie
(just a regular syn). The user isn't going to get any error and would
expect TCPI_OPT_SYN_DATA. Now, TCPI_OPT_SYN_DATA wouldn't be set but we
can't assume then that a SYN+data was sent and the SYN_ACK didn't cover
the data part. Instead, the reason for failure is really -ENOMEM, which
in the proposed states would fall into TFO_STATUS_UNSPEC, but it does
mean that I think we shouldn't have a TFO_DATA_NOT_ACKED state otherwise
we can't differentiate the two.

> 
> 
>>> I'm inclined to say that the second state is valuable. And since
>>> blackhole is already a global thing via MIB, I didn't see a strong need
>>> for it. But it sounded like Yuchung had an interest in it, and I'd
>>> obviously like a set of states that is generally useful.
>>
>> I have not kept up with all the proposals in this thread, but would it
>> work to include all of the cases proposed in this thread? It seems
>> like there are enough bits available in holes in tcp_sock and tcp_info
>> to have up to 7 bits of information saved in tcp_sock and exported to
>> tcp_info. That seems like more than enough?
> I would rather use only at most 2-bits for TFO errors to be
> parsimonious on (bloated) tcp sock. I don't mind if the next patch
> skip my idea of BH detection.
> my experience is reading host global stats for most applications are a
> hassle (or sometimes not even feasible). they mostly care about
> information of their own sockets.

hmmm, if you are ok without having the BH failure state which Christoph
also pushed back on a bit, but we still only want 2 bits as you
suggested how about:

1) TFO_STATUS_UNSPEC - includes black hole state and other failures
2) TFO_COOKIE_UNAVAILABLE - we don't have a cookie in the cache
3) TFO_DATA_NOT_ACKED - syn+data sent but no ack for data part
4) TFO_SYN_RETRANSMITTED - same as 3 but at least 1 additional syn sent

Thanks,

-Jason

Powered by blists - more mailing lists