From e7b5650eff4eb257cd4544fa89eb53c0554c3025 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 15 Jun 2021 00:41:45 +0200 Subject: [PATCH] core: add nm_settings_get_connection_sorted_by_autoconnect_priority() Turns out, we call nm_settings_get_connection_clone() *a lot* with sort order nm_settings_connection_cmp_autoconnect_priority_p_with_data(). As we cache the (differently sorted) list of connections, also cache the presorted list. The only complication is that every time we still need to check whether the list is still sorted, because it would be more complicated to invalidate the list when an entry changes which affects the sort order. Still, such a check is usually successful and requires "only" N-1 comparisons. --- src/core/nm-manager.c | 80 ++++++++++----------------- src/core/settings/nm-settings.c | 97 ++++++++++++++++++++++++++++----- src/core/settings/nm-settings.h | 3 + 3 files changed, 115 insertions(+), 65 deletions(-) diff --git a/src/core/nm-manager.c b/src/core/nm-manager.c index 419ce88dfe..b66f7fbb37 100644 --- a/src/core/nm-manager.c +++ b/src/core/nm-manager.c @@ -1999,16 +1999,16 @@ nm_manager_remove_device(NMManager *self, const char *ifname, NMDeviceType devic static NMDevice * system_create_virtual_device(NMManager *self, NMConnection *connection) { - NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE(self); - NMDeviceFactory * factory; - gs_free NMSettingsConnection **connections = NULL; - guint i; - gs_free char * iface = NULL; - const char * parent_spec; - NMDevice * device = NULL, *parent = NULL; - NMDevice * dev_candidate; - GError * error = NULL; - NMLogLevel log_level; + NMManagerPrivate * priv = NM_MANAGER_GET_PRIVATE(self); + NMDeviceFactory * factory; + NMSettingsConnection *const *connections; + guint i; + gs_free char * iface = NULL; + const char * parent_spec; + NMDevice * device = NULL, *parent = NULL; + NMDevice * dev_candidate; + GError * error = NULL; + NMLogLevel log_level; g_return_val_if_fail(NM_IS_MANAGER(self), NULL); g_return_val_if_fail(NM_IS_CONNECTION(connection), NULL); @@ -2091,13 +2091,7 @@ system_create_virtual_device(NMManager *self, NMConnection *connection) } /* Create backing resources if the device has any autoconnect connections */ - connections = nm_settings_get_connections_clone( - priv->settings, - NULL, - NULL, - NULL, - nm_settings_connection_cmp_autoconnect_priority_p_with_data, - NULL); + connections = nm_settings_get_connections_sorted_by_autoconnect_priority(priv->settings, NULL); for (i = 0; connections[i]; i++) { NMConnection * candidate = nm_settings_connection_get_connection(connections[i]); NMSettingConnection *s_con; @@ -2136,19 +2130,13 @@ system_create_virtual_device(NMManager *self, NMConnection *connection) static void retry_connections_for_parent_device(NMManager *self, NMDevice *device) { - NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE(self); - gs_free NMSettingsConnection **connections = NULL; - guint i; + NMManagerPrivate * priv = NM_MANAGER_GET_PRIVATE(self); + NMSettingsConnection *const *connections; + guint i; g_return_if_fail(device); - connections = nm_settings_get_connections_clone( - priv->settings, - NULL, - NULL, - NULL, - nm_settings_connection_cmp_autoconnect_priority_p_with_data, - NULL); + connections = nm_settings_get_connections_sorted_by_autoconnect_priority(priv->settings, NULL); for (i = 0; connections[i]; i++) { NMSettingsConnection *sett_conn = connections[i]; NMConnection * connection = nm_settings_connection_get_connection(sett_conn); @@ -4425,13 +4413,13 @@ find_slaves(NMManager * manager, guint * out_n_slaves, gboolean for_user_request) { - NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE(manager); - gs_free NMSettingsConnection **all_connections = NULL; - guint n_all_connections; - guint i; - SlaveConnectionInfo * slaves = NULL; - guint n_slaves = 0; - NMSettingConnection * s_con; + NMManagerPrivate * priv = NM_MANAGER_GET_PRIVATE(manager); + NMSettingsConnection *const *all_connections = NULL; + guint n_all_connections; + guint i; + SlaveConnectionInfo * slaves = NULL; + guint n_slaves = 0; + NMSettingConnection * s_con; gs_unref_hashtable GHashTable *devices = NULL; nm_assert(out_n_slaves); @@ -4445,13 +4433,9 @@ find_slaves(NMManager * manager, * even if a slave was already active, it might be deactivated during * master reactivation. */ - all_connections = nm_settings_get_connections_clone( - priv->settings, - &n_all_connections, - NULL, - NULL, - nm_settings_connection_cmp_autoconnect_priority_p_with_data, - NULL); + all_connections = + nm_settings_get_connections_sorted_by_autoconnect_priority(priv->settings, + &n_all_connections); for (i = 0; i < n_all_connections; i++) { NMSettingsConnection *master_connection = NULL; NMDevice * master_device = NULL, *slave_device; @@ -6918,9 +6902,9 @@ devices_inited_cb(gpointer user_data) gboolean nm_manager_start(NMManager *self, GError **error) { - NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE(self); - gs_free NMSettingsConnection **connections = NULL; - guint i; + NMManagerPrivate * priv = NM_MANAGER_GET_PRIVATE(self); + NMSettingsConnection *const *connections; + guint i; nm_device_factory_manager_load_factories(_register_device_factory, self); @@ -6978,13 +6962,7 @@ nm_manager_start(NMManager *self, GError **error) NM_SETTINGS_SIGNAL_CONNECTION_UPDATED, G_CALLBACK(connection_updated_cb), self); - connections = nm_settings_get_connections_clone( - priv->settings, - NULL, - NULL, - NULL, - nm_settings_connection_cmp_autoconnect_priority_p_with_data, - NULL); + connections = nm_settings_get_connections_sorted_by_autoconnect_priority(priv->settings, NULL); for (i = 0; connections[i]; i++) connection_changed(self, connections[i]); diff --git a/src/core/settings/nm-settings.c b/src/core/settings/nm-settings.c index 36a357748e..bb58af9899 100644 --- a/src/core/settings/nm-settings.c +++ b/src/core/settings/nm-settings.c @@ -371,6 +371,7 @@ typedef struct { CList connections_lst_head; NMSettingsConnection **connections_cached_list; + NMSettingsConnection **connections_cached_list_sorted_by_autoconnect_priority; GSList *unmanaged_specs; GSList *unrecognized_specs; @@ -2873,22 +2874,37 @@ impl_settings_reload_connections(NMDBusObject * obj, static void _clear_connections_cached_list(NMSettingsPrivate *priv) { - if (!priv->connections_cached_list) - return; - - nm_assert(priv->connections_len == NM_PTRARRAY_LEN(priv->connections_cached_list)); + if (priv->connections_cached_list) { + nm_assert(priv->connections_len == NM_PTRARRAY_LEN(priv->connections_cached_list)); #if NM_MORE_ASSERTS - /* set the pointer to a bogus value. This makes it more apparent - * if somebody has a reference to the cached list and still uses - * it. That is a bug, this code just tries to make it blow up - * more eagerly. */ - memset(priv->connections_cached_list, - 0x43, - sizeof(NMSettingsConnection *) * (priv->connections_len + 1)); + /* set the pointer to a bogus value. This makes it more apparent + * if somebody has a reference to the cached list and still uses + * it. That is a bug, this code just tries to make it blow up + * more eagerly. */ + memset(priv->connections_cached_list, + 0x43, + sizeof(NMSettingsConnection *) * (priv->connections_len + 1)); #endif - nm_clear_g_free(&priv->connections_cached_list); + nm_clear_g_free(&priv->connections_cached_list); + } + if (priv->connections_cached_list_sorted_by_autoconnect_priority) { + nm_assert(priv->connections_len + == NM_PTRARRAY_LEN(priv->connections_cached_list_sorted_by_autoconnect_priority)); + +#if NM_MORE_ASSERTS + /* set the pointer to a bogus value. This makes it more apparent + * if somebody has a reference to the cached list and still uses + * it. That is a bug, this code just tries to make it blow up + * more eagerly. */ + memset(priv->connections_cached_list_sorted_by_autoconnect_priority, + 0x42, + sizeof(NMSettingsConnection *) * (priv->connections_len + 1)); +#endif + + nm_clear_g_free(&priv->connections_cached_list_sorted_by_autoconnect_priority); + } } static void @@ -3027,6 +3043,55 @@ nm_settings_get_connections(NMSettings *self, guint *out_len) return priv->connections_cached_list; } +NMSettingsConnection *const * +nm_settings_get_connections_sorted_by_autoconnect_priority(NMSettings *self, guint *out_len) +{ + NMSettingsPrivate *priv; + gboolean needs_sort = FALSE; + + g_return_val_if_fail(NM_IS_SETTINGS(self), NULL); + + priv = NM_SETTINGS_GET_PRIVATE(self); + + nm_assert(priv->connections_len == c_list_length(&priv->connections_lst_head)); + nm_assert( + !priv->connections_cached_list_sorted_by_autoconnect_priority + || (priv->connections_len + == NM_PTRARRAY_LEN(priv->connections_cached_list_sorted_by_autoconnect_priority))); + + if (!priv->connections_cached_list_sorted_by_autoconnect_priority) { + NMSettingsConnection *const *list_cached; + guint len; + + list_cached = nm_settings_get_connections(self, &len); + priv->connections_cached_list_sorted_by_autoconnect_priority = + nm_memdup(list_cached, sizeof(NMSettingsConnection *) * (len + 1)); + needs_sort = (len > 1); + } else if (!nm_utils_ptrarray_is_sorted( + (gconstpointer *) priv->connections_cached_list_sorted_by_autoconnect_priority, + priv->connections_len, + FALSE, + nm_settings_connection_cmp_autoconnect_priority_with_data, + NULL)) { + /* We cache the sorted list, but we don't monitor all entries whether they + * get modified to invalidate the sort order. So every time we have to check + * whether the sort order is still correct. The vast majority of the time it + * is, and this check is faster than sorting anew. */ + needs_sort = TRUE; + } + + if (needs_sort) { + g_qsort_with_data(priv->connections_cached_list_sorted_by_autoconnect_priority, + priv->connections_len, + sizeof(NMSettingsConnection *), + nm_settings_connection_cmp_autoconnect_priority_p_with_data, + NULL); + } + + NM_SET_OUT(out_len, priv->connections_len); + return priv->connections_cached_list_sorted_by_autoconnect_priority; +} + /** * nm_settings_get_connections_clone: * @self: the #NMSetting @@ -3058,9 +3123,13 @@ nm_settings_get_connections_clone(NMSettings * self, g_return_val_if_fail(NM_IS_SETTINGS(self), NULL); - list_cached = nm_settings_get_connections(self, &len); + if (sort_compare_func == nm_settings_connection_cmp_autoconnect_priority_p_with_data) { + list_cached = nm_settings_get_connections_sorted_by_autoconnect_priority(self, &len); + sort_compare_func = NULL; + } else + list_cached = nm_settings_get_connections(self, &len); -#if NM_MORE_ASSERTS +#if NM_MORE_ASSERTS > 10 nm_assert(list_cached); for (i = 0; i < len; i++) nm_assert(NM_IS_SETTINGS_CONNECTION(list_cached[i])); diff --git a/src/core/settings/nm-settings.h b/src/core/settings/nm-settings.h index 09a0af57f6..fbbc957477 100644 --- a/src/core/settings/nm-settings.h +++ b/src/core/settings/nm-settings.h @@ -79,6 +79,9 @@ void nm_settings_add_connection_dbus(NMSettings * self, NMSettingsConnection *const *nm_settings_get_connections(NMSettings *settings, guint *out_len); +NMSettingsConnection *const * +nm_settings_get_connections_sorted_by_autoconnect_priority(NMSettings *self, guint *out_len); + NMSettingsConnection **nm_settings_get_connections_clone(NMSettings * self, guint * out_len, NMSettingsConnectionFilterFunc func,