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

On Thu, Nov 17, 2022 at 08:30:29AM +0100, Stefano Brivio wrote:
...
  On Thu, 17 Nov 2022 12:37:04 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  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(a)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(a)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. 
 
 By "directly" I mean assigned by accept4() in the same function,
 instead of accept4() being done in the caller.
 
 That is, if I now look at tcp_tap_conn_from_sock() we have 'sa' there
 which comes as an argument, not directly a couple of lines above from
 accept4(), which would be quicker to review. 
Right, but this is unavoidable.  This patch is a preliminary to
deciding whether to take the spliced or non-spliced route based on the
address we get from accept4().

...
  On the other hand the function comment says "from
accept()", so it's
 not much effort to figure that out either. 
-- 
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