Re: [PATCH 01/10] tcp: no v6 flag in ref

[Stefano Brivio <sbrivio@redhat.com>]

On Fri,  4 Nov 2022 19:43:24 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> ---
>  tcp.c        |  9 ++++-----
>  tcp.h        |  1 -
>  tcp_splice.c | 13 +++++++------
>  3 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 713248f..3d48d6e 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -761,8 +761,7 @@ static int tcp_epoll_ctl(const struct ctx *c, struct tcp_conn *conn)
>  {
>  	int m = (conn->flags & IN_EPOLL) ? EPOLL_CTL_MOD : EPOLL_CTL_ADD;
>  	union epoll_ref ref = { .r.proto = IPPROTO_TCP, .r.s = conn->sock,
> -				.r.p.tcp.tcp.index = conn - tc,
> -				.r.p.tcp.tcp.v6 = CONN_V6(conn) };
> +				.r.p.tcp.tcp.index = conn - tc, };
>  	struct epoll_event ev = { .data.u64 = ref.u64 };
>  
>  	if (conn->events == CLOSED) {
> @@ -2857,6 +2856,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
>  	if (c->tcp.conn_count >= TCP_MAX_CONNS)
>  		return;
>  
> +	sa.ss_family = AF_UNSPEC; /* FIXME: stop clang-tidy complaining */

No need for /* FIXME */ here in my opinion, valgrind should also hit
this -- perhaps move to initialiser though.

>  	sl = sizeof(sa);
>  	s = accept4(ref.r.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK);
>  	if (s < 0)
> @@ -2868,7 +2868,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
>  	conn->ws_to_tap = conn->ws_from_tap = 0;
>  	conn_event(c, conn, SOCK_ACCEPTED);
>  
> -	if (ref.r.p.tcp.tcp.v6) {
> +	if (sa.ss_family == AF_INET6) {
>  		struct sockaddr_in6 sa6;
>  
>  		memcpy(&sa6, &sa, sizeof(sa6));
> @@ -3147,8 +3147,7 @@ static void tcp_sock_init6(const struct ctx *c, int ns,
>  			   const struct in6_addr *addr, const char *ifname,
>  			   in_port_t port)
>  {
> -	union tcp_epoll_ref tref = { .tcp.listen = 1, .tcp.outbound = ns,
> -				     .tcp.v6 = 1 };
> +	union tcp_epoll_ref tref = { .tcp.listen = 1, .tcp.outbound = ns, };

Undocumented style rule: no comma at the end, it's post-modern C anyway.

>  	bool spliced = false, tap = true;
>  	int s;
>  
> diff --git a/tcp.h b/tcp.h
> index 3fabb5a..85ac750 100644
> --- a/tcp.h
> +++ b/tcp.h
> @@ -45,7 +45,6 @@ union tcp_epoll_ref {
>  		uint32_t	listen:1,
>  				splice:1,
>  				outbound:1,
> -				v6:1,

Don't forget to update the comment above.

>  				timer:1,
>  				index:20;
>  	} tcp;
> diff --git a/tcp_splice.c b/tcp_splice.c
> index 99c5fa7..1f26ff4 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -211,12 +211,10 @@ static int tcp_splice_epoll_ctl(const struct ctx *c,
>  	int m = (conn->flags & IN_EPOLL) ? EPOLL_CTL_MOD : EPOLL_CTL_ADD;
>  	union epoll_ref ref_a = { .r.proto = IPPROTO_TCP, .r.s = conn->a,
>  				  .r.p.tcp.tcp.splice = 1,
> -				  .r.p.tcp.tcp.index = conn - tc,
> -				  .r.p.tcp.tcp.v6 = CONN_V6(conn) };
> +				  .r.p.tcp.tcp.index = conn - tc, };
>  	union epoll_ref ref_b = { .r.proto = IPPROTO_TCP, .r.s = conn->b,
>  				  .r.p.tcp.tcp.splice = 1,
> -				  .r.p.tcp.tcp.index = conn - tc,
> -				  .r.p.tcp.tcp.v6 = CONN_V6(conn) };
> +				  .r.p.tcp.tcp.index = conn - tc, };

Commas.

>  	struct epoll_event ev_a = { .data.u64 = ref_a.u64 };
>  	struct epoll_event ev_b = { .data.u64 = ref_b.u64 };
>  	uint32_t events_a, events_b;
> @@ -579,12 +577,15 @@ void tcp_sock_handler_splice(struct ctx *c, union epoll_ref ref,
>  	struct tcp_splice_conn *conn;
>  
>  	if (ref.r.p.tcp.tcp.listen) {
> +		struct sockaddr sa;

Should this be sockaddr_storage in this case and then cast? I never
remember.

> +		socklen_t sl = sizeof(sa);
>  		int s;
>  
>  		if (c->tcp.splice_conn_count >= TCP_SPLICE_MAX_CONNS)
>  			return;
>  
> -		if ((s = accept4(ref.r.s, NULL, NULL, SOCK_NONBLOCK)) < 0)
> +		sa.sa_family = AF_UNSPEC; /* FIXME: stop clang-tidy complaining */
> +		if ((s = accept4(ref.r.s, &sa, &sl, SOCK_NONBLOCK)) < 0)

Same comment about /* FIXME */ as above.

This adds a copy_to_user() and microseconds, but it's unavoidable. I
tried harder to optimise for latency on the spliced path, that's why it
was NULL here and not above.

>  			return;
>  
>  		if (setsockopt(s, SOL_TCP, TCP_QUICKACK, &((int){ 1 }),
> @@ -595,7 +596,7 @@ void tcp_sock_handler_splice(struct ctx *c, union epoll_ref ref,
>  
>  		conn = CONN(c->tcp.splice_conn_count++);
>  		conn->a = s;
> -		conn->flags = ref.r.p.tcp.tcp.v6 ? SOCK_V6 : 0;
> +		conn->flags = (sa.sa_family == AF_INET6) ? SOCK_V6 : 0;
>  
>  		if (tcp_splice_new(c, conn, ref.r.p.tcp.tcp.index,
>  				   ref.r.p.tcp.tcp.outbound))

-- 
Stefano