From 0708c4fbdbea7c190cd1c40d95758e829dcc54bc Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 7 Oct 2023 21:34:43 +0900 Subject: [PATCH 1/7] network/tc: align vtables --- src/network/tc/qdisc.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/network/tc/qdisc.c b/src/network/tc/qdisc.c index ca89169a86..7bb95ede75 100644 --- a/src/network/tc/qdisc.c +++ b/src/network/tc/qdisc.c @@ -19,27 +19,27 @@ #include "tc-util.h" const QDiscVTable * const qdisc_vtable[_QDISC_KIND_MAX] = { - [QDISC_KIND_BFIFO] = &bfifo_vtable, - [QDISC_KIND_CAKE] = &cake_vtable, - [QDISC_KIND_CODEL] = &codel_vtable, - [QDISC_KIND_DRR] = &drr_vtable, - [QDISC_KIND_ETS] = &ets_vtable, - [QDISC_KIND_FQ] = &fq_vtable, - [QDISC_KIND_FQ_CODEL] = &fq_codel_vtable, - [QDISC_KIND_FQ_PIE] = &fq_pie_vtable, - [QDISC_KIND_GRED] = &gred_vtable, - [QDISC_KIND_HHF] = &hhf_vtable, - [QDISC_KIND_HTB] = &htb_vtable, - [QDISC_KIND_NETEM] = &netem_vtable, - [QDISC_KIND_PIE] = &pie_vtable, - [QDISC_KIND_QFQ] = &qfq_vtable, - [QDISC_KIND_PFIFO] = &pfifo_vtable, - [QDISC_KIND_PFIFO_FAST] = &pfifo_fast_vtable, + [QDISC_KIND_BFIFO] = &bfifo_vtable, + [QDISC_KIND_CAKE] = &cake_vtable, + [QDISC_KIND_CODEL] = &codel_vtable, + [QDISC_KIND_DRR] = &drr_vtable, + [QDISC_KIND_ETS] = &ets_vtable, + [QDISC_KIND_FQ] = &fq_vtable, + [QDISC_KIND_FQ_CODEL] = &fq_codel_vtable, + [QDISC_KIND_FQ_PIE] = &fq_pie_vtable, + [QDISC_KIND_GRED] = &gred_vtable, + [QDISC_KIND_HHF] = &hhf_vtable, + [QDISC_KIND_HTB] = &htb_vtable, + [QDISC_KIND_NETEM] = &netem_vtable, + [QDISC_KIND_PIE] = &pie_vtable, + [QDISC_KIND_QFQ] = &qfq_vtable, + [QDISC_KIND_PFIFO] = &pfifo_vtable, + [QDISC_KIND_PFIFO_FAST] = &pfifo_fast_vtable, [QDISC_KIND_PFIFO_HEAD_DROP] = &pfifo_head_drop_vtable, - [QDISC_KIND_SFB] = &sfb_vtable, - [QDISC_KIND_SFQ] = &sfq_vtable, - [QDISC_KIND_TBF] = &tbf_vtable, - [QDISC_KIND_TEQL] = &teql_vtable, + [QDISC_KIND_SFB] = &sfb_vtable, + [QDISC_KIND_SFQ] = &sfq_vtable, + [QDISC_KIND_TBF] = &tbf_vtable, + [QDISC_KIND_TEQL] = &teql_vtable, }; static int qdisc_new(QDiscKind kind, QDisc **ret) { From be8e93390003e45acbb318c6e1e48fbc3c772f78 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 7 Oct 2023 14:20:38 +0900 Subject: [PATCH 2/7] network/tc: drop child tree of traffic control nodes on remove When a node of traffic control tree is removed, all child nodes are also removed but their removal are not notified by the kernel. So, previously, removed TC classes or qdiscs under the removed node were kept in the memory of networkd, and may cause failure on reconfigure. --- src/network/tc/qdisc.c | 38 ++++++++++++++++++++++++++++++-------- src/network/tc/qdisc.h | 2 ++ src/network/tc/tclass.c | 38 ++++++++++++++++++++++++++++++-------- src/network/tc/tclass.h | 2 ++ 4 files changed, 64 insertions(+), 16 deletions(-) diff --git a/src/network/tc/qdisc.c b/src/network/tc/qdisc.c index 7bb95ede75..828fe9ab82 100644 --- a/src/network/tc/qdisc.c +++ b/src/network/tc/qdisc.c @@ -293,6 +293,33 @@ int link_find_qdisc(Link *link, uint32_t handle, uint32_t parent, const char *ki return -ENOENT; } +QDisc* qdisc_drop(QDisc *qdisc) { + TClass *tclass; + Link *link; + + assert(qdisc); + + link = ASSERT_PTR(qdisc->link); + + /* also drop all child classes assigned to the qdisc. */ + SET_FOREACH(tclass, link->tclasses) { + if (TC_H_MAJ(tclass->classid) != qdisc->handle) + continue; + + tclass_drop(tclass); + } + + qdisc_enter_removed(qdisc); + + if (qdisc->state == 0) { + log_qdisc_debug(qdisc, link, "Forgetting"); + qdisc = qdisc_free(qdisc); + } else + log_qdisc_debug(qdisc, link, "Removed"); + + return qdisc; +} + static int qdisc_handler(sd_netlink *rtnl, sd_netlink_message *m, Request *req, Link *link, QDisc *qdisc) { int r; @@ -512,14 +539,9 @@ int manager_rtnl_process_qdisc(sd_netlink *rtnl, sd_netlink_message *message, Ma break; case RTM_DELQDISC: - if (qdisc) { - qdisc_enter_removed(qdisc); - if (qdisc->state == 0) { - log_qdisc_debug(qdisc, link, "Forgetting"); - qdisc_free(qdisc); - } else - log_qdisc_debug(qdisc, link, "Removed"); - } else + if (qdisc) + qdisc_drop(qdisc); + else log_qdisc_debug(tmp, link, "Kernel removed unknown"); break; diff --git a/src/network/tc/qdisc.h b/src/network/tc/qdisc.h index 155e2adf24..86db44763b 100644 --- a/src/network/tc/qdisc.h +++ b/src/network/tc/qdisc.h @@ -77,6 +77,8 @@ DEFINE_NETWORK_CONFIG_STATE_FUNCTIONS(QDisc, qdisc); QDisc* qdisc_free(QDisc *qdisc); int qdisc_new_static(QDiscKind kind, Network *network, const char *filename, unsigned section_line, QDisc **ret); +QDisc* qdisc_drop(QDisc *qdisc); + int link_find_qdisc(Link *link, uint32_t handle, uint32_t parent, const char *kind, QDisc **qdisc); int link_request_qdisc(Link *link, QDisc *qdisc); diff --git a/src/network/tc/tclass.c b/src/network/tc/tclass.c index 0452cc99a8..9e6032b35a 100644 --- a/src/network/tc/tclass.c +++ b/src/network/tc/tclass.c @@ -255,6 +255,33 @@ static void log_tclass_debug(TClass *tclass, Link *link, const char *str) { strna(tclass_get_tca_kind(tclass))); } +TClass* tclass_drop(TClass *tclass) { + QDisc *qdisc; + Link *link; + + assert(tclass); + + link = ASSERT_PTR(tclass->link); + + /* Also drop all child qdiscs assigned to the class. */ + SET_FOREACH(qdisc, link->qdiscs) { + if (qdisc->parent != tclass->classid) + continue; + + qdisc_drop(qdisc); + } + + tclass_enter_removed(tclass); + + if (tclass->state == 0) { + log_tclass_debug(tclass, link, "Forgetting"); + tclass = tclass_free(tclass); + } else + log_tclass_debug(tclass, link, "Removed"); + + return tclass; +} + static int tclass_handler(sd_netlink *rtnl, sd_netlink_message *m, Request *req, Link *link, TClass *tclass) { int r; @@ -464,14 +491,9 @@ int manager_rtnl_process_tclass(sd_netlink *rtnl, sd_netlink_message *message, M break; case RTM_DELTCLASS: - if (tclass) { - tclass_enter_removed(tclass); - if (tclass->state == 0) { - log_tclass_debug(tclass, link, "Forgetting"); - tclass_free(tclass); - } else - log_tclass_debug(tclass, link, "Removed"); - } else + if (tclass) + (void) tclass_drop(tclass); + else log_tclass_debug(tmp, link, "Kernel removed unknown"); break; diff --git a/src/network/tc/tclass.h b/src/network/tc/tclass.h index 606bb3fef3..f0ee318c76 100644 --- a/src/network/tc/tclass.h +++ b/src/network/tc/tclass.h @@ -58,6 +58,8 @@ DEFINE_NETWORK_CONFIG_STATE_FUNCTIONS(TClass, tclass); TClass* tclass_free(TClass *tclass); int tclass_new_static(TClassKind kind, Network *network, const char *filename, unsigned section_line, TClass **ret); +TClass* tclass_drop(TClass *tclass); + int link_find_tclass(Link *link, uint32_t classid, TClass **ret); int link_request_tclass(Link *link, TClass *tclass); From 4147618612bc79fc2ea38914ff75826dd4ae4f09 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 7 Oct 2023 14:24:34 +0900 Subject: [PATCH 3/7] network/tc: fix enumeration logic of traffic control classes TC class can be enumerated only per link. --- src/network/networkd-manager.c | 15 ++++++++------- src/network/networkd-manager.h | 5 +++++ src/network/tc/tclass.c | 15 +++++++++++++++ src/network/tc/tclass.h | 1 + 4 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index ab5460db27..f597eb358b 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -716,7 +716,7 @@ int manager_load_config(Manager *m) { return manager_build_dhcp_pd_subnet_ids(m); } -static int manager_enumerate_internal( +int manager_enumerate_internal( Manager *m, sd_netlink *nl, sd_netlink_message *req, @@ -775,17 +775,18 @@ static int manager_enumerate_qdisc(Manager *m) { } static int manager_enumerate_tclass(Manager *m) { - _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL; - int r; + Link *link; + int r = 0; assert(m); assert(m->rtnl); - r = sd_rtnl_message_new_traffic_control(m->rtnl, &req, RTM_GETTCLASS, 0, 0, 0); - if (r < 0) - return r; + /* TC class can be enumerated only per link. See tc_dump_tclass() in net/sched/sched_api.c. */ - return manager_enumerate_internal(m, m->rtnl, req, manager_rtnl_process_tclass); + HASHMAP_FOREACH(link, m->links_by_index) + RET_GATHER(r, link_enumerate_tclass(link, 0)); + + return r; } static int manager_enumerate_addresses(Manager *m) { diff --git a/src/network/networkd-manager.h b/src/network/networkd-manager.h index a27137a845..65bd5073a5 100644 --- a/src/network/networkd-manager.h +++ b/src/network/networkd-manager.h @@ -113,6 +113,11 @@ int manager_start(Manager *m); int manager_load_config(Manager *m); +int manager_enumerate_internal( + Manager *m, + sd_netlink *nl, + sd_netlink_message *req, + int (*process)(sd_netlink *, sd_netlink_message *, Manager *)); int manager_enumerate(Manager *m); int manager_set_hostname(Manager *m, const char *hostname); diff --git a/src/network/tc/tclass.c b/src/network/tc/tclass.c index 9e6032b35a..4e31b819c8 100644 --- a/src/network/tc/tclass.c +++ b/src/network/tc/tclass.c @@ -505,6 +505,21 @@ int manager_rtnl_process_tclass(sd_netlink *rtnl, sd_netlink_message *message, M return 1; } +int link_enumerate_tclass(Link *link, uint32_t parent) { + _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL; + int r; + + assert(link); + assert(link->manager); + assert(link->manager->rtnl); + + r = sd_rtnl_message_new_traffic_control(link->manager->rtnl, &req, RTM_GETTCLASS, link->ifindex, 0, parent); + if (r < 0) + return r; + + return manager_enumerate_internal(link->manager, link->manager->rtnl, req, manager_rtnl_process_tclass); +} + static int tclass_section_verify(TClass *tclass) { int r; diff --git a/src/network/tc/tclass.h b/src/network/tc/tclass.h index f0ee318c76..e73e23c97f 100644 --- a/src/network/tc/tclass.h +++ b/src/network/tc/tclass.h @@ -67,6 +67,7 @@ int link_request_tclass(Link *link, TClass *tclass); void network_drop_invalid_tclass(Network *network); int manager_rtnl_process_tclass(sd_netlink *rtnl, sd_netlink_message *message, Manager *m); +int link_enumerate_tclass(Link *link, uint32_t parent); DEFINE_SECTION_CLEANUP_FUNCTIONS(TClass, tclass_free); From c9e70be162b10230f91affcf0a92953bf89e3143 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 7 Oct 2023 14:25:50 +0900 Subject: [PATCH 4/7] network/tc: re-enumerate traffic control classes when a qdisc created Some kind of qdisc implicitly creates a class for the qdisc, but the created class is not notified by the kernel. So, we need to explicitly enumerate classes after a qdisc is created. --- src/network/tc/qdisc.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/network/tc/qdisc.c b/src/network/tc/qdisc.c index 828fe9ab82..6a4d2043a0 100644 --- a/src/network/tc/qdisc.c +++ b/src/network/tc/qdisc.c @@ -536,6 +536,14 @@ int manager_rtnl_process_qdisc(sd_netlink *rtnl, sd_netlink_message *message, Ma qdisc = TAKE_PTR(tmp); } + if (!m->enumerating) { + /* Some kind of QDisc (e.g. tbf) also create an implicit class under the qdisc, but + * the kernel may not notify about the class. Hence, we need to enumerate classes. */ + r = link_enumerate_tclass(link, qdisc->handle); + if (r < 0) + log_link_warning_errno(link, r, "Failed to enumerate TClass, ignoring: %m"); + } + break; case RTM_DELQDISC: From 19607e4371619d4f56ab0985a3a1673250f0c174 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 7 Oct 2023 14:37:31 +0900 Subject: [PATCH 5/7] network/tc: allow to configure class or qdisc under foreign one Some qdiscs (e.g. tbf) implicitly create class(es) on create. Previously, we could not create any child qdisc under the class, as the implicit class is tagged as foreign. --- src/network/tc/qdisc.c | 3 --- src/network/tc/tclass.c | 3 --- 2 files changed, 6 deletions(-) diff --git a/src/network/tc/qdisc.c b/src/network/tc/qdisc.c index 6a4d2043a0..193d241c5c 100644 --- a/src/network/tc/qdisc.c +++ b/src/network/tc/qdisc.c @@ -276,9 +276,6 @@ int link_find_qdisc(Link *link, uint32_t handle, uint32_t parent, const char *ki if (qdisc->parent != parent) continue; - if (qdisc->source == NETWORK_CONFIG_SOURCE_FOREIGN) - continue; - if (!qdisc_exists(qdisc)) continue; diff --git a/src/network/tc/tclass.c b/src/network/tc/tclass.c index 4e31b819c8..abc9d7c145 100644 --- a/src/network/tc/tclass.c +++ b/src/network/tc/tclass.c @@ -223,9 +223,6 @@ int link_find_tclass(Link *link, uint32_t classid, TClass **ret) { if (tclass->classid != classid) continue; - if (tclass->source == NETWORK_CONFIG_SOURCE_FOREIGN) - continue; - if (!tclass_exists(tclass)) continue; From 9e4d87166f398be8d2a6b0ff1068b8709b764684 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 7 Oct 2023 15:09:13 +0900 Subject: [PATCH 6/7] network/tc: support Parent=X:0 for qdiscs When the minor part of the parent handle is zero, let's check if the corresponding qdisc exists, rather than tc class. --- src/network/tc/qdisc.c | 19 ++++++++++--------- src/network/tc/qdisc.h | 2 +- src/network/tc/tclass.c | 2 +- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/network/tc/qdisc.c b/src/network/tc/qdisc.c index 193d241c5c..f20f410497 100644 --- a/src/network/tc/qdisc.c +++ b/src/network/tc/qdisc.c @@ -262,20 +262,15 @@ static void log_qdisc_debug(QDisc *qdisc, Link *link, const char *str) { strna(qdisc_get_tca_kind(qdisc))); } -int link_find_qdisc(Link *link, uint32_t handle, uint32_t parent, const char *kind, QDisc **ret) { +int link_find_qdisc(Link *link, uint32_t handle, const char *kind, QDisc **ret) { QDisc *qdisc; assert(link); - handle = TC_H_MAJ(handle); - SET_FOREACH(qdisc, link->qdiscs) { if (qdisc->handle != handle) continue; - if (qdisc->parent != parent) - continue; - if (!qdisc_exists(qdisc)) continue; @@ -378,9 +373,15 @@ static bool qdisc_is_ready_to_configure(QDisc *qdisc, Link *link) { return false; /* TC_H_CLSACT == TC_H_INGRESS */ - if (!IN_SET(qdisc->parent, TC_H_ROOT, TC_H_CLSACT) && - link_find_tclass(link, qdisc->parent, NULL) < 0) - return false; + if (!IN_SET(qdisc->parent, TC_H_ROOT, TC_H_CLSACT)) { + if (TC_H_MIN(qdisc->parent) == 0) { + if (link_find_qdisc(link, qdisc->parent, NULL, NULL) < 0) + return false; + } else { + if (link_find_tclass(link, qdisc->parent, NULL) < 0) + return false; + } + } if (QDISC_VTABLE(qdisc) && QDISC_VTABLE(qdisc)->is_ready && diff --git a/src/network/tc/qdisc.h b/src/network/tc/qdisc.h index 86db44763b..a62b9413ec 100644 --- a/src/network/tc/qdisc.h +++ b/src/network/tc/qdisc.h @@ -79,7 +79,7 @@ int qdisc_new_static(QDiscKind kind, Network *network, const char *filename, uns QDisc* qdisc_drop(QDisc *qdisc); -int link_find_qdisc(Link *link, uint32_t handle, uint32_t parent, const char *kind, QDisc **qdisc); +int link_find_qdisc(Link *link, uint32_t handle, const char *kind, QDisc **qdisc); int link_request_qdisc(Link *link, QDisc *qdisc); diff --git a/src/network/tc/tclass.c b/src/network/tc/tclass.c index abc9d7c145..0a5fec0234 100644 --- a/src/network/tc/tclass.c +++ b/src/network/tc/tclass.c @@ -339,7 +339,7 @@ static bool tclass_is_ready_to_configure(TClass *tclass, Link *link) { if (!IN_SET(link->state, LINK_STATE_CONFIGURING, LINK_STATE_CONFIGURED)) return false; - return link_find_qdisc(link, tclass->classid, tclass->parent, tclass_get_tca_kind(tclass), NULL) >= 0; + return link_find_qdisc(link, TC_H_MAJ(tclass->classid), tclass_get_tca_kind(tclass), NULL) >= 0; } static int tclass_process_request(Request *req, Link *link, TClass *tclass) { From 8fc7e073e3228cdd5797804fa71de2508ed337d0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 7 Oct 2023 15:06:37 +0900 Subject: [PATCH 7/7] test-network: extend testcase for tbf For issue #29485. --- test/test-network/conf/25-qdisc-tbf.network | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/test-network/conf/25-qdisc-tbf.network b/test/test-network/conf/25-qdisc-tbf.network index c5f28fb63c..2875630981 100644 --- a/test/test-network/conf/25-qdisc-tbf.network +++ b/test/test-network/conf/25-qdisc-tbf.network @@ -14,3 +14,8 @@ BurstBytes=5000 LatencySec=70msec PeakRate=100G MTUBytes=1000000 + +[PFIFO] +Parent=35:0 +Handle=0037 +PacketLimit=100000