On Mon, Jan 23, 2017 at 10:59:22AM -0800, Wei Wang wrote:
> This patch adds a new socket option, TCP_FASTOPEN_CONNECT, as an
> alternative way to perform Fast Open on the active side (client).
Wei, I think that nothing prevents from reusin the original TCP_FASTOPEN
sockopt instead of adding a new one. The original one does this :
case TCP_FASTOPEN:
if (val >= 0 && ((1 << sk->sk_state) & (TCPF_CLOSE |
TCPF_LISTEN))) {
tcp_fastopen_init_key_once(true);
fastopen_queue_tune(sk, val);
} else {
err = -EINVAL;
}
break;
and your new option does this :
case TCP_FASTOPEN_CONNECT:
if (val > 1 || val < 0) {
err = -EINVAL;
} else if (sysctl_tcp_fastopen & TFO_CLIENT_ENABLE) {
if (sk->sk_state == TCP_CLOSE)
tp->fastopen_connect = val;
else
err = -EINVAL;
} else {
err = -EOPNOTSUPP;
}
break;
Now if we compare :
- the value ranges are the same (0,1)
- tcp_fastopen_init_key_once() only performs an initialization once
- fastopen_queue_tune() only sets sk->max_qlen based on the backlog,
this has no effect on an outgoing connection ;
- tp->fastopen_connect can be applied to a listening socket without
side effect.
Thus I think we can merge them this way :
case TCP_FASTOPEN:
if (val >= 0) {
if ((sysctl_tcp_fastopen & TFO_CLIENT_ENABLE) &&
(sk->sk_state == TCP_CLOSE)
tp->fastopen_connect = val;
if ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN))) {
tcp_fastopen_init_key_once(true);
fastopen_queue_tune(sk, val);
}
} else {
err = -EINVAL;
}
break;
And for the userland, the API is even simpler because we can use the
same TCP_FASTOPEN sockopt regardless of the socket direction. Also,
I don't know if TCP_FASTOPEN is supported on simultaneous connect,
but at least if it works it would be easier to understand this way.
Do you think there's a compelling reason for adding a new option or
are you interested in a small patch to perform the change above ?
Regards,
Willy
