From b595a80977193c7dd2a79ab5bd3caaa28bb88252 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 21 Nov 2017 15:48:57 +0100 Subject: [PATCH 1/3] 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 --- man/NetworkManager.conf.xml | 18 ++++++++++++++++++ src/devices/nm-device.c | 16 ++++++++++++++-- src/nm-config.h | 1 + 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/man/NetworkManager.conf.xml b/man/NetworkManager.conf.xml index f0c3c5c85..446540aa1 100644 --- a/man/NetworkManager.conf.xml +++ b/man/NetworkManager.conf.xml @@ -856,6 +856,24 @@ managed=1 + + carrier-wait-timeout + + + 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. + + + ignore-carrier diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index f1643ea77..012db82cf 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 17912185f..8bdd5002d 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." From e4099f67640176bce91c2fcb89f1145c4bd264af Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 28 Nov 2017 10:12:11 +0100 Subject: [PATCH 2/3] 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 --- src/devices/nm-device.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 012db82cf..95237205b 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]; From 245590bacd30314039913d437c2dee9c9f23a366 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 28 Nov 2017 10:32:56 +0100 Subject: [PATCH 3/3] 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. --- src/devices/nm-device.c | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 95237205b..83480096b 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];