Re: [PATCH 32/32] tcp: Use dual stack sockets for port forwarding when possible

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 16 Nov 2022 15:42:12 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Platforms like Linux allow IPv6 sockets to listen for IPv4 connections as
> well as native IPv6 connections.  By doing this we halve the number of
> listening sockets we need for TCP (assuming passt/pasta is listening on the
> same ports for IPv4 and IPv6).  When forwarding many ports (e.g. -t all)
> this can significantly reduce the amount of kernel memory that passt
> consumes.
> 
> When forwarding all TCP and UDP ports for both IPv4 and IPv6 (-t all
> -u all), this reduces kernel memory usage from ~677MiB to ~487MiB
> (kernel version 6.0.8 on Fedora 37, x86_64).

Oh, nice, that's quite significant.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 616b9d0..5860c9f 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2991,8 +2991,12 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
>  
>  	s = sock_l4(c, af, IPPROTO_TCP, addr, ifname, port, tref.u32);
>  
> -	if (c->tcp.fwd_in.mode == FWD_AUTO)
> -		tcp_sock_init_ext[port][(af == AF_INET) ? V4 : V6] = s;
> +	if (c->tcp.fwd_in.mode == FWD_AUTO) {
> +		if (af == AF_INET || af == AF_UNSPEC)
> +			tcp_sock_init_ext[port][V4] = s;
> +		if (af == AF_INET6 || af == AF_UNSPEC)

Nit: you could align the || af == AF_UNSPEC above with an extra
whitespace (as it's done in the context below).

> +			tcp_sock_init_ext[port][V6] = s;
> +	}
>  
>  	if (s < 0)
>  		return -1;
> @@ -3012,6 +3016,12 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
>  void tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
>  		   const char *ifname, in_port_t port)
>  {
> +	if (af == AF_UNSPEC && c->ifi4 && c->ifi6)
> +		/* Attempt to get a dual stack socket */
> +		if (tcp_sock_init_af(c, AF_UNSPEC, port, addr, ifname) >= 0)
> +			return;
> +
> +	/* Otherwise create a socket per IP version */

...this looks surprisingly clean by the way, at least much cleaner than
I expected.

>  	if ((af == AF_INET  || af == AF_UNSPEC) && c->ifi4)
>  		tcp_sock_init_af(c, AF_INET, port, addr, ifname);
>  	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6)

I just finished reviewing this series, in general it looks great to me,
I would have another look (and test!) on Thursday -- either using this
version or a re-spin.

-- 
Stefano