From a77ea81ef34c05bd2b84260c7bbbf9d32443ca7f Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Mon, 23 Jan 2023 11:57:57 +0100 Subject: [PATCH 1/2] lib: do not reopen a zclient socket for bfd b7ca809d1c ("lib: BFD automatic source selection") has added a dedicated zclient socket for nht tracking. Since the bfd lib is used by daemons that already has a zclient socket, those daemons has now a second zclient socket. However, zebra does not distinguish the two zclient sessions. For example, the interfaces are asked a second via zebra_message_send(zclient, ZEBRA_INTERFACE_ADD, VRF_DEFAULT) in zclient_start(). As a result, callbacks functions like bgp_ifp_create() are called a second time, which causes some processing overhead and might cause bugs. Re-use the existing zclient socket for nht tracking. Note that BFD automatic source selection is only currently implemented in staticd. Other daemons will require to add the following in their ZEBRA_NEXTHOP_UPDATE callback function: > if (zclient->bfd_integration) > bfd_nht_update(&matched, &nhr); Fixes: b7ca809d1c ("lib: BFD automatic source selection") Signed-off-by: Louis Scalbert --- lib/bfd.c | 73 ++++++------------------------------------ lib/bfd.h | 9 ++++++ staticd/static_zebra.c | 5 +++ 3 files changed, 23 insertions(+), 64 deletions(-) diff --git a/lib/bfd.c b/lib/bfd.c index ae402ec5cb..2e184898c4 100644 --- a/lib/bfd.c +++ b/lib/bfd.c @@ -129,8 +129,6 @@ struct bfd_sessions_global { struct thread_master *tm; /** Pointer to zebra client data structure. */ struct zclient *zc; - /** Zebra next hop tracking (NHT) client. */ - struct zclient *nht_zclient; /** Debugging state. */ bool debugging; @@ -147,10 +145,6 @@ static const struct in6_addr i6a_zero; /* * Prototypes */ -static void bfd_nht_zclient_connect(struct thread *thread); - -static void bfd_nht_zclient_connected(struct zclient *zclient); -static int bfd_nht_update(ZAPI_CALLBACK_ARGS); static void bfd_source_cache_get(struct bfd_session_params *session); static void bfd_source_cache_put(struct bfd_session_params *session); @@ -158,15 +152,14 @@ static void bfd_source_cache_put(struct bfd_session_params *session); static inline void bfd_source_cache_register(const struct bfd_source_cache *source) { - zclient_send_rnh(bsglobal.nht_zclient, ZEBRA_NEXTHOP_REGISTER, - &source->address, SAFI_UNICAST, false, false, - source->vrf_id); + zclient_send_rnh(bsglobal.zc, ZEBRA_NEXTHOP_REGISTER, &source->address, + SAFI_UNICAST, false, false, source->vrf_id); } static inline void bfd_source_cache_unregister(const struct bfd_source_cache *source) { - zclient_send_rnh(bsglobal.nht_zclient, ZEBRA_NEXTHOP_UNREGISTER, + zclient_send_rnh(bsglobal.zc, ZEBRA_NEXTHOP_UNREGISTER, &source->address, SAFI_UNICAST, false, false, source->vrf_id); } @@ -1074,20 +1067,11 @@ static int bfd_protocol_integration_finish(void) if (!SLIST_EMPTY(&bsglobal.source_list)) zlog_warn("BFD integration source cache not empty"); - zclient_stop(bsglobal.nht_zclient); - zclient_free(bsglobal.nht_zclient); - return 0; } -static zclient_handler *const bfd_nht_handlers[] = { - [ZEBRA_NEXTHOP_UPDATE] = bfd_nht_update, -}; - void bfd_protocol_integration_init(struct zclient *zc, struct thread_master *tm) { - struct zclient_options bfd_nht_options = zclient_options_default; - /* Initialize data structure. */ TAILQ_INIT(&bsglobal.bsplist); SLIST_INIT(&bsglobal.source_list); @@ -1102,16 +1086,6 @@ void bfd_protocol_integration_init(struct zclient *zc, struct thread_master *tm) /* Send the client registration */ bfd_client_sendmsg(zc, ZEBRA_BFD_CLIENT_REGISTER, VRF_DEFAULT); - /* Start NHT client (for automatic source decisions). */ - bsglobal.nht_zclient = - zclient_new(tm, &bfd_nht_options, bfd_nht_handlers, - array_size(bfd_nht_handlers)); - bsglobal.nht_zclient->sock = -1; - bsglobal.nht_zclient->privs = zc->privs; - bsglobal.nht_zclient->zebra_connected = bfd_nht_zclient_connected; - thread_add_timer(tm, bfd_nht_zclient_connect, bsglobal.nht_zclient, 1, - &bsglobal.nht_zclient->t_connect); - hook_register(frr_fini, bfd_protocol_integration_finish); } @@ -1378,27 +1352,7 @@ static bool bfd_source_cache_update(struct bfd_source_cache *source, return false; } -static void bfd_nht_zclient_connect(struct thread *thread) -{ - struct zclient *zclient = THREAD_ARG(thread); - - if (bsglobal.debugging) - zlog_debug("BFD NHT zclient connection attempt"); - - if (zclient_start(zclient) == -1) { - if (bsglobal.debugging) - zlog_debug("BFD NHT zclient connection failed"); - - thread_add_timer(bsglobal.tm, bfd_nht_zclient_connect, zclient, - 3, &zclient->t_connect); - return; - } - - if (bsglobal.debugging) - zlog_debug("BFD NHT zclient connection succeeded"); -} - -static void bfd_nht_zclient_connected(struct zclient *zclient) +void bfd_nht_zclient_connected(struct zclient *zclient) { struct bfd_source_cache *source; @@ -1409,28 +1363,19 @@ static void bfd_nht_zclient_connected(struct zclient *zclient) bfd_source_cache_register(source); } -static int bfd_nht_update(ZAPI_CALLBACK_ARGS) +int bfd_nht_update(const struct prefix *match, const struct zapi_route *route) { struct bfd_source_cache *source; - struct zapi_route route; - struct prefix match; - - if (!zapi_nexthop_update_decode(zclient->ibuf, &match, &route)) { - zlog_warn("BFD NHT update decode failure"); - return 0; - } - if (cmd != ZEBRA_NEXTHOP_UPDATE) - return 0; if (bsglobal.debugging) - zlog_debug("BFD NHT update for %pFX", &route.prefix); + zlog_debug("BFD NHT update for %pFX", &route->prefix); SLIST_FOREACH (source, &bsglobal.source_list, entry) { - if (source->vrf_id != route.vrf_id) + if (source->vrf_id != route->vrf_id) continue; - if (!prefix_same(&match, &source->address)) + if (!prefix_same(match, &source->address)) continue; - if (bfd_source_cache_update(source, &route)) + if (bfd_source_cache_update(source, route)) bfd_source_cache_update_sessions(source); } diff --git a/lib/bfd.h b/lib/bfd.h index b7e4eea5f3..91e6d77993 100644 --- a/lib/bfd.h +++ b/lib/bfd.h @@ -473,6 +473,15 @@ extern bool bfd_protocol_integration_debug(void); */ extern bool bfd_protocol_integration_shutting_down(void); +extern void bfd_nht_zclient_connected(struct zclient *zclient); + +/* Update nexthop-tracking (nht) information for BFD auto source selection. + * The function must be called from the daemon callback function + * that deals with the ZEBRA_NEXTHOP_UPDATE zclient command + */ +extern int bfd_nht_update(const struct prefix *match, + const struct zapi_route *route); + #ifdef __cplusplus } #endif diff --git a/staticd/static_zebra.c b/staticd/static_zebra.c index 316247adb3..f4f9878e8d 100644 --- a/staticd/static_zebra.c +++ b/staticd/static_zebra.c @@ -182,6 +182,8 @@ static void zebra_connected(struct zclient *zclient) zclient_send_reg_requests(zclient, VRF_DEFAULT); static_fixup_vrf_ids(vrf_info_lookup(VRF_DEFAULT)); + + bfd_nht_zclient_connected(zclient); } /* API to check whether the configured nexthop address is @@ -211,6 +213,9 @@ static int static_zebra_nexthop_update(ZAPI_CALLBACK_ARGS) return 1; } + if (zclient->bfd_integration) + bfd_nht_update(&matched, &nhr); + if (matched.family == AF_INET6) afi = AFI_IP6; From f6e7fbdae9660d7abbb344b5e19fc68fb87bda78 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Mon, 23 Jan 2023 13:33:22 +0100 Subject: [PATCH 2/2] lib: remove concurrent nexthop zapi (un)registration Daemons like staticd already implement nexthop zapi (un)registration. b7ca809d1c ("lib: BFD automatic source selection") has implemented a concurrent nexthop (un)registration. Some nexthop could be unregistred by the bfd whereas they were still needed by the daemon. Let the deamons deal with nexthop zapi (un)registration. Fixes: b7ca809d1c ("lib: BFD automatic source selection") Signed-off-by: Louis Scalbert --- lib/bfd.c | 30 ------------------------------ lib/bfd.h | 2 -- staticd/static_zebra.c | 2 -- 3 files changed, 34 deletions(-) diff --git a/lib/bfd.c b/lib/bfd.c index 2e184898c4..9078e3559f 100644 --- a/lib/bfd.c +++ b/lib/bfd.c @@ -149,22 +149,6 @@ static const struct in6_addr i6a_zero; static void bfd_source_cache_get(struct bfd_session_params *session); static void bfd_source_cache_put(struct bfd_session_params *session); -static inline void -bfd_source_cache_register(const struct bfd_source_cache *source) -{ - zclient_send_rnh(bsglobal.zc, ZEBRA_NEXTHOP_REGISTER, &source->address, - SAFI_UNICAST, false, false, source->vrf_id); -} - -static inline void -bfd_source_cache_unregister(const struct bfd_source_cache *source) -{ - zclient_send_rnh(bsglobal.zc, ZEBRA_NEXTHOP_UNREGISTER, - &source->address, SAFI_UNICAST, false, false, - source->vrf_id); -} - - /* * bfd_get_peer_info - Extract the Peer information for which the BFD session * went down from the message sent from Zebra to clients. @@ -1226,8 +1210,6 @@ static void bfd_source_cache_get(struct bfd_session_params *session) session->source_cache = source; source->refcount = 1; - bfd_source_cache_register(source); - return; } @@ -1242,7 +1224,6 @@ static void bfd_source_cache_put(struct bfd_session_params *session) return; } - bfd_source_cache_unregister(session->source_cache); SLIST_REMOVE(&bsglobal.source_list, session->source_cache, bfd_source_cache, entry); XFREE(MTYPE_BFD_SOURCE, session->source_cache); @@ -1352,17 +1333,6 @@ static bool bfd_source_cache_update(struct bfd_source_cache *source, return false; } -void bfd_nht_zclient_connected(struct zclient *zclient) -{ - struct bfd_source_cache *source; - - if (bsglobal.debugging) - zlog_debug("BFD NHT zclient connected"); - - SLIST_FOREACH (source, &bsglobal.source_list, entry) - bfd_source_cache_register(source); -} - int bfd_nht_update(const struct prefix *match, const struct zapi_route *route) { struct bfd_source_cache *source; diff --git a/lib/bfd.h b/lib/bfd.h index 91e6d77993..51e831785a 100644 --- a/lib/bfd.h +++ b/lib/bfd.h @@ -473,8 +473,6 @@ extern bool bfd_protocol_integration_debug(void); */ extern bool bfd_protocol_integration_shutting_down(void); -extern void bfd_nht_zclient_connected(struct zclient *zclient); - /* Update nexthop-tracking (nht) information for BFD auto source selection. * The function must be called from the daemon callback function * that deals with the ZEBRA_NEXTHOP_UPDATE zclient command diff --git a/staticd/static_zebra.c b/staticd/static_zebra.c index f4f9878e8d..85e4b1c033 100644 --- a/staticd/static_zebra.c +++ b/staticd/static_zebra.c @@ -182,8 +182,6 @@ static void zebra_connected(struct zclient *zclient) zclient_send_reg_requests(zclient, VRF_DEFAULT); static_fixup_vrf_ids(vrf_info_lookup(VRF_DEFAULT)); - - bfd_nht_zclient_connected(zclient); } /* API to check whether the configured nexthop address is