On Thu, Nov 17, 2022 at 12:53:58AM +0100, Stefano Brivio wrote:
> 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.

Um.. I'm not really sure what you're getting at here.

> > +	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.

Fixed.

> > +	}
> > +
> > +	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 */
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson