[PATCH net-next 0/2] sctp: make the PLPMTUD probe more effective and efficient

As David Laight noticed, it currently takes quite some time to find
the optimal pmtu in the Search state, and also lacks the black hole
detection in the Search Complete state. This patchset is to address
them to mke the PLPMTUD probe more effective and efficient.

Xin Long (2):
  sctp: do black hole detection in search complete state
  sctp: send the next probe immediately once the last one is acked

 Documentation/networking/ip-sysctl.rst | 12 ++++++++----
 include/net/sctp/structs.h             |  3 ++-
 net/sctp/sm_statefuns.c                |  5 ++++-
 net/sctp/transport.c                   | 11 ++++-------
 4 files changed, 18 insertions(+), 13 deletions(-)

-- 
2.27.0

Currently the PLPMUTD probe will stop for a long period (interval * 30)
after it enters search complete state. If there's a pmtu change on the
route path, it takes a long time to be aware if the ICMP TooBig packet
is lost or filtered.

As it says in rfc8899#section-4.3:

  "A DPLPMTUD method MUST NOT rely solely on this method."
  (ICMP PTB message).

This patch is to enable the other method for search complete state:

  "A PL can use the DPLPMTUD probing mechanism to periodically
   generate probe packets of the size of the current PLPMTU."

With this patch, the probe will continue with the current pmtu every
'interval' until the PMTU_RAISE_TIMER 'timeout', which we implement
by adding raise_count to raise the probe size when it counts to 30
and removing the SCTP_PL_COMPLETE check for PMTU_RAISE_TIMER.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/structs.h |  3 ++-
 net/sctp/transport.c       | 11 ++++-------
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 9eaa701cda23..c4a4c1754be8 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h

@@ -987,7 +987,8 @@ struct sctp_transport {
 		__u16 pmtu;
 		__u16 probe_size;
 		__u16 probe_high;
-		__u8 probe_count;
+		__u8 probe_count:3;
+		__u8 raise_count:5;
 		__u8 state;
 	} pl; /* plpmtud related */
 

diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index f27b856ea8ce..5f23804f21c7 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c

@@ -213,15 +213,10 @@ void sctp_transport_reset_reconf_timer(struct sctp_transport *transport)
 
 void sctp_transport_reset_probe_timer(struct sctp_transport *transport)
 {
-	int scale = 1;
-
 	if (timer_pending(&transport->probe_timer))
 		return;
-	if (transport->pl.state == SCTP_PL_COMPLETE &&
-	    transport->pl.probe_count == 1)
-		scale = 30; /* works as PMTU_RAISE_TIMER */
 	if (!mod_timer(&transport->probe_timer,
-		       jiffies + transport->probe_interval * scale))
+		       jiffies + transport->probe_interval))
 		sctp_transport_hold(transport);
 }
 

@@ -333,13 +328,15 @@ void sctp_transport_pl_recv(struct sctp_transport *t)
 		t->pl.probe_size += SCTP_PL_MIN_STEP;
 		if (t->pl.probe_size >= t->pl.probe_high) {
 			t->pl.probe_high = 0;
+			t->pl.raise_count = 0;
 			t->pl.state = SCTP_PL_COMPLETE; /* Search -> Search Complete */
 
 			t->pl.probe_size = t->pl.pmtu;
 			t->pathmtu = t->pl.pmtu + sctp_transport_pl_hlen(t);
 			sctp_assoc_sync_pmtu(t->asoc);
 		}
-	} else if (t->pl.state == SCTP_PL_COMPLETE) {
+	} else if (t->pl.state == SCTP_PL_COMPLETE && ++t->pl.raise_count == 30) {
+		/* Raise probe_size again after 30 * interval in Search Complete */
 		t->pl.state = SCTP_PL_SEARCH; /* Search Complete -> Search */
 		t->pl.probe_size += SCTP_PL_MIN_STEP;
 	}

-- 
2.27.0

These is no need to wait for 'interval' period for the next probe
if the last probe is already acked in search state. The 'interval'
period waiting should be only for probe failure timeout and the
current pmtu check when it's in search complete state.

This change will shorten the probe time a lot in search state, and
also fix the document accordingly.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 Documentation/networking/ip-sysctl.rst | 12 ++++++++----
 net/sctp/sm_statefuns.c                |  5 ++++-
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 8bff728b3a1e..b3fa522e4cd9 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst

@@ -2835,10 +2835,14 @@ encap_port - INTEGER
 	Default: 0
 
 plpmtud_probe_interval - INTEGER
-        The time interval (in milliseconds) for sending PLPMTUD probe chunks.
-        These chunks are sent at the specified interval with a variable size
-        to probe the mtu of a given path between 2 endpoints. PLPMTUD will
-        be disabled when 0 is set, and other values for it must be >= 5000.
+        The time interval (in milliseconds) for the PLPMTUD probe timer,
+        which is configured to expire after this period to receive an
+        acknowledgment to a probe packet. This is also the time interval
+        between the probes for the current pmtu when the probe search
+        is done.
+
+        PLPMTUD will be disabled when 0 is set, and other values for it
+        must be >= 5000.
 
 	Default: 0
 

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index d29b579da904..09a8f23ec709 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c

@@ -1275,7 +1275,10 @@ enum sctp_disposition sctp_sf_backbeat_8_3(struct net *net,
 			return SCTP_DISPOSITION_DISCARD;
 
 		sctp_transport_pl_recv(link);
-		return SCTP_DISPOSITION_CONSUME;
+		if (link->pl.state == SCTP_PL_COMPLETE)
+			return SCTP_DISPOSITION_CONSUME;
+
+		return sctp_sf_send_probe(net, ep, asoc, type, link, commands);
 	}
 
 	max_interval = link->hbinterval + link->rto;

-- 
2.27.0

On Thu, Jun 24, 2021 at 11:48:08AM -0400, Xin Long wrote:

Please either break the condition into 2 lines or even in 2 if()s. The
++ operator here can easily go unnoticed otherwise.

+		/* Raise probe_size again after 30 * interval in Search Complete */
 		t->pl.state = SCTP_PL_SEARCH; /* Search Complete -> Search */
 		t->pl.probe_size += SCTP_PL_MIN_STEP;
 	}
-- 
2.27.0

On Thu, Jun 24, 2021 at 9:11 PM Marcelo Ricardo Leitner
[off-list ref] wrote:

On Thu, Jun 24, 2021 at 11:48:08AM -0400, Xin Long wrote:

quoted

@@ -333,13 +328,15 @@ void sctp_transport_pl_recv(struct sctp_transport *t)
              t->pl.probe_size += SCTP_PL_MIN_STEP;
              if (t->pl.probe_size >= t->pl.probe_high) {
                      t->pl.probe_high = 0;
+                     t->pl.raise_count = 0;
                      t->pl.state = SCTP_PL_COMPLETE; /* Search -> Search Complete */

                      t->pl.probe_size = t->pl.pmtu;
                      t->pathmtu = t->pl.pmtu + sctp_transport_pl_hlen(t);
                      sctp_assoc_sync_pmtu(t->asoc);
              }
-     } else if (t->pl.state == SCTP_PL_COMPLETE) {
+     } else if (t->pl.state == SCTP_PL_COMPLETE && ++t->pl.raise_count == 30) {

Please either break the condition into 2 lines or even in 2 if()s. The
++ operator here can easily go unnoticed otherwise.

will change it to:
        } else if (t->pl.state == SCTP_PL_COMPLETE) {
                t->pl.raise_count++;
                if (t->pl.raise_count == 30) {

Thanks.

quoted

+             /* Raise probe_size again after 30 * interval in Search Complete */
              t->pl.state = SCTP_PL_SEARCH; /* Search Complete -> Search */
              t->pl.probe_size += SCTP_PL_MIN_STEP;
      }
--
2.27.0