[patch] device-carrier-wait-timeout

device-carrier-wait-timeout.patch (text/plain), 13.43 KB, created by Cyril on 2017-11-28 12:24:57 UTC

From b595a80977193c7dd2a79ab5bd3caaa28bb88252 Mon Sep 17 00:00:00 2001
From: Thomas Haller <[e-mail]>
Date: Tue, 21 Nov 2017 15:48:57 +0100
Subject: device: make carrier-wait-timeout configurable per device

As this depends on the particular host configuration, it's hard to find
a default that suits everybody. At least make it configurable per-device.

https://bugzilla.redhat.com/show_bug.cgi?id=1483343
https://bugzilla.redhat.com/show_bug.cgi?id=1515027

diff --git a/man/NetworkManager.conf.xml b/man/NetworkManager.conf.xml
index f0c3c5c..446540a 100644
--- a/man/NetworkManager.conf.xml
+++ b/man/NetworkManager.conf.xml
@@ -856,6 +856,24 @@ managed=1
             </para>
           </listitem>
         </varlistentry>
+        <varlistentry id="carrier-wait-timeout">
+          <term><varname>carrier-wait-timeout</varname></term>
+          <listitem>
+            <para>
+              Specify the timeout for waiting for carrier in milliseconds.
+              When the device loses carrier, NetworkManager does not react
+              immediately. Instead, it waits for this timeout before considering
+              the link lost. Also, on startup, NetworkManager considers the
+              device as busy for this time, as long as the device has no carrier.
+              This delays startup-complete signal and NetworkManager-wait-online.
+              Configuring this too high means to block NetworkManager-wait-online
+              longer then necessary. Configuring it too low, means that NetworkManager
+              will declare startup-complete, although carrier is about to come
+              and auto-activation to kick in.
+              The default is 5000 milliseconds.
+            </para>
+          </listitem>
+        </varlistentry>
         <varlistentry id="ignore-carrier">
           <term><varname>ignore-carrier</varname></term>
           <listitem>
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index f1643ea..012db82 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -328,7 +328,7 @@ typedef struct _NMDevicePrivate {
 	guint32         v4_route_table;
 	guint32         v6_route_table;
 
-	/* when carrier goes away, we give a grace period of CARRIER_WAIT_TIME_MS
+	/* when carrier goes away, we give a grace period of _get_carrier_wait_ms()
 	 * until taking action.
 	 *
 	 * When changing MTU, the device might take longer then that. So, whenever
@@ -10698,6 +10698,18 @@ nm_device_is_up (NMDevice *self)
 	return ifindex > 0 ? nm_platform_link_is_up (nm_device_get_platform (self), ifindex) : TRUE;
 }
 
+static gint64
+_get_carrier_wait_ms (NMDevice *self)
+{
+	gs_free char *value = NULL;
+
+	value = nm_config_data_get_device_config (NM_CONFIG_GET_DATA,
+	                                          NM_CONFIG_KEYFILE_KEY_DEVICE_CARRIER_WAIT_TIMEOUT,
+	                                          self,
+	                                          NULL);
+	return _nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXINT32, CARRIER_WAIT_TIME_MS);
+}
+
 gboolean
 nm_device_bring_up (NMDevice *self, gboolean block, gboolean *no_firmware)
 {
@@ -10772,7 +10784,7 @@ nm_device_bring_up (NMDevice *self, gboolean block, gboolean *no_firmware)
 			nm_device_add_pending_action (self, NM_PENDING_ACTION_CARRIER_WAIT, FALSE);
 
 		now_ms = nm_utils_get_monotonic_timestamp_ms ();
-		until_ms = NM_MAX (now_ms + CARRIER_WAIT_TIME_MS, priv->carrier_wait_until_ms);
+		until_ms = NM_MAX (now_ms + _get_carrier_wait_ms (self), priv->carrier_wait_until_ms);
 		priv->carrier_wait_id = g_timeout_add (until_ms - now_ms, carrier_wait_timeout, self);
 	}
 
diff --git a/src/nm-config.h b/src/nm-config.h
index 1791218..8bdd500 100644
--- a/src/nm-config.h
+++ b/src/nm-config.h
@@ -79,6 +79,7 @@
 #define NM_CONFIG_KEYFILE_KEY_DEVICE_MANAGED                "managed"
 #define NM_CONFIG_KEYFILE_KEY_DEVICE_IGNORE_CARRIER         "ignore-carrier"
 #define NM_CONFIG_KEYFILE_KEY_DEVICE_SRIOV_NUM_VFS          "sriov-num-vfs"
+#define NM_CONFIG_KEYFILE_KEY_DEVICE_CARRIER_WAIT_TIMEOUT   "carrier-wait-timeout"
 
 #define NM_CONFIG_KEYFILE_KEYPREFIX_WAS                     ".was."
 #define NM_CONFIG_KEYFILE_KEYPREFIX_SET                     ".set."
-- 
cgit v0.10.2


From e4099f67640176bce91c2fcb89f1145c4bd264af Mon Sep 17 00:00:00 2001
From: Thomas Haller <[e-mail]>
Date: Tue, 28 Nov 2017 10:12:11 +0100
Subject: device: extend carrier lost defer time after MTU change

Commit a17195b844742be3e01e89e2e5433b600a909ae9 already tried to fix the
bug, but it did so wrongly. We must not increase the carrier-wait-time,
but the carrier-defer-time.

Keep the increase of carrier-wait-time too, and increase both timeouts
after a MTU change.

https://bugzilla.redhat.com/show_bug.cgi?id=1487702
https://bugzilla.gnome.org/show_bug.cgi?id=784854

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 012db82..9523720 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -89,6 +89,9 @@ _LOG_DECLARE_SELF (NMDevice);
 #define CARRIER_WAIT_TIME_MS 5000
 #define CARRIER_WAIT_TIME_AFTER_MTU_MS 10000
 
+#define CARRIER_DEFER_TIME_MS 4000
+#define CARRIER_DEFER_TIME_AFTER_MTU_MS 10000
+
 #define NM_DEVICE_AUTH_RETRIES_UNSET    -1
 #define NM_DEVICE_AUTH_RETRIES_INFINITY -2
 #define NM_DEVICE_AUTH_RETRIES_DEFAULT  3
@@ -335,6 +338,7 @@ typedef struct _NMDevicePrivate {
 	 * NM changes the MTU it sets @carrier_wait_until_ms to CARRIER_WAIT_TIME_AFTER_MTU_MS
 	 * in the future. This is used to extend the grace period in this particular case. */
 	gint64          carrier_wait_until_ms;
+	gint64          carrier_defer_until_ms;
 
 	bool            carrier:1;
 	bool            ignore_carrier:1;
@@ -2465,8 +2469,6 @@ carrier_changed (NMDevice *self, gboolean carrier)
 	}
 }
 
-#define LINK_DISCONNECT_DELAY 4
-
 static gboolean
 carrier_disconnected_action_cb (gpointer user_data)
 {
@@ -2523,10 +2525,13 @@ nm_device_set_carrier (NMDevice *self, gboolean carrier)
 			_LOGD (LOGD_DEVICE, "carrier: link disconnected");
 			carrier_changed (self, FALSE);
 		} else {
-			priv->carrier_defer_id = g_timeout_add_seconds (LINK_DISCONNECT_DELAY,
-			                                                carrier_disconnected_action_cb, self);
-			_LOGD (LOGD_DEVICE, "carrier: link disconnected (deferring action for %d seconds) (id=%u)",
-			       LINK_DISCONNECT_DELAY, priv->carrier_defer_id);
+			gint64 now_ms, until_ms;
+
+			now_ms = nm_utils_get_monotonic_timestamp_ms ();
+			until_ms = NM_MAX (now_ms + CARRIER_DEFER_TIME_MS, priv->carrier_defer_until_ms);
+			priv->carrier_defer_id = g_timeout_add (until_ms - now_ms, carrier_disconnected_action_cb, self);
+			_LOGD (LOGD_DEVICE, "carrier: link disconnected (deferring action for %ld milli seconds) (id=%u)",
+			       (long) (until_ms - now_ms), priv->carrier_defer_id);
 		}
 	}
 }
@@ -7410,6 +7415,7 @@ _commit_mtu (NMDevice *self, const NMIP4Config *config)
 	if (   (mtu_desired && mtu_desired != mtu_plat)
 	    || (ip6_mtu && ip6_mtu != _IP6_MTU_SYS ())) {
 		gboolean anticipated_failure = FALSE;
+		gint64 now_ms;
 
 		if (!priv->mtu_initial && !priv->ip6_mtu_initial) {
 			/* before touching any of the MTU paramters, record the
@@ -7428,7 +7434,9 @@ _commit_mtu (NMDevice *self, const NMIP4Config *config)
 				              ? "Are the MTU sizes of the slaves large enough?"
 				              : "Did you configure the MTU correctly?"));
 			}
-			priv->carrier_wait_until_ms = nm_utils_get_monotonic_timestamp_ms () + CARRIER_WAIT_TIME_AFTER_MTU_MS;
+			now_ms = nm_utils_get_monotonic_timestamp_ms ();
+			priv->carrier_wait_until_ms = now_ms + CARRIER_WAIT_TIME_AFTER_MTU_MS;
+			priv->carrier_defer_until_ms = now_ms + CARRIER_DEFER_TIME_AFTER_MTU_MS;
 		}
 
 		if (ip6_mtu && ip6_mtu != _IP6_MTU_SYS ()) {
@@ -7443,7 +7451,9 @@ _commit_mtu (NMDevice *self, const NMIP4Config *config)
 				           ? ": Is the underlying MTU value successfully set?"
 				           : "");
 			}
-			priv->carrier_wait_until_ms = nm_utils_get_monotonic_timestamp_ms () + CARRIER_WAIT_TIME_AFTER_MTU_MS;
+			now_ms = nm_utils_get_monotonic_timestamp_ms ();
+			priv->carrier_wait_until_ms = now_ms + CARRIER_WAIT_TIME_AFTER_MTU_MS;
+			priv->carrier_defer_until_ms = now_ms + CARRIER_DEFER_TIME_AFTER_MTU_MS;
 		}
 	}
 #undef _IP6_MTU_SYS
@@ -12571,8 +12581,12 @@ nm_device_cleanup (NMDevice *self, NMDeviceStateReason reason, CleanupType clean
 			_LOGT (LOGD_DEVICE, "mtu: reset device-mtu: %u, ipv6-mtu: %u, ifindex: %d",
 			       (guint) priv->mtu_initial, (guint) priv->ip6_mtu_initial, ifindex);
 			if (priv->mtu_initial) {
+				gint64 now;
+
 				nm_platform_link_set_mtu (nm_device_get_platform (self), ifindex, priv->mtu_initial);
-				priv->carrier_wait_until_ms = nm_utils_get_monotonic_timestamp_ms () + CARRIER_WAIT_TIME_AFTER_MTU_MS;
+				now = nm_utils_get_monotonic_timestamp_ms ();
+				priv->carrier_wait_until_ms = now + CARRIER_WAIT_TIME_AFTER_MTU_MS;
+				priv->carrier_defer_until_ms = now + CARRIER_DEFER_TIME_AFTER_MTU_MS;
 			}
 			if (priv->ip6_mtu_initial) {
 				char sbuf[64];
-- 
cgit v0.10.2


From 245590bacd30314039913d437c2dee9c9f23a366 Mon Sep 17 00:00:00 2001
From: Thomas Haller <[e-mail]>
Date: Tue, 28 Nov 2017 10:32:56 +0100
Subject: device: use same values for carrier-wait and carrier-defer time

Waiting for carrier on startup is probably the same times that carrier
needs, e.g. on an MTU change. Use the same timing.

Note, that as carrier-wait-timeout is no configurable, this
configuration applies to both -- as the manual page already
claims to do.

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 9523720..8348009 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -89,9 +89,6 @@ _LOG_DECLARE_SELF (NMDevice);
 #define CARRIER_WAIT_TIME_MS 5000
 #define CARRIER_WAIT_TIME_AFTER_MTU_MS 10000
 
-#define CARRIER_DEFER_TIME_MS 4000
-#define CARRIER_DEFER_TIME_AFTER_MTU_MS 10000
-
 #define NM_DEVICE_AUTH_RETRIES_UNSET    -1
 #define NM_DEVICE_AUTH_RETRIES_INFINITY -2
 #define NM_DEVICE_AUTH_RETRIES_DEFAULT  3
@@ -338,7 +335,6 @@ typedef struct _NMDevicePrivate {
 	 * NM changes the MTU it sets @carrier_wait_until_ms to CARRIER_WAIT_TIME_AFTER_MTU_MS
 	 * in the future. This is used to extend the grace period in this particular case. */
 	gint64          carrier_wait_until_ms;
-	gint64          carrier_defer_until_ms;
 
 	bool            carrier:1;
 	bool            ignore_carrier:1;
@@ -534,6 +530,7 @@ static gboolean addrconf6_start_with_link_ready (NMDevice *self);
 static NMActStageReturn linklocal6_start (NMDevice *self);
 
 static void _carrier_wait_check_queued_act_request (NMDevice *self);
+static gint64 _get_carrier_wait_ms (NMDevice *self);
 
 static const char *_activation_func_to_string (ActivationHandleFunc func);
 static void activation_source_handle_cb (NMDevice *self, int addr_family);
@@ -2528,7 +2525,7 @@ nm_device_set_carrier (NMDevice *self, gboolean carrier)
 			gint64 now_ms, until_ms;
 
 			now_ms = nm_utils_get_monotonic_timestamp_ms ();
-			until_ms = NM_MAX (now_ms + CARRIER_DEFER_TIME_MS, priv->carrier_defer_until_ms);
+			until_ms = NM_MAX (now_ms + _get_carrier_wait_ms (self), priv->carrier_wait_until_ms);
 			priv->carrier_defer_id = g_timeout_add (until_ms - now_ms, carrier_disconnected_action_cb, self);
 			_LOGD (LOGD_DEVICE, "carrier: link disconnected (deferring action for %ld milli seconds) (id=%u)",
 			       (long) (until_ms - now_ms), priv->carrier_defer_id);
@@ -7415,7 +7412,6 @@ _commit_mtu (NMDevice *self, const NMIP4Config *config)
 	if (   (mtu_desired && mtu_desired != mtu_plat)
 	    || (ip6_mtu && ip6_mtu != _IP6_MTU_SYS ())) {
 		gboolean anticipated_failure = FALSE;
-		gint64 now_ms;
 
 		if (!priv->mtu_initial && !priv->ip6_mtu_initial) {
 			/* before touching any of the MTU paramters, record the
@@ -7434,9 +7430,7 @@ _commit_mtu (NMDevice *self, const NMIP4Config *config)
 				              ? "Are the MTU sizes of the slaves large enough?"
 				              : "Did you configure the MTU correctly?"));
 			}
-			now_ms = nm_utils_get_monotonic_timestamp_ms ();
-			priv->carrier_wait_until_ms = now_ms + CARRIER_WAIT_TIME_AFTER_MTU_MS;
-			priv->carrier_defer_until_ms = now_ms + CARRIER_DEFER_TIME_AFTER_MTU_MS;
+			priv->carrier_wait_until_ms = nm_utils_get_monotonic_timestamp_ms () + CARRIER_WAIT_TIME_AFTER_MTU_MS;
 		}
 
 		if (ip6_mtu && ip6_mtu != _IP6_MTU_SYS ()) {
@@ -7451,9 +7445,7 @@ _commit_mtu (NMDevice *self, const NMIP4Config *config)
 				           ? ": Is the underlying MTU value successfully set?"
 				           : "");
 			}
-			now_ms = nm_utils_get_monotonic_timestamp_ms ();
-			priv->carrier_wait_until_ms = now_ms + CARRIER_WAIT_TIME_AFTER_MTU_MS;
-			priv->carrier_defer_until_ms = now_ms + CARRIER_DEFER_TIME_AFTER_MTU_MS;
+			priv->carrier_wait_until_ms = nm_utils_get_monotonic_timestamp_ms () + CARRIER_WAIT_TIME_AFTER_MTU_MS;
 		}
 	}
 #undef _IP6_MTU_SYS
@@ -12581,12 +12573,8 @@ nm_device_cleanup (NMDevice *self, NMDeviceStateReason reason, CleanupType clean
 			_LOGT (LOGD_DEVICE, "mtu: reset device-mtu: %u, ipv6-mtu: %u, ifindex: %d",
 			       (guint) priv->mtu_initial, (guint) priv->ip6_mtu_initial, ifindex);
 			if (priv->mtu_initial) {
-				gint64 now;
-
 				nm_platform_link_set_mtu (nm_device_get_platform (self), ifindex, priv->mtu_initial);
-				now = nm_utils_get_monotonic_timestamp_ms ();
-				priv->carrier_wait_until_ms = now + CARRIER_WAIT_TIME_AFTER_MTU_MS;
-				priv->carrier_defer_until_ms = now + CARRIER_DEFER_TIME_AFTER_MTU_MS;
+				priv->carrier_wait_until_ms = nm_utils_get_monotonic_timestamp_ms () + CARRIER_WAIT_TIME_AFTER_MTU_MS;
 			}
 			if (priv->ip6_mtu_initial) {
 				char sbuf[64];
-- 
cgit v0.10.2