From 1346d0a5302f0f08b7f94483ca8da859fb58eac1 Mon Sep 17 00:00:00 2001 From: Jonas Gorski Date: Tue, 19 Aug 2025 11:24:02 +0200 Subject: [PATCH 1/2] cnetlink: process netlink events synchronously Currently we switch between collecting netlink events and processing them. This makes the processing bursty, and since libnl will have finished applying them to the cache it means the cache will be at a state after applying the last event of the current batch. Change this to process each netlink event as it comes in instead. This ensures that the current libnl caches are in the state of when that event happened. This means that we do not need to look into the future for anymore for objects that get deleted in the same batch. Since we do not queue messages anymore, we need to make sure to be able to handle netlink notifications before we start creating port interfaces, so notify about switch being up earlier and change the state to running. Signed-off-by: Jonas Gorski --- src/netlink/cnetlink.cc | 105 +++++++++++++++++---------------------- src/netlink/cnetlink.h | 2 + src/of-dpa/controller.cc | 3 +- 3 files changed, 49 insertions(+), 61 deletions(-) diff --git a/src/netlink/cnetlink.cc b/src/netlink/cnetlink.cc index e9de4329..92c49819 100644 --- a/src/netlink/cnetlink.cc +++ b/src/netlink/cnetlink.cc @@ -224,6 +224,7 @@ void cnetlink::init_caches() { void cnetlink::init_subsystems() noexcept { assert(swi); l3->init(); + thread.wakeup(this); } void cnetlink::shutdown_subsystems() noexcept { @@ -903,54 +904,6 @@ void cnetlink::handle_wakeup(rofl::cthread &thread) { return; } - // loop through nl_objs - for (int cnt = 0; - cnt < nl_proc_max && nl_objs.size() && state == NL_STATE_RUNNING; - cnt++) { - auto obj = nl_objs.front(); - nl_objs.pop_front(); - - switch (obj.get_msg_type()) { - case RTM_NEWLINK: - case RTM_DELLINK: - route_link_apply(obj); - break; - case RTM_NEWNEIGH: - case RTM_DELNEIGH: - route_neigh_apply(obj); - break; - case RTM_NEWROUTE: - case RTM_DELROUTE: - route_route_apply(obj); - break; - case RTM_NEWNEXTHOP: - case RTM_DELNEXTHOP: - route_nh_apply(obj); - break; - case RTM_NEWADDR: - case RTM_DELADDR: - route_addr_apply(obj); - break; -#ifdef HAVE_NETLINK_ROUTE_MDB_H - case RTM_NEWMDB: - case RTM_DELMDB: - assert(FLAGS_multicast); - route_mdb_apply(obj); - break; -#endif -#ifdef HAVE_NETLINK_ROUTE_BRIDGE_VLAN_H - case RTM_NEWVLAN: - case RTM_DELVLAN: - route_bridge_vlan_apply(obj); - break; -#endif - default: - LOG(ERROR) << __FUNCTION__ << ": unexpected netlink type " - << obj.get_msg_type(); - break; - } - } - if (handle_source_mac_learn()) { do_wakeup = true; } @@ -979,6 +932,48 @@ void cnetlink::handle_read_event(rofl::cthread &thread, int fd) { } } +void cnetlink::netlink_event_apply(const nl_obj &obj) { + switch (obj.get_msg_type()) { + case RTM_NEWLINK: + case RTM_DELLINK: + route_link_apply(obj); + break; + case RTM_NEWNEIGH: + case RTM_DELNEIGH: + route_neigh_apply(obj); + break; + case RTM_NEWROUTE: + case RTM_DELROUTE: + route_route_apply(obj); + break; + case RTM_NEWNEXTHOP: + case RTM_DELNEXTHOP: + route_nh_apply(obj); + break; + case RTM_NEWADDR: + case RTM_DELADDR: + route_addr_apply(obj); + break; +#ifdef HAVE_NETLINK_ROUTE_MDB_H + case RTM_NEWMDB: + case RTM_DELMDB: + assert(FLAGS_multicast); + route_mdb_apply(obj); + break; +#endif +#ifdef HAVE_NETLINK_ROUTE_BRIDGE_VLAN_H + case RTM_NEWVLAN: + case RTM_DELVLAN: + route_bridge_vlan_apply(obj); + break; +#endif + default: + LOG(ERROR) << __FUNCTION__ << ": unexpected netlink type " + << obj.get_msg_type(); + break; + } +} + void cnetlink::handle_write_event(rofl::cthread &thread, int fd) { VLOG(1) << __FUNCTION__ << ": thread=" << thread << ", fd=" << fd; // currently not in use @@ -1022,18 +1017,8 @@ void cnetlink::nl_cb_v2(struct nl_cache *cache, struct nl_object *old_obj, assert(data); auto nl = static_cast(data); - // only enqueue nl msgs if not in stopped state - if (nl->state != NL_STATE_STOPPED) { - // If libnl updated the object instead of replacing it, old_obj will be a - // clone of the old object, and new_obj is the updated old object. Since - // later notifications may update the new_obj further, clone - // it to keep it in the state of the notification. - auto local_new = nl_object_clone(new_obj); - - nl->nl_objs.emplace_back(action, old_obj, local_new); - - nl_object_put(local_new); - } + if (nl->state == NL_STATE_RUNNING) + nl->netlink_event_apply({action, old_obj, new_obj}); } void cnetlink::set_tapmanager(std::shared_ptr pm) { diff --git a/src/netlink/cnetlink.h b/src/netlink/cnetlink.h index 2911df8f..b2942c7c 100644 --- a/src/netlink/cnetlink.h +++ b/src/netlink/cnetlink.h @@ -177,6 +177,8 @@ class cnetlink final : public rofl::cthread_env { int handle_source_mac_learn(); int handle_fdb_timeout(); + void netlink_event_apply(const nl_obj &obj); + void route_addr_apply(const nl_obj &obj); void route_link_apply(const nl_obj &obj); void route_neigh_apply(const nl_obj &obj); diff --git a/src/of-dpa/controller.cc b/src/of-dpa/controller.cc index fe6343ea..5a4fd23c 100644 --- a/src/of-dpa/controller.cc +++ b/src/of-dpa/controller.cc @@ -122,6 +122,8 @@ void controller::handle_dpt_open(rofl::crofdpt &dpt) { ofdpa->ofdpaRxRateSet(rate); + nb->switch_state_notification(nbi::SWITCH_STATE_UP); + dpt.send_features_request(rofl::cauxid(0), 1); dpt.send_desc_stats_request(rofl::cauxid(0), 0, 1); @@ -238,7 +240,6 @@ void controller::handle_desc_stats_reply( // TODO evaluate switch here? - nb->switch_state_notification(nbi::SWITCH_STATE_UP); dpt.send_port_desc_stats_request(rofl::cauxid(0), 0, 2); } From fc1307cb9385dcbe7e1eb4dc8314f7a438d36a71 Mon Sep 17 00:00:00 2001 From: Jonas Gorski Date: Mon, 22 Sep 2025 10:18:14 +0200 Subject: [PATCH 2/2] cnetlink: drop now unused cnetlink::nl_objs Now that we process netlink events as they come in, nl_objs is now unused, and we can drop it and all code referencing it. Signed-off-by: Jonas Gorski --- src/netlink/cnetlink.cc | 95 ++--------------------------------------- src/netlink/cnetlink.h | 1 - 2 files changed, 3 insertions(+), 93 deletions(-) diff --git a/src/netlink/cnetlink.cc b/src/netlink/cnetlink.cc index 92c49819..578bf47d 100644 --- a/src/netlink/cnetlink.cc +++ b/src/netlink/cnetlink.cc @@ -278,24 +278,6 @@ cnetlink::get_link_by_ifindex(int ifindex) const { }, &link); - // check the garbage - if (link == nullptr) { - for (auto &obj : nl_objs) { - if (obj.get_action() != NL_ACT_DEL) - continue; - - if (std::string("route/link") - .compare(nl_object_get_type(obj.get_old_obj())) != 0) - continue; - - if (rtnl_link_get_ifindex(LINK_CAST(obj.get_old_obj())) == ifindex) { - link = LINK_CAST(obj.get_old_obj()); - nl_object_get(OBJ_CAST(link)); - break; - } - } - } - std::unique_ptr ret( link, *rtnl_link_put); return ret; @@ -317,20 +299,6 @@ struct rtnl_link *cnetlink::get_link(int ifindex, int family) const { }, &_link); - if (_link == nullptr) { - // check the garbage - for (auto &obj : nl_objs) { - if (obj.get_action() != NL_ACT_DEL) - continue; - - if (nl_object_match_filter(obj.get_old_obj(), OBJ_CAST(filter.get()))) { - _link = LINK_CAST(obj.get_old_obj()); - VLOG(1) << __FUNCTION__ << ": found deleted link " << _link; - break; - } - } - } - return _link; } @@ -362,21 +330,6 @@ cnetlink::get_route_by_nh_params(const struct nh_params &p) const { }, &route); - if (route == nullptr) { - // check the garbage - for (auto &obj : nl_objs) { - if (obj.get_action() == NL_ACT_NEW) - continue; - - if (nl_object_match_filter(obj.get_old_obj(), OBJ_CAST(filter.get()))) { - nl_object_get(obj.get_old_obj()); - route = ROUTE_CAST(obj.get_old_obj()); - VLOG(1) << __FUNCTION__ << ": found deleted route " << route; - break; - } - } - } - std::unique_ptr ret( route, *rtnl_route_put); return ret; @@ -410,16 +363,6 @@ void cnetlink::get_bridge_ports( list->push_back(LINK_CAST(obj)); }, link_list); - - // check the garbage - for (auto &obj : nl_objs) { - if (obj.get_action() != NL_ACT_DEL) - continue; - - if (nl_object_match_filter(obj.get_old_obj(), OBJ_CAST(filter.get()))) { - link_list->push_back(LINK_CAST(obj.get_old_obj())); - } - } } std::set cnetlink::get_bond_members_by_lag(rtnl_link *bond_link) { @@ -714,38 +657,7 @@ struct rtnl_neigh *cnetlink::get_neighbour(int ifindex, assert(ifindex); assert(a); - auto neigh = rtnl_neigh_get(caches[NL_NEIGH_CACHE], ifindex, a); - - // check the garbage - if (neigh == nullptr) { - for (auto &obj : nl_objs) { - rtnl_neigh *n; - - if (obj.get_action() != NL_ACT_DEL) - continue; - - if (std::string("route/neigh") - .compare(nl_object_get_type(obj.get_old_obj())) != 0) - continue; - - n = NEIGH_CAST(obj.get_old_obj()); - - if (rtnl_neigh_get_ifindex(n) != ifindex) - continue; - - if (rtnl_neigh_get_family(n) != nl_addr_get_family(a)) - continue; - - if (nl_addr_cmp(rtnl_neigh_get_dst(n), a) == 0) { - nl_object_get(OBJ_CAST(n)); - neigh = n; - break; - } - } - } - - // XXX TODO return unique_ptr - return neigh; + return rtnl_neigh_get(caches[NL_NEIGH_CACHE], ifindex, a); } bool cnetlink::is_bridge_interface(int ifindex) const { @@ -912,9 +824,8 @@ void cnetlink::handle_wakeup(rofl::cthread &thread) { do_wakeup = true; } - if (do_wakeup || nl_objs.size()) { - VLOG(3) << __FUNCTION__ - << ": calling wakeup nl_objs.size()=" << nl_objs.size(); + if (do_wakeup) { + VLOG(3) << __FUNCTION__ << ": calling wakeup"; this->thread.wakeup(this); } } diff --git a/src/netlink/cnetlink.h b/src/netlink/cnetlink.h index b2942c7c..01f28ca1 100644 --- a/src/netlink/cnetlink.h +++ b/src/netlink/cnetlink.h @@ -143,7 +143,6 @@ class cnetlink final : public rofl::cthread_env { int nl_proc_max; enum nl_state state; - std::deque nl_objs; std::shared_ptr port_man; nl_bridge *bridge;