Re: [PATCH 15/32] tcp: Unify part of spliced and non-spliced conn_from_sock path

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 16 Nov 2022 15:41:55 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> In tcp_sock_handler() we split off to handle spliced sockets before
> checking anything else.  However the first steps of the "new connection"
> path for each case are the same: allocate a connection entry and accept()
> the connection.
> 
> Remove this duplication by making tcp_conn_from_sock() handle both spliced
> and non-spliced cases, with help from more specific tcp_tap_conn_from_sock
> and tcp_splice_conn_from_sock functions for the later stages which differ.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c        | 68 ++++++++++++++++++++++++++++++++++------------------
>  tcp_splice.c | 58 +++++++++++++++++++++++---------------------
>  tcp_splice.h |  4 ++++
>  3 files changed, 80 insertions(+), 50 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 72d3b49..e66a82a 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2753,28 +2753,19 @@ static void tcp_connect_finish(struct ctx *c, struct tcp_tap_conn *conn)
>  }
>  
>  /**
> - * tcp_conn_from_sock() - Handle new connection request from listening socket
> + * tcp_tap_conn_from_sock() - Initialize state for non-spliced connection
>   * @c:		Execution context
>   * @ref:	epoll reference of listening socket
> + * @conn:	connection structure to initialize
> + * @s:		Accepted socket
> + * @sa:		Peer socket address (from accept())
>   * @now:	Current timestamp
>   */
> -static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
> -			       const struct timespec *now)
> +static void tcp_tap_conn_from_sock(struct ctx *c, union epoll_ref ref,
> +				   struct tcp_tap_conn *conn, int s,
> +				   struct sockaddr *sa,
> +				   const struct timespec *now)
>  {
> -	struct sockaddr_storage sa;
> -	struct tcp_tap_conn *conn;
> -	socklen_t sl;
> -	int s;
> -
> -	if (c->tcp.conn_count >= TCP_MAX_CONNS)
> -		return;
> -
> -	sl = sizeof(sa);
> -	s = accept4(ref.r.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK);
> -	if (s < 0)
> -		return;
> -
> -	conn = CONN(c->tcp.conn_count++);
>  	conn->c.spliced = false;
>  	conn->sock = s;
>  	conn->timer = -1;
> @@ -2784,7 +2775,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
>  	if (ref.r.p.tcp.tcp.v6) {
>  		struct sockaddr_in6 sa6;
>  
> -		memcpy(&sa6, &sa, sizeof(sa6));
> +		memcpy(&sa6, sa, sizeof(sa6));
>  
>  		if (IN6_IS_ADDR_LOOPBACK(&sa6.sin6_addr) ||
>  		    IN6_ARE_ADDR_EQUAL(&sa6.sin6_addr, &c->ip6.addr_seen) ||
> @@ -2813,7 +2804,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
>  	} else {
>  		struct sockaddr_in sa4;
>  
> -		memcpy(&sa4, &sa, sizeof(sa4));
> +		memcpy(&sa4, sa, sizeof(sa4));
>  
>  		memset(&conn->a.a4.zero,   0, sizeof(conn->a.a4.zero));
>  		memset(&conn->a.a4.one, 0xff, sizeof(conn->a.a4.one));
> @@ -2846,6 +2837,37 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
>  	tcp_get_sndbuf(conn);
>  }
>  
> +/**
> + * tcp_conn_from_sock() - Handle new connection request from listening socket
> + * @c:		Execution context
> + * @ref:	epoll reference of listening socket
> + * @now:	Current timestamp
> + */
> +static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
> +			       const struct timespec *now)
> +{
> +	struct sockaddr_storage sa;
> +	union tcp_conn *conn;
> +	socklen_t sl;
> +	int s;
> +
> +	if (c->tcp.conn_count >= TCP_MAX_CONNS)
> +		return;
> +
> +	sl = sizeof(sa);
> +	s = accept4(ref.r.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK);

Combined with 16/32 I'm not sure this is simplifying much -- it looks a
bit unnatural there to get the peer address not "directly" from
accept4(). On the other hand you drop a few lines -- I'm fine with
it either way.

> +	if (s < 0)
> +		return;
> +
> +	conn = tc + c->tcp.conn_count++;
> +
> +	if (ref.r.p.tcp.tcp.splice)
> +		tcp_splice_conn_from_sock(c, ref, &conn->splice, s);
> +	else
> +		tcp_tap_conn_from_sock(c, ref, &conn->tap, s,
> +				       (struct sockaddr *)&sa, now);
> +}
> +
>  /**
>   * tcp_timer_handler() - timerfd events: close, send ACK, retransmit, or reset
>   * @c:		Execution context
> @@ -2925,13 +2947,13 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
>  		return;
>  	}
>  
> -	if (ref.r.p.tcp.tcp.splice) {
> -		tcp_sock_handler_splice(c, ref, events);
> +	if (ref.r.p.tcp.tcp.listen) {
> +		tcp_conn_from_sock(c, ref, now);
>  		return;
>  	}
>  
> -	if (ref.r.p.tcp.tcp.listen) {
> -		tcp_conn_from_sock(c, ref, now);
> +	if (ref.r.p.tcp.tcp.splice) {
> +		tcp_sock_handler_splice(c, ref, events);
>  		return;
>  	}
>  
> diff --git a/tcp_splice.c b/tcp_splice.c
> index 7a06252..7007501 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -501,6 +501,36 @@ static void tcp_splice_dir(struct tcp_splice_conn *conn, int ref_sock,
>  	*pipes = *from == conn->a ? conn->pipe_a_b : conn->pipe_b_a;
>  }
>  
> +/**
> + * tcp_splice_conn_from_sock() - Initialize state for spliced connection
> + * @c:		Execution context
> + * @ref:	epoll reference of listening socket
> + * @conn:	connection structure to initialize
> + * @s:		Accepted socket
> + *
> + * #syscalls:pasta setsockopt
> + */
> +void tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref,
> +			       struct tcp_splice_conn *conn, int s)
> +{
> +	assert(c->mode == MODE_PASTA);
> +
> +	if (setsockopt(s, SOL_TCP, TCP_QUICKACK, &((int){ 1 }),
> +		       sizeof(int))) {
> +		trace("TCP (spliced): failed to set TCP_QUICKACK on %i",
> +		      s);

This could be indented sanely, now.

> +	}
> +
> +	conn->c.spliced = true;
> +	c->tcp.splice_conn_count++;
> +	conn->a = s;
> +	conn->flags = ref.r.p.tcp.tcp.v6 ? SPLICE_V6 : 0;
> +
> +	if (tcp_splice_new(c, conn, ref.r.p.tcp.tcp.index,
> +			   ref.r.p.tcp.tcp.outbound))
> +		conn_flag(c, conn, CLOSING);
> +}
> +
>  /**
>   * tcp_sock_handler_splice() - Handler for socket mapped to spliced connection
>   * @c:		Execution context
> @@ -517,33 +547,7 @@ void tcp_sock_handler_splice(struct ctx *c, union epoll_ref ref,
>  	uint32_t *seq_read, *seq_write;
>  	struct tcp_splice_conn *conn;
>  
> -	if (ref.r.p.tcp.tcp.listen) {
> -		int s;
> -
> -		if (c->tcp.conn_count >= TCP_MAX_CONNS)
> -			return;
> -
> -		if ((s = accept4(ref.r.s, NULL, NULL, SOCK_NONBLOCK)) < 0)
> -			return;
> -
> -		if (setsockopt(s, SOL_TCP, TCP_QUICKACK, &((int){ 1 }),
> -			       sizeof(int))) {
> -			trace("TCP (spliced): failed to set TCP_QUICKACK on %i",
> -			      s);
> -		}
> -
> -		conn = CONN(c->tcp.conn_count++);
> -		conn->c.spliced = true;
> -		c->tcp.splice_conn_count++;
> -		conn->a = s;
> -		conn->flags = ref.r.p.tcp.tcp.v6 ? SPLICE_V6 : 0;
> -
> -		if (tcp_splice_new(c, conn, ref.r.p.tcp.tcp.index,
> -				   ref.r.p.tcp.tcp.outbound))
> -			conn_flag(c, conn, CLOSING);
> -
> -		return;
> -	}
> +	assert(!ref.r.p.tcp.tcp.listen);
>  
>  	conn = CONN(ref.r.p.tcp.tcp.index);
>  
> diff --git a/tcp_splice.h b/tcp_splice.h
> index 22024d6..f9462ae 100644
> --- a/tcp_splice.h
> +++ b/tcp_splice.h
> @@ -6,8 +6,12 @@
>  #ifndef TCP_SPLICE_H
>  #define TCP_SPLICE_H
>  
> +struct tcp_splice_conn;
> +
>  void tcp_sock_handler_splice(struct ctx *c, union epoll_ref ref,
>  			     uint32_t events);
> +void tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref,
> +			       struct tcp_splice_conn *conn, int s);
>  void tcp_splice_init(struct ctx *c);
>  
>  #endif /* TCP_SPLICE_H */

-- 
Stefano