Message-ID: <20230313200601.46663-1-kuniyu@amazon.com>
Date:   Mon, 13 Mar 2023 13:06:01 -0700
From:   Kuniyuki Iwashima <kuniyu@...zon.com>
To:     <mfreemon@...udflare.com>
CC:     <kernel-team@...udflare.com>, <netdev@...r.kernel.org>,
        <kuniyu@...zon.com>
Subject: Re: [RFC PATCH v2] Add a sysctl to allow TCP window shrinking in order to honor memory limits

From:   Mike Freemon <mfreemon@...udflare.com>
Date:   Tue,  7 Mar 2023 23:33:53 -0600
> From: "mfreemon@...udflare.com" <mfreemon@...udflare.com>
> 
> Under certain circumstances, the tcp receive buffer memory limit
> set by autotuning is ignored, and the receive buffer can grow
> unrestrained until it reaches tcp_rmem[2].
> 
> To reproduce:  Connect a TCP session with the receiver doing
> nothing and the sender sending small packets (an infinite loop
> of socket send() with 4 bytes of payload with a sleep of 1 ms
> in between each send()).  This will fill the tcp receive buffer
> all the way to tcp_rmem[2], ignoring the autotuning limit
> (sk_rcvbuf).
> 
> As a result, a host can have individual tcp sessions with receive
> buffers of size tcp_rmem[2], and the host itself can reach tcp_mem
> limits, causing the host to go into tcp memory pressure mode.
> 
> The fundamental issue is the relationship between the granularity
> of the window scaling factor and the number of byte ACKed back
> to the sender.  This problem has previously been identified in
> RFC 7323, appendix F [1].
> 
> The Linux kernel currently adheres to never shrinking the window.
> 
> In addition to the overallocation of memory mentioned above, this
> is also functionally incorrect, because once tcp_rmem[2] is
> reached, the receiver will drop in-window packets resulting in
> retransmissions and an eventual timeout of the tcp session.  A
> receive buffer full condition should instead result in a zero
> window and an indefinite wait.
> 
> In practice, this problem is largely hidden for most flows.  It
> is not applicable to mice flows.  Elephant flows can send data
> fast enough to "overrun" the sk_rcvbuf limit (in a single ACK),
> triggering a zero window.
> 
> But this problem does show up for other types of flows.  Good
> examples are websockets and other type of flows that send small
> amounts of data spaced apart slightly in time.  In these cases,
> we directly encounter the problem described in [1].
> 
> RFC 7323, section 2.4 [2], says there are instances when a retracted
> window can be offered, and that TCP implementations MUST ensure
> that they handle a shrinking window, as specified in RFC 1122,
> section 4.2.2.16 [3].  All prior RFCs on the topic of tcp window
> management have made clear that sender must accept a shrunk window
> from the receiver, including RFC 793 [4] and RFC 1323 [5].
> 
> This patch implements the functionality to shrink the tcp window
> when necessary to keep the right edge within the memory limit set
> by autotuning (sk_rcvbuf).  This new functionality is enabled by
> setting the sysctl net.ipv4.tcp_shrink_window to 1.
> 
> [1] https://www.rfc-editor.org/rfc/rfc7323#appendix-F
> [2] https://www.rfc-editor.org/rfc/rfc7323#section-2.4
> [3] https://www.rfc-editor.org/rfc/rfc1122#page-91
> [4] https://www.rfc-editor.org/rfc/rfc793
> [5] https://www.rfc-editor.org/rfc/rfc1323
> 
> Signed-off-by: Mike Freemon <mfreemon@...udflare.com>
> ---
>  Documentation/networking/ip-sysctl.rst | 14 ++++++
>  include/net/netns/ipv4.h               |  2 +
>  net/ipv4/sysctl_net_ipv4.c             |  7 +++
>  net/ipv4/tcp_ipv4.c                    |  2 +
>  net/ipv4/tcp_output.c                  | 59 +++++++++++++++++++-------
>  5 files changed, 69 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index 87dd1c5283e6..67dfcadfe350 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -968,6 +968,20 @@ tcp_tw_reuse - INTEGER
>  tcp_window_scaling - BOOLEAN
>  	Enable window scaling as defined in RFC1323.
>  
> +tcp_shrink_window - BOOLEAN
> +	This changes how the TCP receive window is calculated when window
> +	scaling is in effect.
> +
> +	RFC 7323, section 2.4, says there are instances when a retracted
> +	window can be offered, and that TCP implementations MUST ensure
> +	that they handle a shrinking window, as specified in RFC 1122.
> +
> +	- 0 - Disabled.	The window is never shrunk.
> +	- 1 - Enabled.	The window is shrunk when necessary to remain within
> +					the memory limit set by autotuning (sk_rcvbuf).
> +
> +	Default: 0
> +
>  tcp_wmem - vector of 3 INTEGERs: min, default, max
>  	min: Amount of memory reserved for send buffers for TCP sockets.
>  	Each TCP socket has rights to use it due to fact of its birth.
> diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
> index db762e35aca9..fbc67afac75a 100644
> --- a/include/net/netns/ipv4.h
> +++ b/include/net/netns/ipv4.h
> @@ -237,5 +237,7 @@ struct netns_ipv4 {
>  
>  	atomic_t	rt_genid;
>  	siphash_key_t	ip_id_key;
> +
> +	unsigned int sysctl_tcp_shrink_window;

u8 can be used instead.
Also, please try filling a (hot if appropriate) hole.

  $ pahole -C netns_ipv4 net/ipv4/sysctl_net_ipv4.o


>  };
>  #endif
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index 0d0cc4ef2b85..c6d181f79534 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -1467,6 +1467,13 @@ static struct ctl_table ipv4_net_table[] = {
>  		.extra1         = SYSCTL_ZERO,
>  		.extra2         = &tcp_plb_max_cong_thresh,
>  	},
> +	{
> +		.procname	= "tcp_shrink_window",
> +		.data		= &init_net.ipv4.sysctl_tcp_shrink_window,
> +		.maxlen		= sizeof(unsigned int),

s/unsigned int/u8/

> +		.mode		= 0644,
> +		.proc_handler	= proc_douintvec_minmax,

You can use u8 handler with min/max.

		.proc_handler   = proc_dou8vec_minmax,
		.extra1         = SYSCTL_ZERO,
                .extra2         = SYSCTL_ONE,

> +	},
>  	{ }
>  };
>  
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index ea370afa70ed..d976f01413d7 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -3275,6 +3275,8 @@ static int __net_init tcp_sk_init(struct net *net)
>  	else
>  		net->ipv4.tcp_congestion_control = &tcp_reno;
>  
> +	net->ipv4.sysctl_tcp_shrink_window = 0;
> +
>  	return 0;
>  }
>  
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 71d01cf3c13e..7f7a96e797fa 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -260,8 +260,8 @@ static u16 tcp_select_window(struct sock *sk)
>  	u32 old_win = tp->rcv_wnd;
>  	u32 cur_win = tcp_receive_window(tp);
>  	u32 new_win = __tcp_select_window(sk);
> +	struct net *net = sock_net(sk);
>  
> -	/* Never shrink the offered window */
>  	if (new_win < cur_win) {
>  		/* Danger Will Robinson!
>  		 * Don't update rcv_wup/rcv_wnd here or else
> @@ -270,11 +270,15 @@ static u16 tcp_select_window(struct sock *sk)
>  		 *
>  		 * Relax Will Robinson.
>  		 */
> -		if (new_win == 0)
> -			NET_INC_STATS(sock_net(sk),
> -				      LINUX_MIB_TCPWANTZEROWINDOWADV);
> -		new_win = ALIGN(cur_win, 1 << tp->rx_opt.rcv_wscale);
> +		if (!net->ipv4.sysctl_tcp_shrink_window) {

Let's use READ_ONCE() to silence KCSAN when reading sysctl knobs.
Same for other readers.

Thanks,
Kuniyuki


> +			/* Never shrink the offered window */
> +			if (new_win == 0)
> +				NET_INC_STATS(sock_net(sk),
> +					      LINUX_MIB_TCPWANTZEROWINDOWADV);
> +			new_win = ALIGN(cur_win, 1 << tp->rx_opt.rcv_wscale);
> +		}
>  	}
> +
>  	tp->rcv_wnd = new_win;
>  	tp->rcv_wup = tp->rcv_nxt;
>  
> @@ -2947,6 +2951,7 @@ u32 __tcp_select_window(struct sock *sk)
>  {
>  	struct inet_connection_sock *icsk = inet_csk(sk);
>  	struct tcp_sock *tp = tcp_sk(sk);
> +	struct net *net = sock_net(sk);
>  	/* MSS for the peer's data.  Previous versions used mss_clamp
>  	 * here.  I don't know if the value based on our guesses
>  	 * of peer's MSS is better for the performance.  It's more correct
> @@ -2968,16 +2973,24 @@ u32 __tcp_select_window(struct sock *sk)
>  		if (mss <= 0)
>  			return 0;
>  	}
> +
> +	if (net->ipv4.sysctl_tcp_shrink_window) {
> +		/* new window should always be an exact multiple of scaling factor */
> +		free_space = round_down(free_space, 1 << tp->rx_opt.rcv_wscale);
> +	}
> +
>  	if (free_space < (full_space >> 1)) {
>  		icsk->icsk_ack.quick = 0;
>  
>  		if (tcp_under_memory_pressure(sk))
>  			tcp_adjust_rcv_ssthresh(sk);
>  
> -		/* free_space might become our new window, make sure we don't
> -		 * increase it due to wscale.
> -		 */
> -		free_space = round_down(free_space, 1 << tp->rx_opt.rcv_wscale);
> +		if (!net->ipv4.sysctl_tcp_shrink_window) {
> +			/* free_space might become our new window, make sure we don't
> +			 * increase it due to wscale.
> +			 */
> +			free_space = round_down(free_space, 1 << tp->rx_opt.rcv_wscale);
> +		}
>  
>  		/* if free space is less than mss estimate, or is below 1/16th
>  		 * of the maximum allowed, try to move to zero-window, else
> @@ -2988,10 +3001,24 @@ u32 __tcp_select_window(struct sock *sk)
>  		 */
>  		if (free_space < (allowed_space >> 4) || free_space < mss)
>  			return 0;
> +
> +		if (net->ipv4.sysctl_tcp_shrink_window && free_space < (1 << tp->rx_opt.rcv_wscale))
> +			return 0;
>  	}
>  
> -	if (free_space > tp->rcv_ssthresh)
> +	if (free_space > tp->rcv_ssthresh) {
>  		free_space = tp->rcv_ssthresh;
> +		if (net->ipv4.sysctl_tcp_shrink_window) {
> +			/* new window should always be an exact multiple of scaling factor
> +			 *
> +			 * For this case, we ALIGN "up" (increase free_space) because
> +			 * we know free_space is not zero here, it has been reduced from
> +			 * the memory-based limit, and rcv_ssthresh is not a hard limit
> +			 * (unlike sk_rcvbuf).
> +			 */
> +			free_space = ALIGN(free_space, (1 << tp->rx_opt.rcv_wscale));
> +		}
> +	}
>  
>  	/* Don't do rounding if we are using window scaling, since the
>  	 * scaled window will not line up with the MSS boundary anyway.
> @@ -2999,11 +3026,13 @@ u32 __tcp_select_window(struct sock *sk)
>  	if (tp->rx_opt.rcv_wscale) {
>  		window = free_space;
>  
> -		/* Advertise enough space so that it won't get scaled away.
> -		 * Import case: prevent zero window announcement if
> -		 * 1<<rcv_wscale > mss.
> -		 */
> -		window = ALIGN(window, (1 << tp->rx_opt.rcv_wscale));
> +		if (!net->ipv4.sysctl_tcp_shrink_window) {
> +			/* Advertise enough space so that it won't get scaled away.
> +			 * Import case: prevent zero window announcement if
> +			 * 1<<rcv_wscale > mss.
> +			 */
> +			window = ALIGN(window, (1 << tp->rx_opt.rcv_wscale));
> +		}
>  	} else {
>  		window = tp->rcv_wnd;
>  		/* Get the largest window that is a nice multiple of mss.
> -- 
> 2.39.2

Powered by blists - more mailing lists