On Mon, Oct 25, 2021 at 01:48:12PM -0700, Eric Dumazet wrote:

On 10/25/21 12:26 PM, Alexander Azimov wrote:

quoted

Hi Eric,
 
Can you please clarify why do you think that SYN RTO should be accessible through BPF and SYN ACK RTO should be bound to TCP_TIMEOUT_INIT constant?

quoted

I can't see the reason for such asymmetry, please advise.

In tcp_conn_request(),  tcp_timeout_init() is used, so the very
first SYN-ACK RTO should work?

iiuc, this patch's changes in reqsk_timer_handler() only affects the
later RTOs (e.g. 2nd RTOs).  For this particular
function (reqsk_timer_handler()), it looks like an overlook
in the original bpf's tcp_timeout_init() implementation.

quoted

I also wonder what kind of existing BPF programs can suffer from these changes. Please give us your insights.

ps: this is a copy of the original message, hope this one will come in a plain text

When commit 8550f328f45db6d37981eb2041bc465810245c03
("bpf: Support for per connection SYN/SYN-ACK RTOs")
was added, tcp_timeout_init() (and potential eBPF prog)
would be called from tcp_conn_request() and tcp_connect_init()

So some users are now using this eBPF program, expecting
it to have an effect only from these call sites.

If you are adding more call sites, suddenly the behavior
of TCP stack for these users will change. You have to
document if bad things could happen for them, like unexpected
max acceptable delays for connection establishment being severely reduced.

In any case, I would prefer adding a new @timeout field
in struct request_sock to carry the base timeout
instead of calling a BPF program all the times,
otherwise we could have weird behavior (eg PAWS checks)
if the return from eBPF program is variable for one 5-tuple.

Seems like a good idea.  This field can be set to
the timeout-value returned by the bpf prog.