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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5580 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]