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

[Stefano Brivio <sbrivio@redhat.com>]

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.

On the other hand the function comment says "from accept()", so it's
not much effort to figure that out either.

-- 
Stefano