[RFC 0/3] tcp: Improve mtu probe preconditions

6 messages, 2 authors, 2021-05-17 · open the first message on its own page

[RFC 0/3] tcp: Improve mtu probe preconditions

From: Leonard Crestez <hidden>
Date: 2021-05-11 12:04:42

According to RFC4821 Section 7.4 "Protocols MAY delay sending non-probes
in order to accumulate enough data" but in practice linux only sends
probes when a lot of data accumulates on the send side.

Bigget improvement is to make tcp_xmit_size_goal (normally used for TSO)
take the probe size into account. This makes probes more likely to be
sent when applications use short writes. This should introduce no delays
beyond existing autocork heuristics.

TCP RACK allows timely loss detection with fewer outstanding packets
than fast transmit, if enabled we can use this to shrink the probe size
and require much less data for probing.

Successive mtu probes will result in reducing the cwnd since it's
measured in packets and we send bigger packets. The cwnd value can get
stuck below 11 so rework the cwnd logic in tcp_mtu_probe to be based on
the number of packets that we actually need to send.

Signed-off-by: Leonard Crestez <redacted>

---

Previous RFCs:
* https://lore.kernel.org/netdev/d7fbf3d3a2490d0a9e99945593ada243da58e0f8.1619000255.git.cdleonard@gmail.com/
* https://lore.kernel.org/netdev/c575e693788233edeb399d8f9b6d9217b3daed9b.1619403511.git.lcrestez@drivenets.com/

The sysctls can probably be dropped, they're there for easy
experimentation.

It is possible that I misunderstood the meaning of "return 0 to wait"
from tcp_mtu_probe.

This introduces a bunch of mtu-to-mss calculations inside
tcp_xmit_size_goal which is called on every write. It might make sense
to cache the size of a pending probe inside icsk_mtup.probe_size. Right
now it's zero unless a probe is outstanding; a separate bit could be
used intead.

Leonard Crestez (3):
tcp: Consider mtu probing for tcp_xmit_size_goal
tcp: Use mtu probes if RACK is enabled
tcp: Adjust congestion window handling for mtu probe

base-commit: 3913ba732e972d88ebc391323999e780a9295852
--
2.25.1

[RFC 1/3] tcp: Consider mtu probing for tcp_xmit_size_goal

From: Leonard Crestez <hidden>
Date: 2021-05-11 12:04:47

According to RFC4821 Section 7.4 "Protocols MAY delay sending non-probes
in order to accumulate enough data" but linux almost never does that.

Linux checks for (probe_size + (1 + reorder) * mss_cache) bytes to be
available in the send buffer and if that condition is not met it will
send anyway using the current MSS. The feature can be made to work by
sending very large chunks of data from userspace (for example 128k) but
for small writes on fast links tcp mtu probes almost never happen.

This patch tries to take mtu probe into account in tcp_xmit_size_goal, a
function which otherwise attempts to accumulate a packet suitable for
TSO. No delays are introduced beyond existing autocork heuristics.

Suggested-by: Matt Mathis <redacted>
Signed-off-by: Leonard Crestez <redacted>
---
 Documentation/networking/ip-sysctl.rst |  5 ++++
 include/net/inet_connection_sock.h     |  4 ++-
 include/net/netns/ipv4.h               |  1 +
 include/net/tcp.h                      |  1 +
 net/ipv4/sysctl_net_ipv4.c             |  7 ++++++
 net/ipv4/tcp.c                         | 11 +++++++-
 net/ipv4/tcp_ipv4.c                    |  1 +
 net/ipv4/tcp_output.c                  | 35 +++++++++++++++++++++++---
 8 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index c2ecc9894fd0..108a5ee227d3 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst

@@ -320,10 +320,15 @@ tcp_mtu_probe_floor - INTEGER
 	If MTU probing is enabled this caps the minimum MSS used for search_low
 	for the connection.
 
 	Default : 48
 
+tcp_mtu_probe_autocork - BOOLEAN
+	Take into account mtu probe size when accumulating data via autocorking.
+
+	Default: 1
+
 tcp_min_snd_mss - INTEGER
 	TCP SYN and SYNACK messages usually advertise an ADVMSS option,
 	as described in RFC 1122 and RFC 6691.
 
 	If this ADVMSS option is smaller than tcp_min_snd_mss,

diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index 3c8c59471bc1..9a53d698c2e6 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h

@@ -123,11 +123,13 @@ struct inet_connection_sock {
 		/* Range of MTUs to search */
 		int		  search_high;
 		int		  search_low;
 
 		/* Information on the current probe. */
-		u32		  probe_size:31,
+		u32		  probe_size:30,
+		/* Are we actively accumulating data for an mtu probe? */
+				  wait_data:1,
 		/* Is the MTUP feature enabled for this connection? */
 				  enabled:1;
 
 		u32		  probe_timestamp;
 	} icsk_mtup;

diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index f6af8d96d3c6..3a2d8bf2b20a 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h

@@ -112,10 +112,11 @@ struct netns_ipv4 {
 #ifdef CONFIG_NET_L3_MASTER_DEV
 	u8 sysctl_tcp_l3mdev_accept;
 #endif
 	u8 sysctl_tcp_mtu_probing;
 	int sysctl_tcp_mtu_probe_floor;
+	int sysctl_tcp_mtu_probe_autocork;
 	int sysctl_tcp_base_mss;
 	int sysctl_tcp_min_snd_mss;
 	int sysctl_tcp_probe_threshold;
 	u32 sysctl_tcp_probe_interval;

diff --git a/include/net/tcp.h b/include/net/tcp.h
index d05193cb0d99..fb656490c901 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h

@@ -666,10 +666,11 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 void tcp_initialize_rcv_mss(struct sock *sk);
 
 int tcp_mtu_to_mss(struct sock *sk, int pmtu);
 int tcp_mss_to_mtu(struct sock *sk, int mss);
 void tcp_mtup_init(struct sock *sk);
+int tcp_mtu_probe_size_needed(struct sock *sk, int *probe_size);
 
 static inline void tcp_bound_rto(const struct sock *sk)
 {
 	if (inet_csk(sk)->icsk_rto > TCP_RTO_MAX)
 		inet_csk(sk)->icsk_rto = TCP_RTO_MAX;

diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index a62934b9f15a..e19176c17973 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c

@@ -827,10 +827,17 @@ static struct ctl_table ipv4_net_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &tcp_min_snd_mss_min,
 		.extra2		= &tcp_min_snd_mss_max,
 	},
+	{
+		.procname	= "tcp_mtu_probe_autocork",
+		.data		= &init_net.ipv4.sysctl_tcp_mtu_probe_autocork,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+	},
 	{
 		.procname	= "tcp_probe_threshold",
 		.data		= &init_net.ipv4.sysctl_tcp_probe_threshold,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index f1c1f9e3de72..23cfb2db28b4 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c

@@ -913,10 +913,11 @@ struct sk_buff *sk_stream_alloc_skb(struct sock *sk, int size, gfp_t gfp,
 }
 
 static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
 				       int large_allowed)
 {
+	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct tcp_sock *tp = tcp_sk(sk);
 	u32 new_size_goal, size_goal;
 
 	if (!large_allowed)
 		return mss_now;

@@ -932,11 +933,19 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
 		tp->gso_segs = min_t(u16, new_size_goal / mss_now,
 				     sk->sk_gso_max_segs);
 		size_goal = tp->gso_segs * mss_now;
 	}
 
-	return max(size_goal, mss_now);
+	size_goal = max(size_goal, mss_now);
+
+	if (unlikely(icsk->icsk_mtup.wait_data)) {
+		int mtu_probe_size_needed = tcp_mtu_probe_size_needed(sk, NULL);
+		if (mtu_probe_size_needed > 0)
+			size_goal = max(size_goal, (u32)mtu_probe_size_needed);
+	}
+
+	return size_goal;
 }
 
 int tcp_send_mss(struct sock *sk, int *size_goal, int flags)
 {
 	int mss_now;

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 312184cead57..7e75423c08c9 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c

@@ -2889,10 +2889,11 @@ static int __net_init tcp_sk_init(struct net *net)
 	net->ipv4.sysctl_tcp_base_mss = TCP_BASE_MSS;
 	net->ipv4.sysctl_tcp_min_snd_mss = TCP_MIN_SND_MSS;
 	net->ipv4.sysctl_tcp_probe_threshold = TCP_PROBE_THRESHOLD;
 	net->ipv4.sysctl_tcp_probe_interval = TCP_PROBE_INTERVAL;
 	net->ipv4.sysctl_tcp_mtu_probe_floor = TCP_MIN_SND_MSS;
+	net->ipv4.sysctl_tcp_mtu_probe_autocork = 1;
 
 	net->ipv4.sysctl_tcp_keepalive_time = TCP_KEEPALIVE_TIME;
 	net->ipv4.sysctl_tcp_keepalive_probes = TCP_KEEPALIVE_PROBES;
 	net->ipv4.sysctl_tcp_keepalive_intvl = TCP_KEEPALIVE_INTVL;

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index bde781f46b41..5a320d792ec4 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c

@@ -2311,10 +2311,31 @@ static bool tcp_can_coalesce_send_queue_head(struct sock *sk, int len)
 	}
 
 	return true;
 }
 
+/* Calculate the size of an MTU probe
+ * Probing the MTU requires one packets which is larger that current MSS as well
+ * as enough following mtu-sized packets to ensure that a probe loss can be
+ * detected without a full Retransmit Time out.
+ */
+int tcp_mtu_probe_size_needed(struct sock *sk, int *probe_size)
+{
+	struct inet_connection_sock *icsk = inet_csk(sk);
+	struct tcp_sock *tp = tcp_sk(sk);
+	int probe_size_val;
+	int size_needed;
+
+	/* This might be a little slow: */
+	probe_size_val = tcp_mtu_to_mss(sk, (icsk->icsk_mtup.search_high + icsk->icsk_mtup.search_low) >> 1);
+	if (probe_size)
+		*probe_size = probe_size_val;
+	size_needed = probe_size_val + (tp->reordering + 1) * tp->mss_cache;
+
+	return size_needed;
+}
+
 /* Create a new MTU probe if we are ready.
  * MTU probe is regularly attempting to increase the path MTU by
  * deliberately sending larger packets.  This discovers routing
  * changes resulting in larger path MTUs.
  *

@@ -2349,13 +2370,12 @@ static int tcp_mtu_probe(struct sock *sk)
 	/* Use binary search for probe_size between tcp_mss_base,
 	 * and current mss_clamp. if (search_high - search_low)
 	 * smaller than a threshold, backoff from probing.
 	 */
 	mss_now = tcp_current_mss(sk);
-	probe_size = tcp_mtu_to_mss(sk, (icsk->icsk_mtup.search_high +
-				    icsk->icsk_mtup.search_low) >> 1);
-	size_needed = probe_size + (tp->reordering + 1) * tp->mss_cache;
+	size_needed = tcp_mtu_probe_size_needed(sk, &probe_size);
+
 	interval = icsk->icsk_mtup.search_high - icsk->icsk_mtup.search_low;
 	/* When misfortune happens, we are reprobing actively,
 	 * and then reprobe timer has expired. We stick with current
 	 * probing process by not resetting search range to its orignal.
 	 */

@@ -2368,11 +2388,11 @@ static int tcp_mtu_probe(struct sock *sk)
 		return -1;
 	}
 
 	/* Have enough data in the send queue to probe? */
 	if (tp->write_seq - tp->snd_nxt < size_needed)
-		return -1;
+		return net->ipv4.sysctl_tcp_mtu_probe_autocork ? 0 : -1;
 
 	if (tp->snd_wnd < size_needed)
 		return -1;
 	if (after(tp->snd_nxt + size_needed, tcp_wnd_end(tp)))
 		return 0;

@@ -2596,11 +2616,13 @@ void tcp_chrono_stop(struct sock *sk, const enum tcp_chrono type)
  * but cannot send anything now because of SWS or another problem.
  */
 static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
 			   int push_one, gfp_t gfp)
 {
+	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct tcp_sock *tp = tcp_sk(sk);
+	struct net *net = sock_net(sk);
 	struct sk_buff *skb;
 	unsigned int tso_segs, sent_pkts;
 	int cwnd_quota;
 	int result;
 	bool is_cwnd_limited = false, is_rwnd_limited = false;

@@ -2611,13 +2633,18 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
 	tcp_mstamp_refresh(tp);
 	if (!push_one) {
 		/* Do MTU probing. */
 		result = tcp_mtu_probe(sk);
 		if (!result) {
+			if (net->ipv4.sysctl_tcp_mtu_probe_autocork)
+				icsk->icsk_mtup.wait_data = true;
 			return false;
 		} else if (result > 0) {
+			icsk->icsk_mtup.wait_data = false;
 			sent_pkts = 1;
+		} else {
+			icsk->icsk_mtup.wait_data = false;
 		}
 	}
 
 	max_segs = tcp_tso_segs(sk, mss_now);
 	while ((skb = tcp_send_head(sk))) {

-- 
2.25.1

[RFC 2/3] tcp: Use mtu probes if RACK is enabled

From: Leonard Crestez <hidden>
Date: 2021-05-11 12:04:50

RACK allows detecting a loss in min_rtt / 4 based on just one extra
packet. If enabled use this instead of relying of fast retransmit.

Suggested-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Leonard Crestez <redacted>
---
 Documentation/networking/ip-sysctl.rst |  5 +++++
 include/net/netns/ipv4.h               |  1 +
 net/ipv4/sysctl_net_ipv4.c             |  7 +++++++
 net/ipv4/tcp_ipv4.c                    |  1 +
 net/ipv4/tcp_output.c                  | 22 +++++++++++++++++++++-
 5 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 108a5ee227d3..4f6ac69f61e7 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst

@@ -325,10 +325,15 @@ tcp_mtu_probe_floor - INTEGER
 tcp_mtu_probe_autocork - BOOLEAN
 	Take into account mtu probe size when accumulating data via autocorking.
 
 	Default: 1
 
+tcp_mtu_probe_rack - BOOLEAN
+	Try to use shorter probes if RACK is also enabled
+
+	Default: 1
+
 tcp_min_snd_mss - INTEGER
 	TCP SYN and SYNACK messages usually advertise an ADVMSS option,
 	as described in RFC 1122 and RFC 6691.
 
 	If this ADVMSS option is smaller than tcp_min_snd_mss,

diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 3a2d8bf2b20a..298e65d8605c 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h

@@ -113,10 +113,11 @@ struct netns_ipv4 {
 	u8 sysctl_tcp_l3mdev_accept;
 #endif
 	u8 sysctl_tcp_mtu_probing;
 	int sysctl_tcp_mtu_probe_floor;
 	int sysctl_tcp_mtu_probe_autocork;
+	int sysctl_tcp_mtu_probe_rack;
 	int sysctl_tcp_base_mss;
 	int sysctl_tcp_min_snd_mss;
 	int sysctl_tcp_probe_threshold;
 	u32 sysctl_tcp_probe_interval;

diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index e19176c17973..f9366f35ff9c 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c

@@ -834,10 +834,17 @@ static struct ctl_table ipv4_net_table[] = {
 		.data		= &init_net.ipv4.sysctl_tcp_mtu_probe_autocork,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
+	{
+		.procname	= "tcp_mtu_probe_rack",
+		.data		= &init_net.ipv4.sysctl_tcp_mtu_probe_rack,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+	},
 	{
 		.procname	= "tcp_probe_threshold",
 		.data		= &init_net.ipv4.sysctl_tcp_probe_threshold,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 7e75423c08c9..4928fcd6e233 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c

@@ -2890,10 +2890,11 @@ static int __net_init tcp_sk_init(struct net *net)
 	net->ipv4.sysctl_tcp_min_snd_mss = TCP_MIN_SND_MSS;
 	net->ipv4.sysctl_tcp_probe_threshold = TCP_PROBE_THRESHOLD;
 	net->ipv4.sysctl_tcp_probe_interval = TCP_PROBE_INTERVAL;
 	net->ipv4.sysctl_tcp_mtu_probe_floor = TCP_MIN_SND_MSS;
 	net->ipv4.sysctl_tcp_mtu_probe_autocork = 1;
+	net->ipv4.sysctl_tcp_mtu_probe_rack = 1;
 
 	net->ipv4.sysctl_tcp_keepalive_time = TCP_KEEPALIVE_TIME;
 	net->ipv4.sysctl_tcp_keepalive_probes = TCP_KEEPALIVE_PROBES;
 	net->ipv4.sysctl_tcp_keepalive_intvl = TCP_KEEPALIVE_INTVL;

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5a320d792ec4..7cd1e8fd9749 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c

@@ -2311,27 +2311,47 @@ static bool tcp_can_coalesce_send_queue_head(struct sock *sk, int len)
 	}
 
 	return true;
 }
 
+static int tcp_mtu_probe_is_rack(const struct sock *sk)
+{
+	struct net *net = sock_net(sk);
+
+	return (net->ipv4.sysctl_tcp_recovery & TCP_RACK_LOSS_DETECTION &&
+			net->ipv4.sysctl_tcp_mtu_probe_rack);
+}
+
 /* Calculate the size of an MTU probe
  * Probing the MTU requires one packets which is larger that current MSS as well
  * as enough following mtu-sized packets to ensure that a probe loss can be
  * detected without a full Retransmit Time out.
  */
 int tcp_mtu_probe_size_needed(struct sock *sk, int *probe_size)
 {
 	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct tcp_sock *tp = tcp_sk(sk);
+	struct net *net = sock_net(sk);
 	int probe_size_val;
 	int size_needed;
 
 	/* This might be a little slow: */
 	probe_size_val = tcp_mtu_to_mss(sk, (icsk->icsk_mtup.search_high + icsk->icsk_mtup.search_low) >> 1);
 	if (probe_size)
 		*probe_size = probe_size_val;
-	size_needed = probe_size_val + (tp->reordering + 1) * tp->mss_cache;
+
+	if (tcp_mtu_probe_is_rack(sk)) {
+		/* RACK allows recovering in min_rtt / 4 based on just one extra packet
+		 * Use two to account for unrelated losses
+		 */
+		size_needed = probe_size_val + 2 * tp->mss_cache;
+	} else {
+		/* Without RACK send enough extra packets to trigger fast retransmit
+		 * This is dynamic DupThresh + 1
+		 */
+		size_needed = probe_size_val + (tp->reordering + 1) * tp->mss_cache;
+	}
 
 	return size_needed;
 }
 
 /* Create a new MTU probe if we are ready.

-- 
2.25.1

[RFC 3/3] tcp: Adjust congestion window handling for mtu probe

From: Leonard Crestez <hidden>
Date: 2021-05-11 12:04:53

When preparing an mtu probe linux checks for snd_cwnd >= 11 and for 2
more packets to fit alongside what is currently in flight. The reasoning
behind these constants is unclear. Replace this with checks based on the
required probe size:

* Skip probing if congestion window is too small to ever fit a probe.
* Wait for the congestion window to drain if too many packets are
already in flight.

This is very similar to snd_wnd logic except packets are counted instead
of bytes.

This patch will allow mtu probing at smaller cwnd values.

On very fast links after successive succesful MTU probes the cwnd
(measured in packets) shrinks and does not grow again because
tcp_is_cwnd_limited returns false. If snd_cwnd falls below then no more
probes are sent despite the link being otherwise idle.

Signed-off-by: Leonard Crestez <redacted>
---
 net/ipv4/tcp_output.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 7cd1e8fd9749..ccf3eb29e7a5 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c

@@ -2369,10 +2369,11 @@ static int tcp_mtu_probe(struct sock *sk)
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct sk_buff *skb, *nskb, *next;
 	struct net *net = sock_net(sk);
 	int probe_size;
 	int size_needed;
+	int packets_needed;
 	int copy, len;
 	int mss_now;
 	int interval;
 
 	/* Not currently probing/verifying,

@@ -2381,20 +2382,20 @@ static int tcp_mtu_probe(struct sock *sk)
 	 * not SACKing (the variable headers throw things off)
 	 */
 	if (likely(!icsk->icsk_mtup.enabled ||
 		   icsk->icsk_mtup.probe_size ||
 		   inet_csk(sk)->icsk_ca_state != TCP_CA_Open ||
-		   tp->snd_cwnd < 11 ||
 		   tp->rx_opt.num_sacks || tp->rx_opt.dsack))
 		return -1;
 
 	/* Use binary search for probe_size between tcp_mss_base,
 	 * and current mss_clamp. if (search_high - search_low)
 	 * smaller than a threshold, backoff from probing.
 	 */
 	mss_now = tcp_current_mss(sk);
 	size_needed = tcp_mtu_probe_size_needed(sk, &probe_size);
+	packets_needed = DIV_ROUND_UP(size_needed, tp->mss_cache);
 
 	interval = icsk->icsk_mtup.search_high - icsk->icsk_mtup.search_low;
 	/* When misfortune happens, we are reprobing actively,
 	 * and then reprobe timer has expired. We stick with current
 	 * probing process by not resetting search range to its orignal.

@@ -2406,26 +2407,26 @@ static int tcp_mtu_probe(struct sock *sk)
 		 */
 		tcp_mtu_check_reprobe(sk);
 		return -1;
 	}
 
+	/* Can probe fit inside snd_cwnd */
+	if (packets_needed > tp->snd_cwnd)
+		return -1;
+
 	/* Have enough data in the send queue to probe? */
 	if (tp->write_seq - tp->snd_nxt < size_needed)
 		return net->ipv4.sysctl_tcp_mtu_probe_autocork ? 0 : -1;
 
 	if (tp->snd_wnd < size_needed)
 		return -1;
 	if (after(tp->snd_nxt + size_needed, tcp_wnd_end(tp)))
 		return 0;
 
-	/* Do we need to wait to drain cwnd? With none in flight, don't stall */
-	if (tcp_packets_in_flight(tp) + 2 > tp->snd_cwnd) {
-		if (!tcp_packets_in_flight(tp))
-			return -1;
-		else
-			return 0;
-	}
+	/* Wait for snd_cwnd to drain */
+	if (tcp_packets_in_flight(tp) + packets_needed > tp->snd_cwnd)
+		return 0;
 
 	if (!tcp_can_coalesce_send_queue_head(sk, probe_size))
 		return -1;
 
 	/* We're allowed to probe.  Build it now. */

-- 
2.25.1

Re: [RFC 1/3] tcp: Consider mtu probing for tcp_xmit_size_goal

From: Eric Dumazet <edumazet@google.com>
Date: 2021-05-11 13:04:45

On Tue, May 11, 2021 at 2:04 PM Leonard Crestez [off-list ref] wrote:

According to RFC4821 Section 7.4 "Protocols MAY delay sending non-probes
in order to accumulate enough data" but linux almost never does that.

Linux checks for (probe_size + (1 + reorder) * mss_cache) bytes to be
available in the send buffer and if that condition is not met it will
send anyway using the current MSS. The feature can be made to work by
sending very large chunks of data from userspace (for example 128k) but
for small writes on fast links tcp mtu probes almost never happen.

Why should they happen ?

I am not sure the kernel should perform extra checks just because
applications are not properly written.

This patch tries to take mtu probe into account in tcp_xmit_size_goal, a
function which otherwise attempts to accumulate a packet suitable for
TSO. No delays are introduced beyond existing autocork heuristics.


MTU probing should not be attempted for every write().
This belongs to some kind of slow path, once in a while.

quoted hunk

Suggested-by: Matt Mathis <redacted>
Signed-off-by: Leonard Crestez <redacted>
---
 Documentation/networking/ip-sysctl.rst |  5 ++++
 include/net/inet_connection_sock.h     |  4 ++-
 include/net/netns/ipv4.h               |  1 +
 include/net/tcp.h                      |  1 +
 net/ipv4/sysctl_net_ipv4.c             |  7 ++++++
 net/ipv4/tcp.c                         | 11 +++++++-
 net/ipv4/tcp_ipv4.c                    |  1 +
 net/ipv4/tcp_output.c                  | 35 +++++++++++++++++++++++---
 8 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index c2ecc9894fd0..108a5ee227d3 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst

@@ -320,10 +320,15 @@ tcp_mtu_probe_floor - INTEGER
        If MTU probing is enabled this caps the minimum MSS used for search_low
        for the connection.

        Default : 48

+tcp_mtu_probe_autocork - BOOLEAN
+       Take into account mtu probe size when accumulating data via autocorking.
+
+       Default: 1
+
 tcp_min_snd_mss - INTEGER
        TCP SYN and SYNACK messages usually advertise an ADVMSS option,
        as described in RFC 1122 and RFC 6691.

        If this ADVMSS option is smaller than tcp_min_snd_mss,

diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index 3c8c59471bc1..9a53d698c2e6 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h

@@ -123,11 +123,13 @@ struct inet_connection_sock {
                /* Range of MTUs to search */
                int               search_high;
                int               search_low;

                /* Information on the current probe. */
-               u32               probe_size:31,
+               u32               probe_size:30,
+               /* Are we actively accumulating data for an mtu probe? */
+                                 wait_data:1,
                /* Is the MTUP feature enabled for this connection? */
                                  enabled:1;

                u32               probe_timestamp;
        } icsk_mtup;

diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index f6af8d96d3c6..3a2d8bf2b20a 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h

@@ -112,10 +112,11 @@ struct netns_ipv4 {
 #ifdef CONFIG_NET_L3_MASTER_DEV
        u8 sysctl_tcp_l3mdev_accept;
 #endif
        u8 sysctl_tcp_mtu_probing;
        int sysctl_tcp_mtu_probe_floor;
+       int sysctl_tcp_mtu_probe_autocork;
        int sysctl_tcp_base_mss;
        int sysctl_tcp_min_snd_mss;
        int sysctl_tcp_probe_threshold;
        u32 sysctl_tcp_probe_interval;

diff --git a/include/net/tcp.h b/include/net/tcp.h
index d05193cb0d99..fb656490c901 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h

@@ -666,10 +666,11 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 void tcp_initialize_rcv_mss(struct sock *sk);

 int tcp_mtu_to_mss(struct sock *sk, int pmtu);
 int tcp_mss_to_mtu(struct sock *sk, int mss);
 void tcp_mtup_init(struct sock *sk);
+int tcp_mtu_probe_size_needed(struct sock *sk, int *probe_size);

 static inline void tcp_bound_rto(const struct sock *sk)
 {
        if (inet_csk(sk)->icsk_rto > TCP_RTO_MAX)
                inet_csk(sk)->icsk_rto = TCP_RTO_MAX;

diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index a62934b9f15a..e19176c17973 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c

@@ -827,10 +827,17 @@ static struct ctl_table ipv4_net_table[] = {
                .mode           = 0644,
                .proc_handler   = proc_dointvec_minmax,
                .extra1         = &tcp_min_snd_mss_min,
                .extra2         = &tcp_min_snd_mss_max,
        },
+       {
+               .procname       = "tcp_mtu_probe_autocork",
+               .data           = &init_net.ipv4.sysctl_tcp_mtu_probe_autocork,
+               .maxlen         = sizeof(int),
+               .mode           = 0644,
+               .proc_handler   = proc_dointvec,
+       },
        {
                .procname       = "tcp_probe_threshold",
                .data           = &init_net.ipv4.sysctl_tcp_probe_threshold,
                .maxlen         = sizeof(int),
                .mode           = 0644,

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index f1c1f9e3de72..23cfb2db28b4 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c

@@ -913,10 +913,11 @@ struct sk_buff *sk_stream_alloc_skb(struct sock *sk, int size, gfp_t gfp,
 }

 static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
                                       int large_allowed)
 {
+       struct inet_connection_sock *icsk = inet_csk(sk);
        struct tcp_sock *tp = tcp_sk(sk);
        u32 new_size_goal, size_goal;

        if (!large_allowed)
                return mss_now;

@@ -932,11 +933,19 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
                tp->gso_segs = min_t(u16, new_size_goal / mss_now,
                                     sk->sk_gso_max_segs);
                size_goal = tp->gso_segs * mss_now;
        }

-       return max(size_goal, mss_now);
+       size_goal = max(size_goal, mss_now);
+
+       if (unlikely(icsk->icsk_mtup.wait_data)) {
+               int mtu_probe_size_needed = tcp_mtu_probe_size_needed(sk, NULL);
+               if (mtu_probe_size_needed > 0)
+                       size_goal = max(size_goal, (u32)mtu_probe_size_needed);
+       }


I think you are mistaken.

This function usually returns 64KB depending on MSS.
 Have you really tested this part ?

Also adding in TCP fast path some tweaks for MTU probing while
applications are performing small writes
is not acceptable.

quoted hunk

+
+       return size_goal;
 }

 int tcp_send_mss(struct sock *sk, int *size_goal, int flags)
 {
        int mss_now;

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 312184cead57..7e75423c08c9 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c

@@ -2889,10 +2889,11 @@ static int __net_init tcp_sk_init(struct net *net)
        net->ipv4.sysctl_tcp_base_mss = TCP_BASE_MSS;
        net->ipv4.sysctl_tcp_min_snd_mss = TCP_MIN_SND_MSS;
        net->ipv4.sysctl_tcp_probe_threshold = TCP_PROBE_THRESHOLD;
        net->ipv4.sysctl_tcp_probe_interval = TCP_PROBE_INTERVAL;
        net->ipv4.sysctl_tcp_mtu_probe_floor = TCP_MIN_SND_MSS;
+       net->ipv4.sysctl_tcp_mtu_probe_autocork = 1;

        net->ipv4.sysctl_tcp_keepalive_time = TCP_KEEPALIVE_TIME;
        net->ipv4.sysctl_tcp_keepalive_probes = TCP_KEEPALIVE_PROBES;
        net->ipv4.sysctl_tcp_keepalive_intvl = TCP_KEEPALIVE_INTVL;

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index bde781f46b41..5a320d792ec4 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c

@@ -2311,10 +2311,31 @@ static bool tcp_can_coalesce_send_queue_head(struct sock *sk, int len)
        }

        return true;
 }

+/* Calculate the size of an MTU probe
+ * Probing the MTU requires one packets which is larger that current MSS as well
+ * as enough following mtu-sized packets to ensure that a probe loss can be
+ * detected without a full Retransmit Time out.
+ */
+int tcp_mtu_probe_size_needed(struct sock *sk, int *probe_size)
+{
+       struct inet_connection_sock *icsk = inet_csk(sk);
+       struct tcp_sock *tp = tcp_sk(sk);
+       int probe_size_val;
+       int size_needed;
+
+       /* This might be a little slow: */
+       probe_size_val = tcp_mtu_to_mss(sk, (icsk->icsk_mtup.search_high + icsk->icsk_mtup.search_low) >> 1);
+       if (probe_size)
+               *probe_size = probe_size_val;
+       size_needed = probe_size_val + (tp->reordering + 1) * tp->mss_cache;
+
+       return size_needed;
+}
+
 /* Create a new MTU probe if we are ready.
  * MTU probe is regularly attempting to increase the path MTU by
  * deliberately sending larger packets.  This discovers routing
  * changes resulting in larger path MTUs.
  *

@@ -2349,13 +2370,12 @@ static int tcp_mtu_probe(struct sock *sk)
        /* Use binary search for probe_size between tcp_mss_base,
         * and current mss_clamp. if (search_high - search_low)
         * smaller than a threshold, backoff from probing.
         */
        mss_now = tcp_current_mss(sk);
-       probe_size = tcp_mtu_to_mss(sk, (icsk->icsk_mtup.search_high +
-                                   icsk->icsk_mtup.search_low) >> 1);
-       size_needed = probe_size + (tp->reordering + 1) * tp->mss_cache;
+       size_needed = tcp_mtu_probe_size_needed(sk, &probe_size);
+
        interval = icsk->icsk_mtup.search_high - icsk->icsk_mtup.search_low;
        /* When misfortune happens, we are reprobing actively,
         * and then reprobe timer has expired. We stick with current
         * probing process by not resetting search range to its orignal.
         */

@@ -2368,11 +2388,11 @@ static int tcp_mtu_probe(struct sock *sk)
                return -1;
        }

        /* Have enough data in the send queue to probe? */
        if (tp->write_seq - tp->snd_nxt < size_needed)
-               return -1;
+               return net->ipv4.sysctl_tcp_mtu_probe_autocork ? 0 : -1;

        if (tp->snd_wnd < size_needed)
                return -1;
        if (after(tp->snd_nxt + size_needed, tcp_wnd_end(tp)))
                return 0;

@@ -2596,11 +2616,13 @@ void tcp_chrono_stop(struct sock *sk, const enum tcp_chrono type)
  * but cannot send anything now because of SWS or another problem.
  */
 static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
                           int push_one, gfp_t gfp)
 {
+       struct inet_connection_sock *icsk = inet_csk(sk);
        struct tcp_sock *tp = tcp_sk(sk);
+       struct net *net = sock_net(sk);
        struct sk_buff *skb;
        unsigned int tso_segs, sent_pkts;
        int cwnd_quota;
        int result;
        bool is_cwnd_limited = false, is_rwnd_limited = false;

@@ -2611,13 +2633,18 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
        tcp_mstamp_refresh(tp);
        if (!push_one) {
                /* Do MTU probing. */
                result = tcp_mtu_probe(sk);
                if (!result) {
+                       if (net->ipv4.sysctl_tcp_mtu_probe_autocork)
+                               icsk->icsk_mtup.wait_data = true;
                        return false;
                } else if (result > 0) {
+                       icsk->icsk_mtup.wait_data = false;
                        sent_pkts = 1;
+               } else {
+                       icsk->icsk_mtup.wait_data = false;
                }
        }

        max_segs = tcp_tso_segs(sk, mss_now);
        while ((skb = tcp_send_head(sk))) {
--

2.25.1

Re: [RFC 1/3] tcp: Consider mtu probing for tcp_xmit_size_goal

From: Leonard Crestez <hidden>
Date: 2021-05-17 13:42:46

On 5/11/21 4:04 PM, Eric Dumazet wrote:

On Tue, May 11, 2021 at 2:04 PM Leonard Crestez [off-list ref] wrote:

quoted

According to RFC4821 Section 7.4 "Protocols MAY delay sending non-probes
in order to accumulate enough data" but linux almost never does that.

Linux checks for (probe_size + (1 + reorder) * mss_cache) bytes to be
available in the send buffer and if that condition is not met it will
send anyway using the current MSS. The feature can be made to work by
sending very large chunks of data from userspace (for example 128k) but
for small writes on fast links tcp mtu probes almost never happen.

Why should they happen ?

I am not sure the kernel should perform extra checks just because
applications are not properly written.

My tests show that application writing a few kb at a time almost never 
trigger MTU probing enough to reach 9200. The reasons for this are very 
difficult for me to understand.

It seems that only writing in very large chunks like 160k makes it 
happen, much more than the size_needed calculated inside tcp_mtu_probing 
(which is about 50k). This seems unreasonable. Ideally linux should try 
to accumulate enough data for a probe (as the RFC suggests) but at least 
it should send probes that fit inside a single userspace write.

I dug a little deeper and what seems to happen is this:

  * size_needed is ~60k
  * once the head of the queue reached size_needed tcp_push_one is 
called which sends everything ignoring MTU probing
  * size_needed is reached again and tcp_push_pending_frames is called. 
At this point the cwnd has shrunk < 11 (due to the previous burst) so 
probing is skipped again in favor of just sending in mss-sized chunks.

This happens repeatedly, a sender-limited app performing periodic 128k 
writes will see MSS stuck below MTU.

I don't understand the push_one logic and why it completely skips mtu 
probing, it seems like an optimization which doesn't take RFC4821 into 
account.

quoted

This patch tries to take mtu probe into account in tcp_xmit_size_goal, a
function which otherwise attempts to accumulate a packet suitable for
TSO. No delays are introduced beyond existing autocork heuristics.


MTU probing should not be attempted for every write().
This belongs to some kind of slow path, once in a while.

MTU probing is only attempted every 10 minutes but once a probe is 
pending it does have a slight impact on every write. This is already the 
case, tcp_write_xmit calls tcp_mtu_probe almost every time.

I had an idea for reducing the overhead in tcp_size_needed but it turns 
out I was indeed mistaken about what this function does. I thought it 
returned ~mss when all GSO is disabled but this is not so.

quoted

  static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
                                        int large_allowed)
  {
+       struct inet_connection_sock *icsk = inet_csk(sk);
         struct tcp_sock *tp = tcp_sk(sk);
         u32 new_size_goal, size_goal;

         if (!large_allowed)
                 return mss_now;

@@ -932,11 +933,19 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
                 tp->gso_segs = min_t(u16, new_size_goal / mss_now,
                                      sk->sk_gso_max_segs);
                 size_goal = tp->gso_segs * mss_now;
         }

-       return max(size_goal, mss_now);
+       size_goal = max(size_goal, mss_now);
+
+       if (unlikely(icsk->icsk_mtup.wait_data)) {
+               int mtu_probe_size_needed = tcp_mtu_probe_size_needed(sk, NULL);
+               if (mtu_probe_size_needed > 0)
+                       size_goal = max(size_goal, (u32)mtu_probe_size_needed);
+       }


I think you are mistaken.

This function usually returns 64KB depending on MSS.
  Have you really tested this part ?

I assumed that with all gso features disabled this function returns one 
MSS but this is not true. My patch had a positive effect just because I 
made tcp_mtu_probing return "0" instead of "-1" if not enough data is 
queued.

I don't fully understand the implications of that change though. If 
tcp_mtu_probe returns zero what guarantee is there that data will 
eventually be sent even if no further userspace writes happen?

I'd welcome any suggestions.

--
Regards,
Leonard

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help