From 9c236416c854ddfb1e99ddd7215cc197e8b86c64 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 26 May 2020 18:55:26 +0200 Subject: device: only ready existing ethtool ring settings if needed Imagine you have a veth device. That device supports certain offload features (like "ethtool.feature-rx-checksum") but doesn't support any ring options. Even trying to read the current ring settings will fail. If you try to activate that profile, NMDevice previously would always try to fetch the ring options and log a warning and extra debugging messages: [1590511552.3943] ethtool[31]: ETHTOOL_GRINGPARAM, v: failed: Operation not supported [1590511552.3944] ethtool[31]: get-ring: failure getting ring settings [1590511552.3944] device (v): ethtool: failure getting ring settings (cannot read) It does so, although you didn't specify any ring settings and there was no need to fetch the ring settings to begin with. Avoid this extra logging by only fetching the ring option when they are actually required. --- src/devices/nm-device.c | 90 +++++++++++++++++++------------------------------ 1 file changed, 35 insertions(+), 55 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 9b018ccc8c..aae1949887 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -950,50 +950,6 @@ _ethtool_coalesce_set (NMDevice *self, _LOGD (LOGD_DEVICE, "ethtool: coalesce settings successfully set"); } -static gboolean -_ethtool_init_ring (NMDevice *self, - NMPlatform *platform, - NMSettingEthtool *s_ethtool, - NMEthtoolRingState *ring) -{ - GHashTable *hash; - GHashTableIter iter; - const char *name; - GVariant *variant; - gsize n_ring_set = 0; - - nm_assert (self); - nm_assert (platform); - nm_assert (ring); - nm_assert (NM_IS_SETTING_ETHTOOL (s_ethtool)); - - hash = _nm_setting_option_hash (NM_SETTING (s_ethtool), FALSE); - if (!hash) - return FALSE; - - g_hash_table_iter_init (&iter, hash); - while (g_hash_table_iter_next (&iter, (gpointer *) &name, (gpointer *) &variant)) { - if (!g_variant_is_of_type (variant, G_VARIANT_TYPE_UINT32)) - continue; - if (!nm_ethtool_optname_is_ring (name)) - continue; - - if (!nm_platform_ethtool_init_ring (platform, - ring, - name, - g_variant_get_uint32(variant))) { - _LOGW (LOGD_DEVICE, "ethtool: invalid ring setting %s", name); - return FALSE; - } - ++n_ring_set; - - } - - return !!n_ring_set; -} - - - static void _ethtool_ring_reset (NMDevice *self, NMPlatform *platform, @@ -1025,25 +981,49 @@ _ethtool_ring_set (NMDevice *self, { NMEthtoolRingState ring_old; NMEthtoolRingState ring_new; + GHashTable *hash; + GHashTableIter iter; + const char *name; + GVariant *variant; + gboolean has_old = FALSE; - nm_assert (ethtool_state); nm_assert (NM_IS_DEVICE (self)); nm_assert (NM_IS_PLATFORM (platform)); nm_assert (NM_IS_SETTING_ETHTOOL (s_ethtool)); + nm_assert (ethtool_state); + nm_assert (!ethtool_state->ring); - if (!nm_platform_ethtool_get_link_ring (platform, - ethtool_state->ifindex, - &ring_old)) { - _LOGW (LOGD_DEVICE, "ethtool: failure getting ring settings (cannot read)"); + hash = _nm_setting_option_hash (NM_SETTING (s_ethtool), FALSE); + if (!hash) return; - } - ring_new = ring_old; + g_hash_table_iter_init (&iter, hash); + while (g_hash_table_iter_next (&iter, (gpointer *) &name, (gpointer *) &variant)) { + if (!nm_ethtool_optname_is_ring (name)) + continue; + nm_assert (g_variant_is_of_type (variant, G_VARIANT_TYPE_UINT32)); + + if (!has_old) { + if (!nm_platform_ethtool_get_link_ring (platform, + ethtool_state->ifindex, + &ring_old)) { + _LOGW (LOGD_DEVICE, "ethtool: failure setting ring options (cannot read existing setting)"); + return; + } + has_old = TRUE; + ring_new = ring_old; + } - if (!_ethtool_init_ring (self, - platform, - s_ethtool, - &ring_new)) + if (!nm_platform_ethtool_init_ring (platform, + &ring_new, + name, + g_variant_get_uint32 (variant))) { + _LOGW (LOGD_DEVICE, "ethtool: invalid ring setting %s", name); + return; + } + } + + if (!has_old) return; ethtool_state->ring = nm_memdup (&ring_old, sizeof (ring_old)); -- cgit v1.2.1 From 23d0a76b16205bcda082ba07786efbd49030d171 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 26 May 2020 19:02:18 +0200 Subject: device: inline nm_platform_ethtool_init_ring() function nm_platform_ethtool_init_ring() only has one caller. It's simpler to drop the function and implement it at the only place where it is needed. Maybe there could be a place for a function to initialize NMEthtoolRingState, one option after the other. However, at the moment there is only one user, so don't implement it. This fixes various minor issues: - the function had a NMPlatform argument, although the argument is not used. Thus function merely operates on a NMEthtoolRingState instance and shouldn't have a nm_platform_*() name. - nm_platform_ethtool_init_ring() returned a boolean, but all code paths (except assertion failures) returned success. - as the function returned an error status, the caller was compelled to handle an error that could never happen. - the option was specified by name, although we already have a more efficient way to express the option: the NMEthtoolID. Also, the caller already needed to resolve the name to the NMEthtoolID, so there was no need to again lookup the ID by name. --- src/devices/nm-device.c | 31 ++++++++++++++++++++++--------- src/platform/nm-platform.c | 35 ----------------------------------- src/platform/nm-platform.h | 5 ----- 3 files changed, 22 insertions(+), 49 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index aae1949887..327df52c6c 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -999,8 +999,12 @@ _ethtool_ring_set (NMDevice *self, g_hash_table_iter_init (&iter, hash); while (g_hash_table_iter_next (&iter, (gpointer *) &name, (gpointer *) &variant)) { - if (!nm_ethtool_optname_is_ring (name)) + NMEthtoolID ethtool_id = nm_ethtool_id_get_by_name (name); + guint32 u32; + + if (!nm_ethtool_id_is_ring (ethtool_id)) continue; + nm_assert (g_variant_is_of_type (variant, G_VARIANT_TYPE_UINT32)); if (!has_old) { @@ -1014,12 +1018,23 @@ _ethtool_ring_set (NMDevice *self, ring_new = ring_old; } - if (!nm_platform_ethtool_init_ring (platform, - &ring_new, - name, - g_variant_get_uint32 (variant))) { - _LOGW (LOGD_DEVICE, "ethtool: invalid ring setting %s", name); - return; + u32 = g_variant_get_uint32 (variant); + + switch (ethtool_id) { + case NM_ETHTOOL_ID_RING_RX: + ring_new.rx_pending = u32; + break; + case NM_ETHTOOL_ID_RING_RX_JUMBO: + ring_new.rx_jumbo_pending = u32; + break; + case NM_ETHTOOL_ID_RING_RX_MINI: + ring_new.rx_mini_pending = u32; + break; + case NM_ETHTOOL_ID_RING_TX: + ring_new.tx_pending = u32; + break; + default: + nm_assert_not_reached (); } } @@ -1056,8 +1071,6 @@ _ethtool_state_reset (NMDevice *self) _ethtool_ring_reset (self, platform, ethtool_state); } - - static void _ethtool_state_set (NMDevice *self) { diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index b7a43a0ea1..2f2f99f00a 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -3252,41 +3252,6 @@ nm_platform_ethtool_get_link_ring (NMPlatform *self, return nmp_utils_ethtool_get_ring (ifindex, ring); } -gboolean -nm_platform_ethtool_init_ring (NMPlatform *self, - NMEthtoolRingState *ring, - const char *option_name, - guint32 value) -{ - NMEthtoolID ethtool_id; - - g_return_val_if_fail (ring, FALSE); - g_return_val_if_fail (option_name, FALSE); - - ethtool_id = nm_ethtool_id_get_by_name (option_name); - - g_return_val_if_fail (nm_ethtool_id_is_ring (ethtool_id), FALSE); - - switch (ethtool_id) { - case NM_ETHTOOL_ID_RING_RX: - ring->rx_pending = value; - break; - case NM_ETHTOOL_ID_RING_RX_JUMBO: - ring->rx_jumbo_pending = value; - break; - case NM_ETHTOOL_ID_RING_RX_MINI: - ring->rx_mini_pending = value; - break; - case NM_ETHTOOL_ID_RING_TX: - ring->tx_pending = value; - break; - default: - g_return_val_if_reached (FALSE); - } - - return TRUE; -} - gboolean nm_platform_ethtool_set_ring (NMPlatform *self, int ifindex, diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index fb82262979..c4eda16cc4 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -1975,11 +1975,6 @@ gboolean nm_platform_ethtool_get_link_ring (NMPlatform *self, int ifindex, NMEthtoolRingState *ring); -gboolean nm_platform_ethtool_init_ring (NMPlatform *self, - NMEthtoolRingState *ring, - const char *option_name, - guint32 value); - gboolean nm_platform_ethtool_set_ring (NMPlatform *self, int ifindex, const NMEthtoolRingState *ring); -- cgit v1.2.1 From 0b23ae315850540c9078869862b29d494508f03f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 27 May 2020 13:50:40 +0200 Subject: device: reset original autoneg/speed/duplex setting on deactivate The autoneg/speed ethtool settings are important. If they are wrong, the device might not get any carrier. Having no carrier means that you may be unable to activate a profile (because depending on configuration, carrier is required to activate a profile). Since activating profiles are the means to configure the link settings in NetworkManager, and activating a profile can be hampered by wrong link settings, it's important to reset the "correct" settings, when deactivating a profile. "Correct" in this case means to restore the settings that were present before NM changed the settings. Presumably, these are the right once. Beyond that, in the future it might make sense to support configuring the default link settings per device. So that NM will always restore a defined, configured, working state. The problem is that per-device settings currently are only available via NetworkManager.conf, which is rather inflexible. Also, when you restart NetworkManager service, it leaves the interface up but forgets the previous setting. That possibly could be fixed by persisting the previous link state in /run. However, it's not implemented yet. https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/356 https://bugzilla.redhat.com/show_bug.cgi?id=1807171 --- src/devices/nm-device-ethernet.c | 59 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c index 7556bb27ba..9e7601f005 100644 --- a/src/devices/nm-device-ethernet.c +++ b/src/devices/nm-device-ethernet.c @@ -100,7 +100,15 @@ typedef struct _NMDeviceEthernetPrivate { DcbWait dcb_wait; guint dcb_timeout_id; + guint32 ethtool_prev_speed; + + NMPlatformLinkDuplexType ethtool_prev_duplex:3; + bool dcb_handle_carrier_changes:1; + + bool ethtool_prev_set:1; + bool ethtool_prev_autoneg:1; + } NMDeviceEthernetPrivate; NM_GOBJECT_PROPERTIES_DEFINE (NMDeviceEthernet, @@ -882,6 +890,7 @@ static void link_negotiation_set (NMDevice *device) { NMDeviceEthernet *self = NM_DEVICE_ETHERNET (device); + NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE (self); NMSettingWired *s_wired; gboolean autoneg = TRUE; gboolean link_autoneg; @@ -925,7 +934,7 @@ link_negotiation_set (NMDevice *device) && !duplex) _LOGD (LOGD_DEVICE, "set-link: configure auto-negotiation"); else { - _LOGD (LOGD_DEVICE, "set-link: configure %snegotiation (%u Mbit%s - %s duplex%s)", + _LOGD (LOGD_DEVICE, "set-link: configure %snegotiation (%u Mbit%s, %s duplex%s)", autoneg ? "auto-" : "static ", speed ?: link_speed, speed ? "" : "*", @@ -935,6 +944,14 @@ link_negotiation_set (NMDevice *device) duplex ? "" : "*"); } + if (!priv->ethtool_prev_set) { + /* remember the values we had before setting it. */ + priv->ethtool_prev_autoneg = link_autoneg; + priv->ethtool_prev_speed = link_speed; + priv->ethtool_prev_duplex = link_duplex; + priv->ethtool_prev_set = TRUE; + } + if (!nm_platform_ethtool_set_link_settings (nm_device_get_platform (device), nm_device_get_ifindex (device), autoneg, @@ -964,6 +981,28 @@ act_stage1_prepare (NMDevice *device, NMDeviceStateReason *out_failure_reason) NMDeviceEthernet *self = NM_DEVICE_ETHERNET (device); NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE (self); + if (nm_device_sys_iface_state_is_external_or_assume (device)) { + if ( !priv->ethtool_prev_set + && !nm_device_sys_iface_state_is_external (device)) { + NMSettingWired *s_wired; + + /* During restart of NetworkManager service we forget the original auto + * negotiation settings. When taking over a device, remember to reset + * the "default" during deactivate. */ + s_wired = nm_device_get_applied_setting (device, NM_TYPE_SETTING_WIRED); + if ( s_wired + && ( nm_setting_wired_get_auto_negotiate (s_wired) + || nm_setting_wired_get_speed (s_wired) + || nm_setting_wired_get_duplex (s_wired))) { + priv->ethtool_prev_set = TRUE; + priv->ethtool_prev_autoneg = TRUE; + priv->ethtool_prev_speed = 0; + priv->ethtool_prev_duplex = NM_PLATFORM_LINK_DUPLEX_UNKNOWN; + } + } + return NM_ACT_STAGE_RETURN_SUCCESS; + } + link_negotiation_set (device); /* If we're re-activating a PPPoE connection a short while after @@ -1525,6 +1564,23 @@ deactivate (NMDevice *device) /* Set last PPPoE connection time */ if (nm_device_get_applied_setting (device, NM_TYPE_SETTING_PPPOE)) priv->last_pppoe_time = nm_utils_get_monotonic_timestamp_sec (); + + if (priv->ethtool_prev_set) { + priv->ethtool_prev_set = FALSE; + + _LOGD (LOGD_DEVICE, "set-link: reset %snegotiation (%u Mbit, %s duplex)", + priv->ethtool_prev_autoneg ? "auto-" : "static ", + priv->ethtool_prev_speed, + nm_platform_link_duplex_type_to_string (priv->ethtool_prev_duplex)); + if (!nm_platform_ethtool_set_link_settings (nm_device_get_platform (device), + nm_device_get_ifindex (device), + priv->ethtool_prev_autoneg, + priv->ethtool_prev_speed, + priv->ethtool_prev_duplex)) { + _LOGW (LOGD_DEVICE, "set-link: failure to reset link negotiation"); + return; + } + } } static gboolean @@ -1914,6 +1970,7 @@ nm_device_ethernet_class_init (NMDeviceEthernetClass *klass) device_class->complete_connection = complete_connection; device_class->new_default_connection = new_default_connection; + device_class->act_stage1_prepare_also_for_external_or_assume = TRUE; device_class->act_stage1_prepare = act_stage1_prepare; device_class->act_stage1_prepare_set_hwaddr_ethernet = TRUE; device_class->act_stage2_config = act_stage2_config; -- cgit v1.2.1