From d9b2e9d7ea67698c0d10d14942d3ea92ec33193a Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 14 Sep 2021 17:46:02 +0200 Subject: [PATCH 1/6] platform: add methods to delete tc qdiscs and tfilters Introduce two platform methods to delete tc qdiscs and filters by ifindex and parent. --- src/libnm-platform/nm-linux-platform.c | 86 +++++++++++++++++++++++++- src/libnm-platform/nm-platform.c | 18 ++++++ src/libnm-platform/nm-platform.h | 6 +- 3 files changed, 106 insertions(+), 4 deletions(-) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 4af9bfbd64..a1c03c7dbe 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -8850,7 +8850,79 @@ qdisc_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformQdisc *qdisc) return -NME_UNSPEC; } -/*****************************************************************************/ +static int +tc_delete(NMPlatform *platform, int nlmsgtype, int ifindex, guint32 parent, gboolean log_error) +{ + WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; + gs_free char * errmsg = NULL; + int nle; + char s_buf[256]; + const char * log_tag; + nm_auto_nlmsg struct nl_msg *msg = NULL; + const struct tcmsg tcm = { + .tcm_ifindex = ifindex, + .tcm_parent = parent, + }; + + switch (nlmsgtype) { + case RTM_DELQDISC: + log_tag = "do-delete-qdisc"; + break; + case RTM_DELTFILTER: + log_tag = "do-delete-tfilter"; + break; + default: + nm_assert_not_reached(); + log_tag = "do-delete-tc"; + } + + msg = nlmsg_alloc_simple(nlmsgtype, NMP_NLM_FLAG_F_ECHO); + + if (nlmsg_append_struct(msg, &tcm) < 0) + goto nla_put_failure; + + event_handler_read_netlink(platform, FALSE); + + nle = _nl_send_nlmsg(platform, + msg, + &seq_result, + &errmsg, + DELAYED_ACTION_RESPONSE_TYPE_VOID, + NULL); + if (nle < 0) { + _NMLOG(log_error ? LOGL_ERR : LOGL_DEBUG, + "%s: failed sending netlink request \"%s\" (%d)", + log_tag, + nm_strerror(nle), + -nle); + return -NME_PL_NETLINK; + } + + delayed_action_handle_all(platform, FALSE); + + nm_assert(seq_result); + + _NMLOG((seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK || !log_error) ? LOGL_DEBUG + : LOGL_WARN, + "%s: %s", + log_tag, + wait_for_nl_response_to_string(seq_result, errmsg, s_buf, sizeof(s_buf))); + + if (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK) + return 0; + if (seq_result < 0) + return seq_result; + return -NME_UNSPEC; + +nla_put_failure: + g_return_val_if_reached(-NME_UNSPEC); +} + +static int +qdisc_delete(NMPlatform *platform, int ifindex, guint32 parent, gboolean log_error) +{ + return tc_delete(platform, RTM_DELQDISC, ifindex, parent, log_error); +} static int tfilter_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformTfilter *tfilter) @@ -8893,6 +8965,12 @@ tfilter_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformTfilter *tf return -NME_UNSPEC; } +static int +tfilter_delete(NMPlatform *platform, int ifindex, guint32 parent, gboolean log_error) +{ + return tc_delete(platform, RTM_DELTFILTER, ifindex, parent, log_error); +} + /*****************************************************************************/ static gboolean @@ -9687,8 +9765,10 @@ nm_linux_platform_class_init(NMLinuxPlatformClass *klass) platform_class->routing_rule_add = routing_rule_add; - platform_class->qdisc_add = qdisc_add; - platform_class->tfilter_add = tfilter_add; + platform_class->qdisc_add = qdisc_add; + platform_class->qdisc_delete = qdisc_delete; + platform_class->tfilter_add = tfilter_add; + platform_class->tfilter_delete = tfilter_delete; platform_class->process_events = process_events; } diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index fb9dcf50b8..4a3e7e9b3f 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -5295,6 +5295,15 @@ nm_platform_qdisc_add(NMPlatform *self, NMPNlmFlags flags, const NMPlatformQdisc return klass->qdisc_add(self, flags, qdisc); } +int +nm_platform_qdisc_delete(NMPlatform *self, int ifindex, guint32 parent, gboolean log_error) +{ + _CHECK_SELF(self, klass, -NME_BUG); + + _LOG3D("deleting a qdisc: parent 0x%08x", parent); + return klass->qdisc_delete(self, ifindex, parent, log_error); +} + /** * nm_platform_qdisc_sync: * @self: the #NMPlatform instance @@ -5398,6 +5407,15 @@ nm_platform_tfilter_add(NMPlatform *self, NMPNlmFlags flags, const NMPlatformTfi return klass->tfilter_add(self, flags, tfilter); } +int +nm_platform_tfilter_delete(NMPlatform *self, int ifindex, guint32 parent, gboolean log_error) +{ + _CHECK_SELF(self, klass, -NME_BUG); + + _LOG3D("deleting a tfilter: parent 0x%08x", parent); + return klass->tfilter_delete(self, ifindex, parent, log_error); +} + /** * nm_platform_qdisc_sync: * @self: the #NMPlatform instance diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index 8634f2e243..f1e09a2b91 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -1266,8 +1266,10 @@ typedef struct { const NMPlatformRoutingRule *routing_rule); int (*qdisc_add)(NMPlatform *self, NMPNlmFlags flags, const NMPlatformQdisc *qdisc); + int (*qdisc_delete)(NMPlatform *self, int ifindex, guint32 parent, gboolean log_error); int (*tfilter_add)(NMPlatform *self, NMPNlmFlags flags, const NMPlatformTfilter *tfilter); + int (*tfilter_delete)(NMPlatform *self, int ifindex, guint32 parent, gboolean log_error); } NMPlatformClass; /* NMPlatform signals @@ -2219,10 +2221,12 @@ int nm_platform_routing_rule_add(NMPlatform * self, NMPNlmFlags flags, const NMPlatformRoutingRule *routing_rule); -int nm_platform_qdisc_add(NMPlatform *self, NMPNlmFlags flags, const NMPlatformQdisc *qdisc); +int nm_platform_qdisc_add(NMPlatform *self, NMPNlmFlags flags, const NMPlatformQdisc *qdisc); +int nm_platform_qdisc_delete(NMPlatform *self, int ifindex, guint32 parent, gboolean log_error); gboolean nm_platform_qdisc_sync(NMPlatform *self, int ifindex, GPtrArray *known_qdiscs); int nm_platform_tfilter_add(NMPlatform *self, NMPNlmFlags flags, const NMPlatformTfilter *tfilter); +int nm_platform_tfilter_delete(NMPlatform *self, int ifindex, guint32 parent, gboolean log_error); gboolean nm_platform_tfilter_sync(NMPlatform *self, int ifindex, GPtrArray *known_tfilters); const char *nm_platform_link_to_string(const NMPlatformLink *link, char *buf, gsize len); From 3981bff2a01482d9a6a31958d4de770c48e49e2b Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 14 Sep 2021 17:48:09 +0200 Subject: [PATCH 2/6] core: rework tc sync functions Update nm_platform_qdisc_sync() and nm_platform_tfilter_sync() to avoid looking into the platform cache, so that we no longer require to keep tc and qdiscs in the cache. There is no API in kernel to retrieve tc objects only for a specific interface, so NM had to receive all tc events, even for unmanaged interfaces. This could cause high CPU usage in some scenarios with many objects. Instead, try to delete root qdiscs and filters and then add the known ones. Also, combine the two functions together since they are related. In particular, removing all qdiscs also removes all attached filters. --- src/core/devices/nm-device.c | 8 +- src/core/platform/tests/test-tc.c | 8 +- src/libnm-platform/nm-platform.c | 148 ++++++------------------------ src/libnm-platform/nm-platform.h | 7 +- 4 files changed, 37 insertions(+), 134 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 8e13692703..07c3930520 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -8486,10 +8486,7 @@ tc_commit(NMDevice *self) qdiscs = nm_utils_qdiscs_from_tc_setting(platform, s_tc, ip_ifindex); tfilters = nm_utils_tfilters_from_tc_setting(platform, s_tc, ip_ifindex); - if (!nm_platform_qdisc_sync(platform, ip_ifindex, qdiscs)) - return FALSE; - - if (!nm_platform_tfilter_sync(platform, ip_ifindex, tfilters)) + if (!nm_platform_tc_sync(platform, ip_ifindex, qdiscs, tfilters)) return FALSE; return TRUE; @@ -16054,8 +16051,7 @@ nm_device_cleanup(NMDevice *self, NMDeviceStateReason reason, CleanupType cleanu set_ipv6_token(self, &iid, "::"); if (nm_device_get_applied_setting(self, NM_TYPE_SETTING_TC_CONFIG)) { - nm_platform_tfilter_sync(platform, ifindex, NULL); - nm_platform_qdisc_sync(platform, ifindex, NULL); + nm_platform_tc_sync(platform, ifindex, NULL, NULL); } } } diff --git a/src/core/platform/tests/test-tc.c b/src/core/platform/tests/test-tc.c index cd9536cced..ddac2642b3 100644 --- a/src/core/platform/tests/test-tc.c +++ b/src/core/platform/tests/test-tc.c @@ -57,7 +57,7 @@ test_qdisc1(void) g_ptr_array_add(known, qdisc_new(ifindex, "fq_codel", TC_H_ROOT)); g_ptr_array_add(known, qdisc_new(ifindex, "ingress", TC_H_INGRESS)); - g_assert(nm_platform_qdisc_sync(NM_PLATFORM_GET, ifindex, known)); + g_assert(nm_platform_tc_sync(NM_PLATFORM_GET, ifindex, known, NULL)); plat = qdiscs_lookup(ifindex); g_assert(plat); g_assert_cmpint(plat->len, ==, 2); @@ -97,7 +97,7 @@ test_qdisc_fq_codel(void) obj->qdisc.fq_codel.quantum = 1000; g_ptr_array_add(known, obj); - g_assert(nm_platform_qdisc_sync(NM_PLATFORM_GET, ifindex, known)); + g_assert(nm_platform_tc_sync(NM_PLATFORM_GET, ifindex, known, NULL)); plat = qdiscs_lookup(ifindex); g_assert(plat); g_assert_cmpint(plat->len, ==, 1); @@ -136,7 +136,7 @@ test_qdisc_sfq(void) obj->qdisc.sfq.flows = 256; g_ptr_array_add(known, obj); - g_assert(nm_platform_qdisc_sync(NM_PLATFORM_GET, ifindex, known)); + g_assert(nm_platform_tc_sync(NM_PLATFORM_GET, ifindex, known, NULL)); plat = qdiscs_lookup(ifindex); g_assert(plat); g_assert_cmpint(plat->len, ==, 1); @@ -179,7 +179,7 @@ test_qdisc_tbf(void) obj->qdisc.handle = TC_H_MAKE(0x8005 << 16, 0); g_ptr_array_add(known, obj); - g_assert(nm_platform_qdisc_sync(NM_PLATFORM_GET, ifindex, known)); + g_assert(nm_platform_tc_sync(NM_PLATFORM_GET, ifindex, known, NULL)); plat = qdiscs_lookup(ifindex); g_assert(plat); g_assert_cmpint(plat->len, ==, 2); diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index 4a3e7e9b3f..0426172d4c 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -5304,94 +5304,6 @@ nm_platform_qdisc_delete(NMPlatform *self, int ifindex, guint32 parent, gboolean return klass->qdisc_delete(self, ifindex, parent, log_error); } -/** - * nm_platform_qdisc_sync: - * @self: the #NMPlatform instance - * @ifindex: the ifindex where to configure the qdiscs. - * @known_qdiscs: the list of qdiscs (#NMPObject). - * - * The function promises not to take any reference to the qdisc - * instances from @known_qdiscs, nor to keep them around after - * the function returns. This is important, because it allows the - * caller to pass NMPlatformQdisc instances which "kind" string - * have a limited lifetime. - * - * Returns: %TRUE on success. - */ -gboolean -nm_platform_qdisc_sync(NMPlatform *self, int ifindex, GPtrArray *known_qdiscs) -{ - gs_unref_ptrarray GPtrArray *plat_qdiscs = NULL; - NMPLookup lookup; - guint i; - gboolean success = TRUE; - gs_unref_hashtable GHashTable *known_qdiscs_idx = NULL; - - nm_assert(NM_IS_PLATFORM(self)); - nm_assert(ifindex > 0); - - known_qdiscs_idx = - g_hash_table_new((GHashFunc) nmp_object_id_hash, (GEqualFunc) nmp_object_id_equal); - if (known_qdiscs) { - for (i = 0; i < known_qdiscs->len; i++) { - const NMPObject *q = g_ptr_array_index(known_qdiscs, i); - - if (!g_hash_table_insert(known_qdiscs_idx, (gpointer) q, (gpointer) q)) { - _LOGW("duplicate qdisc %s", nm_platform_qdisc_to_string(&q->qdisc, NULL, 0)); - return FALSE; - } - } - } - - plat_qdiscs = - nm_platform_lookup_clone(self, - nmp_lookup_init_object(&lookup, NMP_OBJECT_TYPE_QDISC, ifindex), - NULL, - NULL); - if (plat_qdiscs) { - for (i = 0; i < plat_qdiscs->len; i++) { - const NMPObject *p = g_ptr_array_index(plat_qdiscs, i); - const NMPObject *k; - - /* look up known qdisc with same parent */ - k = g_hash_table_lookup(known_qdiscs_idx, p); - - if (k) { - const NMPlatformQdisc *qdisc_k = NMP_OBJECT_CAST_QDISC(k); - const NMPlatformQdisc *qdisc_p = NMP_OBJECT_CAST_QDISC(p); - - /* check other fields */ - if (nm_platform_qdisc_cmp_full(qdisc_k, qdisc_p, FALSE) != 0 - || (qdisc_k->handle != qdisc_p->handle && qdisc_k != 0)) { - k = NULL; - } - } - - if (k) { - g_hash_table_remove(known_qdiscs_idx, k); - } else { - /* can't delete qdisc with zero handle */ - if (TC_H_MAJ(p->qdisc.handle) != 0) { - success &= nm_platform_object_delete(self, p); - } - } - } - } - - if (known_qdiscs) { - for (i = 0; i < known_qdiscs->len; i++) { - const NMPObject *q = g_ptr_array_index(known_qdiscs, i); - - if (g_hash_table_contains(known_qdiscs_idx, q)) { - success &= - (nm_platform_qdisc_add(self, NMP_NLM_FLAG_ADD, NMP_OBJECT_CAST_QDISC(q)) >= 0); - } - } - } - - return success; -} - /*****************************************************************************/ int @@ -5417,54 +5329,48 @@ nm_platform_tfilter_delete(NMPlatform *self, int ifindex, guint32 parent, gboole } /** - * nm_platform_qdisc_sync: + * nm_platform_tc_sync: * @self: the #NMPlatform instance - * @ifindex: the ifindex where to configure the qdiscs. + * @ifindex: the ifindex where to configure qdiscs and filters. + * @known_qdiscs: the list of qdiscs (#NMPObject). * @known_tfilters: the list of tfilters (#NMPObject). * - * The function promises not to take any reference to the tfilter - * instances from @known_tfilters, nor to keep them around after - * the function returns. This is important, because it allows the - * caller to pass NMPlatformTfilter instances which "kind" string - * have a limited lifetime. + * The function promises not to take any reference to the + * instances from @known_qdiscs and @known_tfilters, nor to + * keep them around after the function returns. This is important, + * because it allows the caller to pass NMPlatformQdisc and + * NMPlatformTfilter instances which "kind" string have a limited + * lifetime. * * Returns: %TRUE on success. */ gboolean -nm_platform_tfilter_sync(NMPlatform *self, int ifindex, GPtrArray *known_tfilters) +nm_platform_tc_sync(NMPlatform *self, + int ifindex, + GPtrArray * known_qdiscs, + GPtrArray * known_tfilters) { - gs_unref_ptrarray GPtrArray *plat_tfilters = NULL; - NMPLookup lookup; - guint i; - gboolean success = TRUE; - gs_unref_hashtable GHashTable *known_tfilters_idx = NULL; + guint i; + gboolean success = TRUE; nm_assert(NM_IS_PLATFORM(self)); nm_assert(ifindex > 0); - known_tfilters_idx = - g_hash_table_new((GHashFunc) nmp_object_id_hash, (GEqualFunc) nmp_object_id_equal); + nm_platform_qdisc_delete(self, ifindex, TC_H_ROOT, FALSE); + nm_platform_qdisc_delete(self, ifindex, TC_H_INGRESS, FALSE); - if (known_tfilters) { - for (i = 0; i < known_tfilters->len; i++) { - const NMPObject *q = g_ptr_array_index(known_tfilters, i); + /* At this point we can only have a root default qdisc + * (which can't be deleted). Ensure it doesn't have any + * filters attached. + */ + nm_platform_tfilter_delete(self, ifindex, TC_H_ROOT, FALSE); - g_hash_table_insert(known_tfilters_idx, (gpointer) q, (gpointer) q); - } - } + if (known_qdiscs) { + for (i = 0; i < known_qdiscs->len; i++) { + const NMPObject *q = g_ptr_array_index(known_qdiscs, i); - plat_tfilters = - nm_platform_lookup_clone(self, - nmp_lookup_init_object(&lookup, NMP_OBJECT_TYPE_TFILTER, ifindex), - NULL, - NULL); - - if (plat_tfilters) { - for (i = 0; i < plat_tfilters->len; i++) { - const NMPObject *q = g_ptr_array_index(plat_tfilters, i); - - if (!g_hash_table_lookup(known_tfilters_idx, q)) - success &= nm_platform_object_delete(self, q); + success &= + (nm_platform_qdisc_add(self, NMP_NLM_FLAG_ADD, NMP_OBJECT_CAST_QDISC(q)) >= 0); } } diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index f1e09a2b91..f41cf7cb24 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -2223,11 +2223,12 @@ int nm_platform_routing_rule_add(NMPlatform * self, int nm_platform_qdisc_add(NMPlatform *self, NMPNlmFlags flags, const NMPlatformQdisc *qdisc); int nm_platform_qdisc_delete(NMPlatform *self, int ifindex, guint32 parent, gboolean log_error); -gboolean nm_platform_qdisc_sync(NMPlatform *self, int ifindex, GPtrArray *known_qdiscs); - int nm_platform_tfilter_add(NMPlatform *self, NMPNlmFlags flags, const NMPlatformTfilter *tfilter); int nm_platform_tfilter_delete(NMPlatform *self, int ifindex, guint32 parent, gboolean log_error); -gboolean nm_platform_tfilter_sync(NMPlatform *self, int ifindex, GPtrArray *known_tfilters); +gboolean nm_platform_tc_sync(NMPlatform *self, + int ifindex, + GPtrArray * known_qdiscs, + GPtrArray * known_tfilters); const char *nm_platform_link_to_string(const NMPlatformLink *link, char *buf, gsize len); const char *nm_platform_lnk_bridge_to_string(const NMPlatformLnkBridge *lnk, char *buf, gsize len); From e0691f9528296b28bd455a177bdf90f9ab457c8a Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 15 Sep 2021 17:20:36 +0200 Subject: [PATCH 3/6] device: ensure tc_commit() is called only once per activation Stage2 can be called multiple times. Ensure that tc_commit() is only called the first time. This is important now that tc synchronization requires to clear all qdiscs and recreate them. --- src/core/devices/nm-device.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 07c3930520..b70795a817 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -505,6 +505,7 @@ typedef struct _NMDevicePrivate { NMDeviceStageState stage1_sriov_state : 3; bool ip_config_started : 1; + bool tc_committed : 1; char *current_stable_id; @@ -8516,13 +8517,14 @@ activate_stage2_device_config(NMDevice *self) _ethtool_state_set(self); if (!nm_device_sys_iface_state_is_external_or_assume(self)) { - if (!tc_commit(self)) { - _LOGW(LOGD_IP6, "failed applying traffic control rules"); + if (!priv->tc_committed && !tc_commit(self)) { + _LOGW(LOGD_DEVICE, "failed applying traffic control rules"); nm_device_state_changed(self, NM_DEVICE_STATE_FAILED, NM_DEVICE_STATE_REASON_CONFIG_FAILED); return; } + priv->tc_committed = TRUE; } _routing_rules_sync(self, NM_TERNARY_TRUE); @@ -16056,6 +16058,8 @@ nm_device_cleanup(NMDevice *self, NMDeviceStateReason reason, CleanupType cleanu } } + priv->tc_committed = FALSE; + _routing_rules_sync(self, cleanup_type == CLEANUP_TYPE_KEEP ? NM_TERNARY_DEFAULT : NM_TERNARY_FALSE); From c896973debc4e7ec63e35b9e0bbefc083c5b9d27 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 17 Sep 2021 17:22:00 +0200 Subject: [PATCH 4/6] platform: drop test-tc-fake It doesn't seem useful to have a copy of the test which does nothing. --- Makefile.am | 6 ------ src/core/platform/tests/meson.build | 1 - src/core/platform/tests/test-tc.c | 12 +++++------- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/Makefile.am b/Makefile.am index 22634626d0..05406d674e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -4180,7 +4180,6 @@ check_programs += \ src/core/platform/tests/test-platform-general \ src/core/platform/tests/test-route-fake \ src/core/platform/tests/test-route-linux \ - src/core/platform/tests/test-tc-fake \ src/core/platform/tests/test-tc-linux \ $(NULL) @@ -4236,11 +4235,6 @@ src_core_platform_tests_test_route_linux_CPPFLAGS = $(src_core_tests_cppflags_li src_core_platform_tests_test_route_linux_LDFLAGS = $(src_core_platform_tests_ldflags) src_core_platform_tests_test_route_linux_LDADD = $(src_core_platform_tests_libadd) -src_core_platform_tests_test_tc_fake_SOURCES = src/core/platform/tests/test-tc.c -src_core_platform_tests_test_tc_fake_CPPFLAGS = $(src_core_tests_cppflags_fake) -src_core_platform_tests_test_tc_fake_LDFLAGS = $(src_core_platform_tests_ldflags) -src_core_platform_tests_test_tc_fake_LDADD = $(src_core_platform_tests_libadd) - src_core_platform_tests_test_tc_linux_SOURCES = src/core/platform/tests/test-tc.c src_core_platform_tests_test_tc_linux_CPPFLAGS = $(src_core_tests_cppflags_linux) src_core_platform_tests_test_tc_linux_LDFLAGS = $(src_core_platform_tests_ldflags) diff --git a/src/core/platform/tests/meson.build b/src/core/platform/tests/meson.build index 5d55707a08..8824ed2f67 100644 --- a/src/core/platform/tests/meson.build +++ b/src/core/platform/tests/meson.build @@ -14,7 +14,6 @@ test_units = [ ['test-platform-general', 'test-platform-general.c', test_c_flags, default_test_timeout], ['test-route-fake', 'test-route.c', test_fake_c_flags, default_test_timeout], ['test-route-linux', 'test-route.c', test_linux_c_flags, default_test_timeout], - ['test-tc-fake', 'test-tc.c', test_fake_c_flags, default_test_timeout], ['test-tc-linux', 'test-tc.c', test_linux_c_flags, default_test_timeout], ] diff --git a/src/core/platform/tests/test-tc.c b/src/core/platform/tests/test-tc.c index ddac2642b3..8b7e4d4a94 100644 --- a/src/core/platform/tests/test-tc.c +++ b/src/core/platform/tests/test-tc.c @@ -202,7 +202,7 @@ test_qdisc_tbf(void) /*****************************************************************************/ -NMTstpSetupFunc const _nmtstp_setup_platform_func = SETUP; +NMTstpSetupFunc const _nmtstp_setup_platform_func = nm_linux_platform_setup; void _nmtstp_init_tests(int *argc, char ***argv) @@ -213,10 +213,8 @@ _nmtstp_init_tests(int *argc, char ***argv) void _nmtstp_setup_tests(void) { - if (nmtstp_is_root_test()) { - nmtstp_env1_add_test_func("/link/qdisc/1", test_qdisc1, TRUE); - nmtstp_env1_add_test_func("/link/qdisc/fq_codel", test_qdisc_fq_codel, TRUE); - nmtstp_env1_add_test_func("/link/qdisc/sfq", test_qdisc_sfq, TRUE); - nmtstp_env1_add_test_func("/link/qdisc/tbf", test_qdisc_tbf, TRUE); - } + nmtstp_env1_add_test_func("/link/qdisc/1", test_qdisc1, TRUE); + nmtstp_env1_add_test_func("/link/qdisc/fq_codel", test_qdisc_fq_codel, TRUE); + nmtstp_env1_add_test_func("/link/qdisc/sfq", test_qdisc_sfq, TRUE); + nmtstp_env1_add_test_func("/link/qdisc/tbf", test_qdisc_tbf, TRUE); } From 864e4e6369397f6cbface423773e24ff6c774dd2 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 17 Sep 2021 17:32:57 +0200 Subject: [PATCH 5/6] platform: allow disabling caching of tc objects Introduce a construct-only property for platform objects to enable or disable the caching of tc objects. When disabled, the netlink socket doesn't receive netlink events for tc objects, and objects are never added to the cache. This commit doesn't change behavior yet. --- src/core/NetworkManagerUtils.c | 2 +- src/core/platform/tests/test-link.c | 12 ++++----- .../platform/tests/test-platform-general.c | 4 +-- src/libnm-platform/nm-linux-platform.c | 25 ++++++++++++++----- src/libnm-platform/nm-linux-platform.h | 2 +- src/libnm-platform/nm-platform.c | 21 ++++++++++++++++ src/libnm-platform/nm-platform.h | 2 ++ 7 files changed, 52 insertions(+), 16 deletions(-) diff --git a/src/core/NetworkManagerUtils.c b/src/core/NetworkManagerUtils.c index 04e7ef5500..69b8d83ab4 100644 --- a/src/core/NetworkManagerUtils.c +++ b/src/core/NetworkManagerUtils.c @@ -1709,5 +1709,5 @@ nm_platform_get() void nm_linux_platform_setup(void) { - nm_platform_setup(nm_linux_platform_new(FALSE, FALSE)); + nm_platform_setup(nm_linux_platform_new(FALSE, FALSE, TRUE)); } diff --git a/src/core/platform/tests/test-link.c b/src/core/platform/tests/test-link.c index 394274daf3..24e3fd999d 100644 --- a/src/core/platform/tests/test-link.c +++ b/src/core/platform/tests/test-link.c @@ -2751,7 +2751,7 @@ _test_netns_create_platform(void) netns = nmp_netns_new(); g_assert(NMP_IS_NETNS(netns)); - platform = nm_linux_platform_new(TRUE, TRUE); + platform = nm_linux_platform_new(TRUE, TRUE, TRUE); g_assert(NM_IS_LINUX_PLATFORM(platform)); nmp_netns_pop(netns); @@ -2840,7 +2840,7 @@ test_netns_general(gpointer fixture, gconstpointer test_data) if (_check_sysctl_skip()) return; - platform_1 = nm_linux_platform_new(TRUE, TRUE); + platform_1 = nm_linux_platform_new(TRUE, TRUE, TRUE); platform_2 = _test_netns_create_platform(); /* add some dummy devices. The "other-*" devices are there to bump the ifindex */ @@ -2968,7 +2968,7 @@ test_netns_set_netns(gpointer fixture, gconstpointer test_data) if (_test_netns_check_skip()) return; - platforms[0] = platform_0 = nm_linux_platform_new(TRUE, TRUE); + platforms[0] = platform_0 = nm_linux_platform_new(TRUE, TRUE, TRUE); platforms[1] = platform_1 = _test_netns_create_platform(); platforms[2] = platform_2 = _test_netns_create_platform(); @@ -3067,7 +3067,7 @@ test_netns_push(gpointer fixture, gconstpointer test_data) if (_check_sysctl_skip()) return; - pl[0].platform = platform_0 = nm_linux_platform_new(TRUE, TRUE); + pl[0].platform = platform_0 = nm_linux_platform_new(TRUE, TRUE, TRUE); pl[1].platform = platform_1 = _test_netns_create_platform(); pl[2].platform = platform_2 = _test_netns_create_platform(); @@ -3214,7 +3214,7 @@ test_netns_bind_to_path(gpointer fixture, gconstpointer test_data) if (_test_netns_check_skip()) return; - platforms[0] = platform_0 = nm_linux_platform_new(TRUE, TRUE); + platforms[0] = platform_0 = nm_linux_platform_new(TRUE, TRUE, TRUE); platforms[1] = platform_1 = _test_netns_create_platform(); platforms[2] = platform_2 = _test_netns_create_platform(); @@ -3379,7 +3379,7 @@ test_sysctl_netns_switch(void) if (_test_netns_check_skip()) return; - platforms[0] = platform_0 = nm_linux_platform_new(TRUE, TRUE); + platforms[0] = platform_0 = nm_linux_platform_new(TRUE, TRUE, TRUE); platforms[1] = platform_1 = _test_netns_create_platform(); platforms[2] = platform_2 = _test_netns_create_platform(); PL = platforms[nmtst_get_rand_uint32() % 3]; diff --git a/src/core/platform/tests/test-platform-general.c b/src/core/platform/tests/test-platform-general.c index 8474e0427d..8ffd92b5a5 100644 --- a/src/core/platform/tests/test-platform-general.c +++ b/src/core/platform/tests/test-platform-general.c @@ -31,7 +31,7 @@ test_init_linux_platform(void) { gs_unref_object NMPlatform *platform = NULL; - platform = nm_linux_platform_new(TRUE, NM_PLATFORM_NETNS_SUPPORT_DEFAULT); + platform = nm_linux_platform_new(TRUE, NM_PLATFORM_NETNS_SUPPORT_DEFAULT, TRUE); } /*****************************************************************************/ @@ -42,7 +42,7 @@ test_link_get_all(void) gs_unref_object NMPlatform *platform = NULL; gs_unref_ptrarray GPtrArray *links = NULL; - platform = nm_linux_platform_new(TRUE, NM_PLATFORM_NETNS_SUPPORT_DEFAULT); + platform = nm_linux_platform_new(TRUE, NM_PLATFORM_NETNS_SUPPORT_DEFAULT, TRUE); links = nm_platform_link_get_all(platform, TRUE); } diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index a1c03c7dbe..4ea545a434 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -3880,6 +3880,9 @@ _new_from_nl_qdisc(NMPlatform *platform, struct nlmsghdr *nlh, gboolean id_only) const struct tcmsg *tcm; nm_auto_nmpobj NMPObject *obj = NULL; + if (!nm_platform_get_cache_tc(platform)) + return NULL; + if (nlmsg_parse_arr(nlh, sizeof(*tcm), tb, policy) < 0) return NULL; @@ -3980,7 +3983,7 @@ _new_from_nl_qdisc(NMPlatform *platform, struct nlmsghdr *nlh, gboolean id_only) } static NMPObject * -_new_from_nl_tfilter(struct nlmsghdr *nlh, gboolean id_only) +_new_from_nl_tfilter(NMPlatform *platform, struct nlmsghdr *nlh, gboolean id_only) { static const struct nla_policy policy[] = { [TCA_KIND] = {.type = NLA_STRING}, @@ -3989,6 +3992,9 @@ _new_from_nl_tfilter(struct nlmsghdr *nlh, gboolean id_only) NMPObject * obj = NULL; const struct tcmsg *tcm; + if (!nm_platform_get_cache_tc(platform)) + return NULL; + if (nlmsg_parse_arr(nlh, sizeof(*tcm), tb, policy) < 0) return NULL; @@ -4059,7 +4065,7 @@ nmp_object_new_from_nl(NMPlatform * platform, case RTM_NEWTFILTER: case RTM_DELTFILTER: case RTM_GETTFILTER: - return _new_from_nl_tfilter(msghdr, id_only); + return _new_from_nl_tfilter(platform, msghdr, id_only); default: return NULL; } @@ -9466,7 +9472,7 @@ constructed(GObject *_object) priv->udev_client = nm_udev_client_new(NM_MAKE_STRV("net"), handle_udev_event, platform); } - _LOGD("create (%s netns, %s, %s udev)", + _LOGD("create (%s netns, %s, %s udev, %s tc-cache)", !platform->_netns ? "ignore" : "use", !platform->_netns && nmp_netns_is_initial() ? "initial netns" @@ -9477,7 +9483,8 @@ constructed(GObject *_object) nmp_netns_get_current(), nmp_netns_get_current() == nmp_netns_get_initial() ? "/main" : "")), - nm_platform_get_use_udev(platform) ? "use" : "no"); + nm_platform_get_use_udev(platform) ? "use" : "no", + nm_platform_get_cache_tc(platform) ? "use" : "no"); priv->genl = nl_socket_alloc(); g_assert(priv->genl); @@ -9523,10 +9530,14 @@ constructed(GObject *_object) RTNLGRP_IPV6_IFADDR, RTNLGRP_IPV6_ROUTE, RTNLGRP_LINK, - RTNLGRP_TC, 0); g_assert(!nle); + if (nm_platform_get_cache_tc(platform)) { + nle = nl_socket_add_memberships(priv->nlh, RTNLGRP_TC, 0); + nm_assert(!nle); + } + fd = nl_socket_get_fd(priv->nlh); _LOGD("Netlink socket for events established: port=%u, fd=%d", @@ -9610,7 +9621,7 @@ path_is_read_only_fs(const char *path) } NMPlatform * -nm_linux_platform_new(gboolean log_with_ptr, gboolean netns_support) +nm_linux_platform_new(gboolean log_with_ptr, gboolean netns_support, gboolean cache_tc) { gboolean use_udev = FALSE; @@ -9624,6 +9635,8 @@ nm_linux_platform_new(gboolean log_with_ptr, gboolean netns_support) use_udev, NM_PLATFORM_NETNS_SUPPORT, netns_support, + NM_PLATFORM_CACHE_TC, + cache_tc, NULL); } diff --git a/src/libnm-platform/nm-linux-platform.h b/src/libnm-platform/nm-linux-platform.h index 26c31e3e4c..85e0693424 100644 --- a/src/libnm-platform/nm-linux-platform.h +++ b/src/libnm-platform/nm-linux-platform.h @@ -23,6 +23,6 @@ typedef struct _NMLinuxPlatformClass NMLinuxPlatformClass; GType nm_linux_platform_get_type(void); -NMPlatform *nm_linux_platform_new(gboolean log_with_ptr, gboolean netns_support); +NMPlatform *nm_linux_platform_new(gboolean log_with_ptr, gboolean netns_support, gboolean cache_tc); #endif /* __NETWORKMANAGER_LINUX_PLATFORM_H__ */ diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index 0426172d4c..0a7b4458b6 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -176,12 +176,14 @@ enum { PROP_NETNS_SUPPORT, PROP_USE_UDEV, PROP_LOG_WITH_PTR, + PROP_CACHE_TC, LAST_PROP, }; typedef struct _NMPlatformPrivate { bool use_udev : 1; bool log_with_ptr : 1; + bool cache_tc : 1; guint ip4_dev_route_blacklist_check_id; guint ip4_dev_route_blacklist_gc_timeout_id; @@ -212,6 +214,12 @@ nm_platform_get_log_with_ptr(NMPlatform *self) return NM_PLATFORM_GET_PRIVATE(self)->log_with_ptr; } +gboolean +nm_platform_get_cache_tc(NMPlatform *self) +{ + return NM_PLATFORM_GET_PRIVATE(self)->cache_tc; +} + /*****************************************************************************/ guint @@ -8861,6 +8869,10 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps /* construct-only */ priv->log_with_ptr = g_value_get_boolean(value); break; + case PROP_CACHE_TC: + /* construct-only */ + priv->cache_tc = g_value_get_boolean(value); + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); break; @@ -8946,6 +8958,15 @@ nm_platform_class_init(NMPlatformClass *platform_class) TRUE, G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS)); + g_object_class_install_property( + object_class, + PROP_CACHE_TC, + g_param_spec_boolean(NM_PLATFORM_CACHE_TC, + "", + "", + FALSE, + G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS)); + #define SIGNAL(signal, signal_id, method) \ G_STMT_START \ { \ diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index f41cf7cb24..a13f921dc7 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -25,6 +25,7 @@ #define NM_PLATFORM_NETNS_SUPPORT "netns-support" #define NM_PLATFORM_USE_UDEV "use-udev" #define NM_PLATFORM_LOG_WITH_PTR "log-with-ptr" +#define NM_PLATFORM_CACHE_TC "cache-tc" /*****************************************************************************/ @@ -1449,6 +1450,7 @@ nm_platform_route_type_uncoerce(guint8 type_coerced) gboolean nm_platform_get_use_udev(NMPlatform *self); gboolean nm_platform_get_log_with_ptr(NMPlatform *self); +gboolean nm_platform_get_cache_tc(NMPlatform *self); NMPNetns *nm_platform_netns_get(NMPlatform *self); gboolean nm_platform_netns_push(NMPlatform *self, NMPNetns **netns); From d50c4eba9ec7be67920a512b6cdaa620ca6bac69 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 17 Sep 2021 17:48:35 +0200 Subject: [PATCH 6/6] core: disable tc cache by default We no longer use tc objects from the platform cache; disable caching by default. The only exception where the cache is needed is in tc tests, as we look into the platform there to check that objects look as expected. --- src/core/NetworkManagerUtils.c | 6 ++++++ src/core/NetworkManagerUtils.h | 1 + src/core/platform/tests/test-common.c | 4 +++- src/core/platform/tests/test-tc.c | 2 +- 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/core/NetworkManagerUtils.c b/src/core/NetworkManagerUtils.c index 69b8d83ab4..0da8e0a949 100644 --- a/src/core/NetworkManagerUtils.c +++ b/src/core/NetworkManagerUtils.c @@ -1708,6 +1708,12 @@ nm_platform_get() void nm_linux_platform_setup(void) +{ + nm_platform_setup(nm_linux_platform_new(FALSE, FALSE, FALSE)); +} + +void +nm_linux_platform_setup_with_tc_cache(void) { nm_platform_setup(nm_linux_platform_new(FALSE, FALSE, TRUE)); } diff --git a/src/core/NetworkManagerUtils.h b/src/core/NetworkManagerUtils.h index af5b5a5ef2..a2ac732e83 100644 --- a/src/core/NetworkManagerUtils.h +++ b/src/core/NetworkManagerUtils.h @@ -277,6 +277,7 @@ NMPlatform *nm_platform_get(void); #define NM_PLATFORM_GET (nm_platform_get()) void nm_linux_platform_setup(void); +void nm_linux_platform_setup_with_tc_cache(void); /*****************************************************************************/ diff --git a/src/core/platform/tests/test-common.c b/src/core/platform/tests/test-common.c index 2fc9e836ea..ad6bc114bc 100644 --- a/src/core/platform/tests/test-common.c +++ b/src/core/platform/tests/test-common.c @@ -37,7 +37,9 @@ gboolean nmtstp_is_root_test(void) { g_assert(_nmtstp_setup_platform_func); - return _nmtstp_setup_platform_func == nm_linux_platform_setup; + return NM_IN_SET(_nmtstp_setup_platform_func, + nm_linux_platform_setup, + nm_linux_platform_setup_with_tc_cache); } gboolean diff --git a/src/core/platform/tests/test-tc.c b/src/core/platform/tests/test-tc.c index 8b7e4d4a94..ee7b861ec7 100644 --- a/src/core/platform/tests/test-tc.c +++ b/src/core/platform/tests/test-tc.c @@ -202,7 +202,7 @@ test_qdisc_tbf(void) /*****************************************************************************/ -NMTstpSetupFunc const _nmtstp_setup_platform_func = nm_linux_platform_setup; +NMTstpSetupFunc const _nmtstp_setup_platform_func = nm_linux_platform_setup_with_tc_cache; void _nmtstp_init_tests(int *argc, char ***argv)