Re: [PATCH 7/9] epoll: Split handling of listening TCP sockets into their own handler

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

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

On Mon, Aug 07, 2023 at 11:46:29PM +1000, David Gibson wrote:
> tcp_sock_handler() handles both listening TCP sockets, and connected TCP
> sockets, but what it needs to do in those cases has essentially nothing in
> common.  Therefore, give listening sockets their own epoll_type value and
> dispatch directly to their own handler from the top level.  This also lets
> us remove the listen field from tcp_epoll_ref, since the information it
> carries is implicit in the epoll_type.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Stefano,

I've realized an additional change that belongs with this patch, so
I'll be making a second spin.  Go ahead and review, but don't apply
just yet.

> ---
>  passt.c |  8 ++++++--
>  passt.h |  4 +++-
>  tcp.c   | 31 +++++++++----------------------
>  tcp.h   |  8 ++++----
>  util.c  |  2 +-
>  5 files changed, 23 insertions(+), 30 deletions(-)
> 
> diff --git a/passt.c b/passt.c
> index c750fad..c32981d 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -56,7 +56,8 @@
>  char pkt_buf[PKT_BUF_BYTES]	__attribute__ ((aligned(PAGE_SIZE)));
>  
>  char *epoll_type_str[EPOLL_TYPE_MAX+1] = {
> -	[EPOLL_TYPE_TCP]	= "TCP socket",
> +	[EPOLL_TYPE_TCP]	= "connected TCP socket",
> +	[EPOLL_TYPE_TCP_LISTEN]	= "listening TCP socket",
>  	[EPOLL_TYPE_TCP_TIMER]	= "TCP timer",
>  	[EPOLL_TYPE_UDP]	= "UDP socket",
>  	[EPOLL_TYPE_ICMP]	= "ICMP socket",
> @@ -323,7 +324,10 @@ loop:
>  			break;
>  		case EPOLL_TYPE_TCP:
>  			if (!c.no_tcp)
> -				tcp_sock_handler(&c, ref, eventmask, &now);
> +				tcp_sock_handler(&c, ref, eventmask);
> +			break;
> +		case EPOLL_TYPE_TCP_LISTEN:
> +			tcp_listen_handler(&c, ref, &now);
>  			break;
>  		case EPOLL_TYPE_TCP_TIMER:
>  			tcp_timer_handler(&c, ref);
> diff --git a/passt.h b/passt.h
> index fc1efdb..176bc85 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -47,8 +47,10 @@ union epoll_ref;
>  enum epoll_type {
>  	/* Special value to indicate an invalid type */
>  	EPOLL_TYPE_NONE = 0,
> -	/* TCP sockets */
> +	/* Connected TCP sockets */
>  	EPOLL_TYPE_TCP,
> +	/* Listening TCP sockets */
> +	EPOLL_TYPE_TCP_LISTEN,
>  	/* timerfds used for TCP timers */
>  	EPOLL_TYPE_TCP_TIMER,
>  	/* UDP sockets */
> diff --git a/tcp.c b/tcp.c
> index 98761a2..c237393 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2765,22 +2765,20 @@ static void tcp_tap_conn_from_sock(struct ctx *c, union epoll_ref ref,
>  }
>  
>  /**
> - * tcp_conn_from_sock() - Handle new connection request from listening socket
> + * tcp_listen_handler() - 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)
> +void tcp_listen_handler(struct ctx *c, union epoll_ref ref,
> +			const struct timespec *now)
>  {
>  	struct sockaddr_storage sa;
>  	union tcp_conn *conn;
>  	socklen_t sl;
>  	int s;
>  
> -	ASSERT(ref.tcp.listen);
> -
> -	if (c->tcp.conn_count >= TCP_MAX_CONNS)
> +	if (c->no_tcp || c->tcp.conn_count >= TCP_MAX_CONNS)
>  		return;
>  
>  	sl = sizeof(sa);
> @@ -2926,19 +2924,10 @@ static void tcp_tap_sock_handler(struct ctx *c, struct tcp_tap_conn *conn,
>   * @c:		Execution context
>   * @ref:	epoll reference
>   * @events:	epoll events bitmap
> - * @now:	Current timestamp
>   */
> -void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
> -		      const struct timespec *now)
> +void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
>  {
> -	union tcp_conn *conn;
> -
> -	if (ref.tcp.listen) {
> -		tcp_conn_from_sock(c, ref, now);
> -		return;
> -	}
> -
> -	conn = tc + ref.tcp.index;
> +	union tcp_conn *conn = tc + ref.tcp.index;
>  
>  	if (conn->c.spliced)
>  		tcp_splice_sock_handler(c, &conn->splice, ref.fd, events);
> @@ -2960,7 +2949,7 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
>  			    const struct in_addr *addr, const char *ifname)
>  {
>  	in_port_t idx = port + c->tcp.fwd_in.delta[port];
> -	union tcp_epoll_ref tref = { .listen = 1, .index = idx };
> +	union tcp_epoll_ref tref = { .index = idx };
>  	int s;
>  
>  	s = sock_l4(c, af, IPPROTO_TCP, addr, ifname, port, tref.u32);
> @@ -3020,8 +3009,7 @@ int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
>  static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port)
>  {
>  	in_port_t idx = port + c->tcp.fwd_out.delta[port];
> -	union tcp_epoll_ref tref = { .listen = 1, .outbound = 1,
> -				     .index = idx };
> +	union tcp_epoll_ref tref = { .outbound = 1, .index = idx };
>  	struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
>  	int s;
>  
> @@ -3045,8 +3033,7 @@ static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port)
>  static void tcp_ns_sock_init6(const struct ctx *c, in_port_t port)
>  {
>  	in_port_t idx = port + c->tcp.fwd_out.delta[port];
> -	union tcp_epoll_ref tref = { .listen = 1, .outbound = 1,
> -				     .index = idx };
> +	union tcp_epoll_ref tref = { .outbound = 1, .index = idx };
>  	int s;
>  
>  	ASSERT(c->mode == MODE_PASTA);
> diff --git a/tcp.h b/tcp.h
> index 8eb7782..8189ac0 100644
> --- a/tcp.h
> +++ b/tcp.h
> @@ -14,8 +14,9 @@
>  struct ctx;
>  
>  void tcp_timer_handler(struct ctx *c, union epoll_ref ref);
> -void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
> -		      const struct timespec *now);
> +void tcp_listen_handler(struct ctx *c, union epoll_ref ref,
> +			const struct timespec *now);
> +void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events);
>  int tcp_tap_handler(struct ctx *c, int af, const void *addr,
>  		    const struct pool *p, const struct timespec *now);
>  int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
> @@ -37,8 +38,7 @@ void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s,
>   */
>  union tcp_epoll_ref {
>  	struct {
> -		uint32_t	listen:1,
> -				outbound:1,
> +		uint32_t	outbound:1,
>  				index:20;
>  	};
>  	uint32_t u32;
> diff --git a/util.c b/util.c
> index 2cac7ba..d965f48 100644
> --- a/util.c
> +++ b/util.c
> @@ -120,7 +120,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
>  
>  	switch (proto) {
>  	case IPPROTO_TCP:
> -		ref.type = EPOLL_TYPE_TCP;
> +		ref.type = EPOLL_TYPE_TCP_LISTEN;
>  		break;
>  	case IPPROTO_UDP:
>  		ref.type = EPOLL_TYPE_UDP;

-- 
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 --]