From 6f53f390a2698e69195eb8215be0ba1cc2396212 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 20 Nov 2023 10:57:42 +0100 Subject: [PATCH 1/6] cli: unifiy handling of IPv4/IPv6 in ac_overview() As often, the code for IPv4/IPv6 is very similar. It's better to treat the address family in a similar way. --- src/nmcli/general.c | 45 ++++++++++++++++----------------------------- 1 file changed, 16 insertions(+), 29 deletions(-) diff --git a/src/nmcli/general.c b/src/nmcli/general.c index 0d8ddc47fe..104faf1e4d 100644 --- a/src/nmcli/general.c +++ b/src/nmcli/general.c @@ -1412,8 +1412,8 @@ static void ac_overview(NmCli *nmc, NMActiveConnection *ac) { GString *outbuf = g_string_sized_new(80); - NMIPConfig *ip; - nm_auto_str_buf NMStrBuf str = NM_STR_BUF_INIT(NM_UTILS_GET_NEXT_REALLOC_SIZE_104, FALSE); + nm_auto_str_buf NMStrBuf str = NM_STR_BUF_INIT(NM_UTILS_GET_NEXT_REALLOC_SIZE_104, FALSE); + int IS_IPv4; if (nm_active_connection_get_controller(ac)) { g_string_append_printf(outbuf, @@ -1432,15 +1432,24 @@ ac_overview(NmCli *nmc, NMActiveConnection *ac) nmc_print("\t%s\n", outbuf->str); } - ip = nm_active_connection_get_ip4_config(ac); - if (ip) { + for (IS_IPv4 = 1; IS_IPv4 >= 0; IS_IPv4--) { + NMIPConfig *ip; const GPtrArray *p; - int i; + guint i; + + ip = IS_IPv4 ? nm_active_connection_get_ip4_config(ac) + : nm_active_connection_get_ip6_config(ac); + if (!ip) + continue; p = nm_ip_config_get_addresses(ip); for (i = 0; i < p->len; i++) { NMIPAddress *a = p->pdata[i]; - nmc_print("\tinet4 %s/%d\n", nm_ip_address_get_address(a), nm_ip_address_get_prefix(a)); + + nmc_print("\tinet%c %s/%d\n", + IS_IPv4 ? '4' : '6', + nm_ip_address_get_address(a), + nm_ip_address_get_prefix(a)); } p = nm_ip_config_get_routes(ip); @@ -1450,29 +1459,7 @@ ac_overview(NmCli *nmc, NMActiveConnection *ac) nm_str_buf_reset(&str); _nm_ip_route_to_string(a, &str); - nmc_print("\troute4 %s\n", nm_str_buf_get_str(&str)); - } - } - - ip = nm_active_connection_get_ip6_config(ac); - if (ip) { - const GPtrArray *p; - int i; - - p = nm_ip_config_get_addresses(ip); - for (i = 0; i < p->len; i++) { - NMIPAddress *a = p->pdata[i]; - nmc_print("\tinet6 %s/%d\n", nm_ip_address_get_address(a), nm_ip_address_get_prefix(a)); - } - - p = nm_ip_config_get_routes(ip); - for (i = 0; i < p->len; i++) { - NMIPRoute *a = p->pdata[i]; - - nm_str_buf_reset(&str); - _nm_ip_route_to_string(a, &str); - - nmc_print("\troute6 %s\n", nm_str_buf_get_str(&str)); + nmc_print("\troute%c %s\n", IS_IPv4 ? '4' : '6', nm_str_buf_get_str(&str)); } } From 39d900593bcb586bb8468e15d747e1de225a8f56 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 20 Nov 2023 11:02:37 +0100 Subject: [PATCH 2/6] cli: reuse NMStrBuf in ac_overview() No need to mix GString and NMStrBuf. --- src/nmcli/general.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/nmcli/general.c b/src/nmcli/general.c index 104faf1e4d..9009c41cd4 100644 --- a/src/nmcli/general.c +++ b/src/nmcli/general.c @@ -1411,27 +1411,28 @@ device_overview(NmCli *nmc, NMDevice *device) static void ac_overview(NmCli *nmc, NMActiveConnection *ac) { - GString *outbuf = g_string_sized_new(80); - nm_auto_str_buf NMStrBuf str = NM_STR_BUF_INIT(NM_UTILS_GET_NEXT_REALLOC_SIZE_104, FALSE); + nm_auto_str_buf NMStrBuf str = NM_STR_BUF_INIT_A(NM_UTILS_GET_NEXT_REALLOC_SIZE_488, FALSE); int IS_IPv4; if (nm_active_connection_get_controller(ac)) { - g_string_append_printf(outbuf, - "%s %s, ", - _("master"), - nm_device_get_iface(nm_active_connection_get_controller(ac))); + nm_str_buf_append_printf(&str, + "%s %s, ", + _("master"), + nm_device_get_iface(nm_active_connection_get_controller(ac))); } if (nm_active_connection_get_vpn(ac)) - g_string_append_printf(outbuf, "%s, ", _("VPN")); + nm_str_buf_append_printf(&str, "%s, ", _("VPN")); if (nm_active_connection_get_default(ac)) - g_string_append_printf(outbuf, "%s, ", _("ip4 default")); + nm_str_buf_append_printf(&str, "%s, ", _("ip4 default")); if (nm_active_connection_get_default6(ac)) - g_string_append_printf(outbuf, "%s, ", _("ip6 default")); - if (outbuf->len >= 2) { - g_string_truncate(outbuf, outbuf->len - 2); - nmc_print("\t%s\n", outbuf->str); + nm_str_buf_append_printf(&str, "%s, ", _("ip6 default")); + if (str.len >= 2) { + nm_str_buf_set_size(&str, str.len - 2u, TRUE, FALSE); + nmc_print("\t%s\n", nm_str_buf_get_str(&str)); } + nm_str_buf_reset(&str); + for (IS_IPv4 = 1; IS_IPv4 >= 0; IS_IPv4--) { NMIPConfig *ip; const GPtrArray *p; @@ -1462,8 +1463,6 @@ ac_overview(NmCli *nmc, NMActiveConnection *ac) nmc_print("\troute%c %s\n", IS_IPv4 ? '4' : '6', nm_str_buf_get_str(&str)); } } - - g_string_free(outbuf, TRUE); } void From a7a36cde83d3c939a53122b65e861a6acbbd62ed Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 20 Nov 2023 11:14:46 +0100 Subject: [PATCH 3/6] cli: limit number of shown addresses/routes in `nmcli` overview If you add a large number of addresses/routes, then the output of `nmcli` is unusable. It also doesn't seem too useful. Limit the number to show up to 10 addresses and 10 routes. If there are more than 10 addresses, then print an 11th line with inet4 ... N more Actually, if there are exactly 11 addresses, then don't waste an extra line to print "1 more". Instead, still print the 11th address. Same for routes. --- NEWS | 1 + src/nmcli/general.c | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/NEWS b/NEWS index 5759157b42..63f21d0457 100644 --- a/NEWS +++ b/NEWS @@ -15,6 +15,7 @@ Overview of changes since NetworkManager-1.44 * Honor udev property ID_NET_MANAGED_BY to only manage an interface when set to "org.freedesktop.NetworkManager". * Drop build support with Python2. Python3 is now required. +* nmcli: limit number of printed addresses/routes in `nmcli` overview to 10. ============================================= NetworkManager-1.44 diff --git a/src/nmcli/general.c b/src/nmcli/general.c index 9009c41cd4..df76e9fa08 100644 --- a/src/nmcli/general.c +++ b/src/nmcli/general.c @@ -1412,6 +1412,8 @@ static void ac_overview(NmCli *nmc, NMActiveConnection *ac) { nm_auto_str_buf NMStrBuf str = NM_STR_BUF_INIT_A(NM_UTILS_GET_NEXT_REALLOC_SIZE_488, FALSE); + const guint MAX_ADDRESSES = 10; + const guint MAX_ROUTES = 10; int IS_IPv4; if (nm_active_connection_get_controller(ac)) { @@ -1451,6 +1453,15 @@ ac_overview(NmCli *nmc, NMActiveConnection *ac) IS_IPv4 ? '4' : '6', nm_ip_address_get_address(a), nm_ip_address_get_prefix(a)); + + if (i >= MAX_ADDRESSES - 1u && p->len - i > 2u) { + /* Print always at least MAX_ADDRESSES fully. + * If there are MAX_ADDRESSES+1 addresses, print them all fully. + * If there are more addresses, print MAX_ADDRESSES fully, and a + * "N more" line. */ + nmc_print("\tinet%c ... %u more\n", IS_IPv4 ? '4' : '6', p->len - i - 1u); + break; + } } p = nm_ip_config_get_routes(ip); @@ -1461,6 +1472,11 @@ ac_overview(NmCli *nmc, NMActiveConnection *ac) _nm_ip_route_to_string(a, &str); nmc_print("\troute%c %s\n", IS_IPv4 ? '4' : '6', nm_str_buf_get_str(&str)); + + if (i >= MAX_ROUTES - 1u && p->len - i > 2u) { + nmc_print("\troute%c ... %u more\n", IS_IPv4 ? '4' : '6', p->len - i - 1u); + break; + } } } } From 623012c14a192f0a4f7fe98e26224ff2055b7650 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 20 Nov 2023 12:09:13 +0100 Subject: [PATCH 4/6] core: rate-limit updates to IP addresses/routes on D-Bus API It doesn't scale. If you add 100k routes one-by-one, then upon each change from platform, we will send the growing list of the routes on D-Bus. That is too expensive. Especially, if you imagine that the receiving end is a NMClient instance. There is a D-Bus worker thread that queues the received GVariant messages, while the main thread may not be able to process them fast enough. In that case, the memory keeps growing very fast and due to fragmentation it is not freed. Instead, rate limit updates to 3 per second. Note that the receive buffer of the netlink socket can fill up and we loose messages. Therefore, already on the lowest level, we may miss addresses/routes. Next, on top of NMPlatform, NMIPConfig listens to NM_L3_CONFIG_NOTIFY_TYPE_PLATFORM_CHANGE_ON_IDLE events. Thereby it further will miss intermediate state (e.g. a route that exists only for a short moment). Now adding another delay and rate limiting on top of that, does not make that fundamentally different, we anyway didn't get all intermediate states from netlink. We may miss addresses/routes that only exist for a short amount of time. This makes "the problem" worse, but not fundamentally new. We can only get a (correct) settled state, after all events are processed. And we never know, whether there isn't the next event just waiting to be received. Rate limiting is important to not overwhelm D-Bus clients. In reality, none of the users really need this information, because it's also incomplete. Users who really need to know addresses/routes should use netlink or find another way (a way that scales and where they explicitly request this information). --- src/core/nm-ip-config.c | 75 ++++++++++++++++++++++++++++++++++++++++- src/core/nm-ip-config.h | 5 ++- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/core/nm-ip-config.c b/src/core/nm-ip-config.c index bec67d9a46..48f59bd427 100644 --- a/src/core/nm-ip-config.c +++ b/src/core/nm-ip-config.c @@ -16,6 +16,10 @@ /*****************************************************************************/ +#define NOTIFY_PLATFORM_RATELIMIT_MSEC 333 + +/*****************************************************************************/ + GType nm_ip4_config_get_type(void); GType nm_ip6_config_get_type(void); @@ -67,6 +71,73 @@ _value_set_variant_as(GValue *value, const char *const *strv, guint len) /*****************************************************************************/ +static void +_notify_platform_handle(NMIPConfig *self, gint64 now_msec) +{ + NMIPConfigPrivate *priv = NM_IP_CONFIG_GET_PRIVATE(self); + guint32 obj_type_flags; + + nm_clear_g_source_inst(&priv->notify_platform_timeout_source); + + priv->notify_platform_rlimited_until_msec = now_msec + NOTIFY_PLATFORM_RATELIMIT_MSEC; + + obj_type_flags = nm_steal_int(&priv->notify_platform_obj_type_flags); + + nm_assert(obj_type_flags != 0u); + + _handle_platform_change(self, obj_type_flags, FALSE); +} + +static gboolean +_notify_platform_cb(gpointer user_data) +{ + _notify_platform_handle(user_data, nm_utils_get_monotonic_timestamp_msec()); + return G_SOURCE_CONTINUE; +} + +static void +_notify_platform(NMIPConfig *self, guint32 obj_type_flags) +{ + const int addr_family = nm_ip_config_get_addr_family(self); + const int IS_IPv4 = NM_IS_IPv4(addr_family); + NMIPConfigPrivate *priv = NM_IP_CONFIG_GET_PRIVATE(self); + gint64 now_msec; + + obj_type_flags &= (nmp_object_type_to_flags(NMP_OBJECT_TYPE_IP_ADDRESS(IS_IPv4)) + | nmp_object_type_to_flags(NMP_OBJECT_TYPE_IP_ROUTE(IS_IPv4))); + + if (obj_type_flags == 0u) + return; + + priv->notify_platform_obj_type_flags |= obj_type_flags; + + if (priv->notify_platform_timeout_source) { + /* We are currently rate limited. Don't bother to check whether + * (now_msec < priv->notify_platform_rlimited_until_msec), just always + * delegate to the timeout handler. It is scheduled with a lower idle + * priority, so we want that additional backoff. */ + return; + } + + now_msec = nm_utils_get_monotonic_timestamp_msec(); + + if (now_msec < priv->notify_platform_rlimited_until_msec) { + priv->notify_platform_timeout_source = nm_g_source_attach( + /* Schedule with a low G_PRIORITY_LOW. */ + nm_g_timeout_source_new(priv->notify_platform_rlimited_until_msec - now_msec, + G_PRIORITY_LOW - 10, + _notify_platform_cb, + self, + NULL), + NULL); + return; + } + + _notify_platform_handle(self, now_msec); +} + +/*****************************************************************************/ + static void _l3cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMIPConfig *self) { @@ -76,7 +147,7 @@ _l3cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMIPCo _handle_l3cd_changed(self, notify_data->l3cd_changed.l3cd_new); break; case NM_L3_CONFIG_NOTIFY_TYPE_PLATFORM_CHANGE_ON_IDLE: - _handle_platform_change(self, notify_data->platform_change_on_idle.obj_type_flags, FALSE); + _notify_platform(self, notify_data->platform_change_on_idle.obj_type_flags); break; default: break; @@ -207,6 +278,8 @@ finalize(GObject *object) NMIPConfig *self = NM_IP_CONFIG(object); NMIPConfigPrivate *priv = NM_IP_CONFIG_GET_PRIVATE(self); + nm_clear_g_source_inst(&priv->notify_platform_timeout_source); + nm_clear_g_signal_handler(priv->l3cfg, &priv->l3cfg_notify_id); g_object_unref(priv->l3cfg); diff --git a/src/core/nm-ip-config.h b/src/core/nm-ip-config.h index 0dcea83b51..47a40bd223 100644 --- a/src/core/nm-ip-config.h +++ b/src/core/nm-ip-config.h @@ -32,7 +32,10 @@ struct _NMIPConfigPrivate { struct { const NMPObject *best_default_route; } v_gateway; - gulong l3cfg_notify_id; + GSource *notify_platform_timeout_source; + gint64 notify_platform_rlimited_until_msec; + gulong l3cfg_notify_id; + guint32 notify_platform_obj_type_flags; }; struct _NMIPConfig { From 4bb3ad0977e0bf163334b6cbbfa9cdc0b3d02433 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 20 Nov 2023 12:38:08 +0100 Subject: [PATCH 5/6] core: limit number of exported addresses/routes on D-Bus to 100 It doesn't scale to export all addresses/routes on D-Bus as properties. In particular not combined with PropertiesChanged signal. On a busy system, this causes severe performance issues. It also doesn't seem very useful. Routes and addresses are complex things (e.g. policy routing). If you want to do anything serious, you must check netlink (or find another way to get the information). Note that NMPlatform already ignores routes of certain protocols (ip_route_is_alive()). It also does not expose most route attributes, making the output only useful for very limited cases (e.g. displaying to the user for information). Limit the number of exported entries to 100. Try adding 100K routes one-by-one. Run a `nmcli monitor` instance. Re-nice the nmcli process and/or keep the CPUs busy. Then start a script that adds 100k routes. Observe. Glib's D-Bus worker thread receives the messages and queues them for the main thread. The main thread is too slow to process them, the memory consumption grows very quickly in Giga bytes. Afterwards, the memory also is not returned to the operation system, either because of fragmentation or because the libc allocator does anyway not return heap memory. It doesn't work to expose an unlimited number of objects on D-Bus. At least not with an API, that sends the full list of all routes, whenever a route changes. Nobody can use that feature either, because the only use is a quick overview in `nmcli` output or a GUI. If you see 100+ routes there, that becomes unmanageable anyway. Instead use netlink if you want to handle the full list of addresses/routes (or some other API). --- NEWS | 3 +++ src/core/NetworkManagerUtils.c | 18 +++++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 63f21d0457..4b618a57d2 100644 --- a/NEWS +++ b/NEWS @@ -16,6 +16,9 @@ Overview of changes since NetworkManager-1.44 when set to "org.freedesktop.NetworkManager". * Drop build support with Python2. Python3 is now required. * nmcli: limit number of printed addresses/routes in `nmcli` overview to 10. +* Limit number of exported IP addresses/routes on D-Bus to 100 to reduce + performance cost. Also, D-Bus updates for addresses/routes are now rate + limited to 3 per second. ============================================= NetworkManager-1.44 diff --git a/src/core/NetworkManagerUtils.c b/src/core/NetworkManagerUtils.c index 84379cdf95..e65f7ddae5 100644 --- a/src/core/NetworkManagerUtils.c +++ b/src/core/NetworkManagerUtils.c @@ -1558,7 +1558,8 @@ nm_utils_ip_addresses_to_dbus(int addr_family, char addr_str[NM_INET_ADDRSTRLEN]; NMDedupMultiIter iter; const NMPObject *obj; - guint i; + const gsize MAX_ADDRESSES = 100; + gsize i; nm_assert_addr_family(addr_family); @@ -1580,6 +1581,11 @@ nm_utils_ip_addresses_to_dbus(int addr_family, nm_platform_dedup_multi_iter_next_obj(&iter, &obj, NMP_OBJECT_TYPE_IP_ADDRESS(IS_IPv4))) { const NMPlatformIPXAddress *address = NMP_OBJECT_CAST_IPX_ADDRESS(obj); + if (i > MAX_ADDRESSES) { + /* Limited. The rest is hidden. */ + break; + } + if (out_address_data) { GVariantBuilder addr_builder; gconstpointer p; @@ -1669,6 +1675,8 @@ nm_utils_ip_routes_to_dbus(int addr_family, GVariantBuilder builder_data; GVariantBuilder builder_legacy; char addr_str[NM_INET_ADDRSTRLEN]; + const gsize MAX_ROUTES = 100; + gsize i; nm_assert_addr_family(addr_family); @@ -1681,6 +1689,7 @@ nm_utils_ip_routes_to_dbus(int addr_family, g_variant_builder_init(&builder_legacy, G_VARIANT_TYPE("a(ayuayu)")); } + i = 0; nm_dedup_multi_iter_init(&iter, head_entry); while (nm_platform_dedup_multi_iter_next_obj(&iter, &obj, NMP_OBJECT_TYPE_IP_ROUTE(IS_IPv4))) { const NMPlatformIPXRoute *r = NMP_OBJECT_CAST_IPX_ROUTE(obj); @@ -1699,6 +1708,13 @@ nm_utils_ip_routes_to_dbus(int addr_family, if (r->rx.type_coerced != nm_platform_route_type_coerce(RTN_UNICAST)) continue; + if (i >= MAX_ROUTES) { + /* Limited. The rest is hidden. */ + break; + } + + i++; + if (out_route_data) { GVariantBuilder route_builder; gconstpointer gateway; From a2cd6599d2e27a6cc425a29bc950ef314ede5be7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 20 Nov 2023 12:46:59 +0100 Subject: [PATCH 6/6] cli: don't print how many more addresses/routes in `nmcli` output Since the number of addresses/routes that the daemon exposes on D-Bus is now limited, we don't know how many more there would be. Just say "more". --- src/nmcli/general.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nmcli/general.c b/src/nmcli/general.c index df76e9fa08..6fc8184cb0 100644 --- a/src/nmcli/general.c +++ b/src/nmcli/general.c @@ -1458,8 +1458,8 @@ ac_overview(NmCli *nmc, NMActiveConnection *ac) /* Print always at least MAX_ADDRESSES fully. * If there are MAX_ADDRESSES+1 addresses, print them all fully. * If there are more addresses, print MAX_ADDRESSES fully, and a - * "N more" line. */ - nmc_print("\tinet%c ... %u more\n", IS_IPv4 ? '4' : '6', p->len - i - 1u); + * "more" line. */ + nmc_print("\tinet%c ... more\n", IS_IPv4 ? '4' : '6'); break; } } @@ -1474,7 +1474,7 @@ ac_overview(NmCli *nmc, NMActiveConnection *ac) nmc_print("\troute%c %s\n", IS_IPv4 ? '4' : '6', nm_str_buf_get_str(&str)); if (i >= MAX_ROUTES - 1u && p->len - i > 2u) { - nmc_print("\troute%c ... %u more\n", IS_IPv4 ? '4' : '6', p->len - i - 1u); + nmc_print("\troute%c ... more\n", IS_IPv4 ? '4' : '6'); break; } }