Re: [PATCH net-next 3/3] net/tcp-fastopen: Add new API support

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