From 67cb00401f52cc26eecfd2335c681451ffb214ee Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 5 Oct 2022 10:26:07 -0400 Subject: [PATCH 1/8] zebra: Return statements do not use paranthesis Signed-off-by: Donald Sharp --- zebra/zebra_rib.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index fceaaaa9f026..ca4ba6e6af8d 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -1519,8 +1519,7 @@ static bool rib_route_match_ctx(const struct route_entry *re, } done: - - return (result); + return result; } static void zebra_rib_fixup_system(struct route_node *rn) From 4e1380811003203886fc7396c806c73c45a28c2d Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 5 Oct 2022 10:04:11 -0400 Subject: [PATCH 2/8] zebra: Add `zrouter.asic_notification_nexthop_control` Volta submitted notification changes for the dplane that had a special use case for their system. Volta is no more, the code is not being actively developed and from talking with ex-Volta employees there is no current plans to even maintain this code. Wrap the special handling of nexthops that their asic-dataplane did in a bit of code to isolate it and allow for future removal, as that I do not actually believe anyone else is using this code. Add a CPP_NOTICE several years into the future that will tell us to remove the code. If someone starts using it then they will have to notice this variable to set it and hopefully they will see my CPP_NOTICE to come talk to us. If this is being used then we can just remove this wrapper. Signed-off-by: Donald Sharp --- zebra/zebra_rib.c | 95 +++++++++++++++++++++++++------------------- zebra/zebra_router.c | 11 +++++ zebra/zebra_router.h | 8 ++++ zebra/zebra_vty.c | 9 +++++ 4 files changed, 83 insertions(+), 40 deletions(-) diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index ca4ba6e6af8d..a5c1e354f06f 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -2328,55 +2328,70 @@ static void rib_process_dplane_notify(struct zebra_dplane_ctx *ctx) /* Various fib transitions: changed nexthops; from installed to * not-installed; or not-installed to installed. */ - if (start_count > 0 && end_count > 0) { - if (debug_p) - zlog_debug( - "%s(%u:%u):%pRN applied nexthop changes from dplane notification", - VRF_LOGNAME(vrf), dplane_ctx_get_vrf(ctx), - dplane_ctx_get_table(ctx), rn); + if (zrouter.asic_notification_nexthop_control) { + if (start_count > 0 && end_count > 0) { + if (debug_p) + zlog_debug( + "%s(%u:%u):%pRN applied nexthop changes from dplane notification", + VRF_LOGNAME(vrf), + dplane_ctx_get_vrf(ctx), + dplane_ctx_get_table(ctx), rn); - /* Changed nexthops - update kernel/others */ - dplane_route_notif_update(rn, re, - DPLANE_OP_ROUTE_UPDATE, ctx); + /* Changed nexthops - update kernel/others */ + dplane_route_notif_update(rn, re, + DPLANE_OP_ROUTE_UPDATE, ctx); - } else if (start_count == 0 && end_count > 0) { - if (debug_p) - zlog_debug( - "%s(%u:%u):%pRN installed transition from dplane notification", - VRF_LOGNAME(vrf), dplane_ctx_get_vrf(ctx), - dplane_ctx_get_table(ctx), rn); + } else if (start_count == 0 && end_count > 0) { + if (debug_p) + zlog_debug( + "%s(%u:%u):%pRN installed transition from dplane notification", + VRF_LOGNAME(vrf), + dplane_ctx_get_vrf(ctx), + dplane_ctx_get_table(ctx), rn); - /* We expect this to be the selected route, so we want - * to tell others about this transition. - */ - SET_FLAG(re->status, ROUTE_ENTRY_INSTALLED); + /* We expect this to be the selected route, so we want + * to tell others about this transition. + */ + SET_FLAG(re->status, ROUTE_ENTRY_INSTALLED); - /* Changed nexthops - update kernel/others */ - dplane_route_notif_update(rn, re, DPLANE_OP_ROUTE_UPDATE, ctx); + /* Changed nexthops - update kernel/others */ + dplane_route_notif_update(rn, re, + DPLANE_OP_ROUTE_UPDATE, ctx); - /* Redistribute, lsp, and nht update */ - redistribute_update(rn, re, NULL); + /* Redistribute, lsp, and nht update */ + redistribute_update(rn, re, NULL); - } else if (start_count > 0 && end_count == 0) { - if (debug_p) - zlog_debug( - "%s(%u:%u):%pRN un-installed transition from dplane notification", - VRF_LOGNAME(vrf), dplane_ctx_get_vrf(ctx), - dplane_ctx_get_table(ctx), rn); + } else if (start_count > 0 && end_count == 0) { + if (debug_p) + zlog_debug( + "%s(%u:%u):%pRN un-installed transition from dplane notification", + VRF_LOGNAME(vrf), + dplane_ctx_get_vrf(ctx), + dplane_ctx_get_table(ctx), rn); - /* Transition from _something_ installed to _nothing_ - * installed. - */ - /* We expect this to be the selected route, so we want - * to tell others about this transistion. - */ - UNSET_FLAG(re->status, ROUTE_ENTRY_INSTALLED); + /* Transition from _something_ installed to _nothing_ + * installed. + */ + /* We expect this to be the selected route, so we want + * to tell others about this transistion. + */ + UNSET_FLAG(re->status, ROUTE_ENTRY_INSTALLED); - /* Changed nexthops - update kernel/others */ - dplane_route_notif_update(rn, re, DPLANE_OP_ROUTE_DELETE, ctx); + /* Changed nexthops - update kernel/others */ + dplane_route_notif_update(rn, re, + DPLANE_OP_ROUTE_DELETE, ctx); - /* Redistribute, lsp, and nht update */ - redistribute_delete(rn, re, NULL); + /* Redistribute, lsp, and nht update */ + redistribute_delete(rn, re, NULL); + } + } + + if (!zebra_router_notify_on_ack()) { + if (CHECK_FLAG(re->flags, ZEBRA_FLAG_OFFLOADED)) + zsend_route_notify_owner_ctx(ctx, ZAPI_ROUTE_INSTALLED); + if (CHECK_FLAG(re->flags, ZEBRA_FLAG_OFFLOAD_FAILED)) + zsend_route_notify_owner_ctx(ctx, + ZAPI_ROUTE_FAIL_INSTALL); } /* Make any changes visible for lsp and nexthop-tracking processing */ diff --git a/zebra/zebra_router.c b/zebra/zebra_router.c index c66849863e86..d140fa6d1ef2 100644 --- a/zebra/zebra_router.c +++ b/zebra/zebra_router.c @@ -315,6 +315,17 @@ void zebra_router_init(bool asic_offload, bool notify_on_ack) zrouter.asic_offloaded = asic_offload; zrouter.notify_on_ack = notify_on_ack; + /* + * If you start using asic_notification_nexthop_control + * come talk to the FRR community about what you are doing + * We would like to know. + */ +#if CONFDATE > 20251231 + CPP_NOTICE( + "Remove zrouter.asic_notification_nexthop_control as that it's not being maintained or used"); +#endif + zrouter.asic_notification_nexthop_control = false; + #ifdef HAVE_SCRIPTING zebra_script_init(); #endif diff --git a/zebra/zebra_router.h b/zebra/zebra_router.h index 992bcd5c08c0..1484434aa372 100644 --- a/zebra/zebra_router.h +++ b/zebra/zebra_router.h @@ -220,6 +220,14 @@ struct zebra_router { bool asic_offloaded; bool notify_on_ack; + /* + * If the asic is notifying us about successful nexthop + * allocation/control. Some developers have made their + * asic take control of how many nexthops/ecmp they can + * have and will report what is successfull or not + */ + bool asic_notification_nexthop_control; + bool supports_nhgs; bool all_mc_forwardingv4, default_mc_forwardingv4; diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c index 525e0366e782..b4ae2c50432b 100644 --- a/zebra/zebra_vty.c +++ b/zebra/zebra_vty.c @@ -3999,6 +3999,15 @@ DEFUN (show_zebra, ttable_add_row(table, "ASIC offload|%s", zrouter.asic_offloaded ? "Used" : "Unavailable"); + /* + * Do not display this unless someone is actually using it + * + * Why this distinction? I think this is effectively dead code + * and should not be exposed. Maybe someone proves me wrong. + */ + if (zrouter.asic_notification_nexthop_control) + ttable_add_row(table, "ASIC offload and nexthop control|Used"); + ttable_add_row(table, "RA|%s", rtadv_compiled_in() ? "Compiled in" : "Not Compiled in"); ttable_add_row(table, "RFC 5549|%s", From 8614d570775c2f5dcc2ac38c90f5dc874742bc3b Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 5 Oct 2022 11:28:43 -0400 Subject: [PATCH 3/8] zebra: Re-arrange fpm_read to reduce code duplication Signed-off-by: Donald Sharp --- zebra/dplane_fpm_nl.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/zebra/dplane_fpm_nl.c b/zebra/dplane_fpm_nl.c index d07c4c63324b..f5c85dc3770d 100644 --- a/zebra/dplane_fpm_nl.c +++ b/zebra/dplane_fpm_nl.c @@ -466,13 +466,6 @@ static void fpm_read(struct thread *t) /* Let's ignore the input at the moment. */ rv = stream_read_try(fnc->ibuf, fnc->socket, STREAM_WRITEABLE(fnc->ibuf)); - /* We've got an interruption. */ - if (rv == -2) { - /* Schedule next read. */ - thread_add_read(fnc->fthread->master, fpm_read, fnc, - fnc->socket, &fnc->t_read); - return; - } if (rv == 0) { atomic_fetch_add_explicit(&fnc->counters.connection_closes, 1, memory_order_relaxed); @@ -491,14 +484,23 @@ static void fpm_read(struct thread *t) FPM_RECONNECT(fnc); return; } + + /* Schedule the next read */ + thread_add_read(fnc->fthread->master, fpm_read, fnc, fnc->socket, + &fnc->t_read); + /* We've got an interruption. */ + if (rv == -2) { + /* Schedule next read. */ + thread_add_read(fnc->fthread->master, fpm_read, fnc, + fnc->socket, &fnc->t_read); + return; + } + stream_reset(fnc->ibuf); /* Account all bytes read. */ atomic_fetch_add_explicit(&fnc->counters.bytes_read, rv, memory_order_relaxed); - - thread_add_read(fnc->fthread->master, fpm_read, fnc, fnc->socket, - &fnc->t_read); } static void fpm_write(struct thread *t) From e3296ff3c738c5e6ab13ac883df52a2359483045 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 3 Oct 2022 13:13:50 -0400 Subject: [PATCH 4/8] zebra: Remove goto's that do not do anything special If we have this semantics: int ret = FAILURE; if (foo) goto done; .... done: return ret; This pattern does us no favors and makes it harder to figure out what is going on. Let's remove. Signed-off-by: Donald Sharp --- zebra/zebra_dplane.c | 94 +++++++++++++++----------------------------- 1 file changed, 32 insertions(+), 62 deletions(-) diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index 6a691a222f63..9a7ddcf20fcc 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -2633,7 +2633,7 @@ int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, struct dplane_intf_extra *if_extra; if (!ctx || !rn || !re) - goto done; + return ret; TAILQ_INIT(&ctx->u.rinfo.intf_extra_q); @@ -2724,8 +2724,7 @@ int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, /* Don't need some info when capturing a system notification */ if (op == DPLANE_OP_SYS_ROUTE_ADD || op == DPLANE_OP_SYS_ROUTE_DELETE) { - ret = AOK; - goto done; + return AOK; } /* Extract ns info - can't use pointers to 'core' structs */ @@ -2746,14 +2745,12 @@ int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, * If its a delete we only use the prefix anyway, so this only * matters for INSTALL/UPDATE. */ - if (zebra_nhg_kernel_nexthops_enabled() - && (((op == DPLANE_OP_ROUTE_INSTALL) - || (op == DPLANE_OP_ROUTE_UPDATE)) - && !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED) - && !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_QUEUED))) { - ret = ENOENT; - goto done; - } + if (zebra_nhg_kernel_nexthops_enabled() && + (((op == DPLANE_OP_ROUTE_INSTALL) || + (op == DPLANE_OP_ROUTE_UPDATE)) && + !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED) && + !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_QUEUED))) + return ENOENT; re->nhe_installed_id = nhe->id; } @@ -2765,10 +2762,7 @@ int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, re->dplane_sequence = zebra_router_get_next_sequence(); ctx->zd_seq = re->dplane_sequence; - ret = AOK; - -done: - return ret; + return AOK; } int dplane_ctx_tc_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op) @@ -2807,7 +2801,7 @@ int dplane_ctx_nexthop_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, int ret = EINVAL; if (!ctx || !nhe) - goto done; + return ret; ctx->zd_op = op; ctx->zd_status = ZEBRA_DPLANE_REQUEST_SUCCESS; @@ -2842,7 +2836,6 @@ int dplane_ctx_nexthop_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, ret = AOK; -done: return ret; } @@ -2864,7 +2857,7 @@ int dplane_ctx_intf_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, bool set_pdown, unset_pdown; if (!ctx || !ifp) - goto done; + return ret; ctx->zd_op = op; ctx->zd_status = ZEBRA_DPLANE_REQUEST_SUCCESS; @@ -2909,7 +2902,6 @@ int dplane_ctx_intf_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, ret = AOK; -done: return ret; } @@ -2937,10 +2929,8 @@ int dplane_ctx_lsp_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, /* This may be called to create/init a dplane context, not necessarily * to copy an lsp object. */ - if (lsp == NULL) { - ret = AOK; - goto done; - } + if (lsp == NULL) + return ret; if (IS_ZEBRA_DEBUG_DPLANE_DETAIL) zlog_debug("init dplane ctx %s: in-label %u ecmp# %d", @@ -2983,7 +2973,7 @@ int dplane_ctx_lsp_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, } if (ret != AOK) - goto done; + return ret; /* Capture backup nhlfes/nexthops */ frr_each(nhlfe_list, &lsp->backup_nhlfe_list, nhlfe) { @@ -3004,11 +2994,6 @@ int dplane_ctx_lsp_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, new_nhlfe->nexthop->flags = nhlfe->nexthop->flags; } - /* On error the ctx will be cleaned-up, so we don't need to - * deal with any allocated nhlfe or nexthop structs here. - */ -done: - return ret; } @@ -3069,11 +3054,11 @@ static int dplane_ctx_pw_init(struct zebra_dplane_ctx *ctx, afi = (pw->af == AF_INET) ? AFI_IP : AFI_IP6; table = zebra_vrf_table(afi, SAFI_UNICAST, pw->vrf_id); if (table == NULL) - goto done; + return ret; rn = route_node_match(table, &p); if (rn == NULL) - goto done; + return ret; re = NULL; RNODE_FOREACH_RE(rn, re) { @@ -3141,10 +3126,7 @@ static int dplane_ctx_pw_init(struct zebra_dplane_ctx *ctx, } route_unlock_node(rn); - ret = AOK; - -done: - return ret; + return AOK; } /** @@ -3602,12 +3584,11 @@ enum zebra_dplane_result dplane_route_add(struct route_node *rn, enum zebra_dplane_result ret = ZEBRA_DPLANE_REQUEST_FAILURE; if (rn == NULL || re == NULL) - goto done; + return ret; ret = dplane_route_update_internal(rn, re, NULL, DPLANE_OP_ROUTE_INSTALL); -done: return ret; } @@ -3621,11 +3602,11 @@ enum zebra_dplane_result dplane_route_update(struct route_node *rn, enum zebra_dplane_result ret = ZEBRA_DPLANE_REQUEST_FAILURE; if (rn == NULL || re == NULL) - goto done; + return ret; ret = dplane_route_update_internal(rn, re, old_re, DPLANE_OP_ROUTE_UPDATE); -done: + return ret; } @@ -3638,12 +3619,11 @@ enum zebra_dplane_result dplane_route_delete(struct route_node *rn, enum zebra_dplane_result ret = ZEBRA_DPLANE_REQUEST_FAILURE; if (rn == NULL || re == NULL) - goto done; + return ret; ret = dplane_route_update_internal(rn, re, NULL, DPLANE_OP_ROUTE_DELETE); -done: return ret; } @@ -3656,18 +3636,16 @@ enum zebra_dplane_result dplane_sys_route_add(struct route_node *rn, enum zebra_dplane_result ret = ZEBRA_DPLANE_REQUEST_FAILURE; /* Ignore this event unless a provider plugin has requested it. */ - if (!zdplane_info.dg_sys_route_notifs) { - ret = ZEBRA_DPLANE_REQUEST_SUCCESS; - goto done; - } + if (!zdplane_info.dg_sys_route_notifs) + return ZEBRA_DPLANE_REQUEST_SUCCESS; + if (rn == NULL || re == NULL) - goto done; + return ret; ret = dplane_route_update_internal(rn, re, NULL, DPLANE_OP_SYS_ROUTE_ADD); -done: return ret; } @@ -3680,18 +3658,15 @@ enum zebra_dplane_result dplane_sys_route_del(struct route_node *rn, enum zebra_dplane_result ret = ZEBRA_DPLANE_REQUEST_FAILURE; /* Ignore this event unless a provider plugin has requested it. */ - if (!zdplane_info.dg_sys_route_notifs) { - ret = ZEBRA_DPLANE_REQUEST_SUCCESS; - goto done; - } + if (!zdplane_info.dg_sys_route_notifs) + return ZEBRA_DPLANE_REQUEST_SUCCESS; if (rn == NULL || re == NULL) - goto done; + return ret; ret = dplane_route_update_internal(rn, re, NULL, DPLANE_OP_SYS_ROUTE_DELETE); -done: return ret; } @@ -6109,7 +6084,7 @@ int dplane_clean_ctx_queue(bool (*context_cb)(struct zebra_dplane_ctx *ctx, TAILQ_INIT(&work_list); if (context_cb == NULL) - goto done; + return AOK; /* Walk the pending context queue under the dplane lock. */ DPLANE_LOCK(); @@ -6133,9 +6108,7 @@ int dplane_clean_ctx_queue(bool (*context_cb)(struct zebra_dplane_ctx *ctx, dplane_ctx_fini(&ctx); } -done: - - return 0; + return AOK; } /* Indicates zebra shutdown/exit is in progress. Some operations may be @@ -6199,10 +6172,8 @@ static bool dplane_work_pending(void) } DPLANE_UNLOCK(); - if (ctx != NULL) { - ret = true; - goto done; - } + if (ctx != NULL) + return true; while (prov) { @@ -6225,7 +6196,6 @@ static bool dplane_work_pending(void) if (ctx != NULL) ret = true; -done: return ret; } From 5d0a7599020c15112653b8ca52d7c1e425b6fe24 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 3 Oct 2022 15:28:48 -0400 Subject: [PATCH 5/8] zebra: Add dplane_ctx_get|set_flags Zebra needs the ability to pass this data around. Add it to the dplanes ability to pass. Signed-off-by: Donald Sharp zebra: Add a dplane_ctx_set_flags The dplane_ctx_set_flags call is missing, we will need it. Add it. Signed-off-by: Donald Sharp --- zebra/zebra_dplane.c | 17 +++++++++++++++++ zebra/zebra_dplane.h | 3 +++ 2 files changed, 20 insertions(+) diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index 9a7ddcf20fcc..3b8ac65a9f35 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -134,6 +134,8 @@ struct dplane_route_info { uint32_t zd_mtu; uint32_t zd_nexthop_mtu; + uint32_t zd_flags; + /* Nexthop hash entry info */ struct dplane_nexthop_info nhe; @@ -1406,6 +1408,20 @@ uint16_t dplane_ctx_get_old_instance(const struct zebra_dplane_ctx *ctx) return ctx->u.rinfo.zd_old_instance; } +uint32_t dplane_ctx_get_flags(const struct zebra_dplane_ctx *ctx) +{ + DPLANE_CTX_VALID(ctx); + + return ctx->u.rinfo.zd_flags; +} + +void dplane_ctx_set_flags(struct zebra_dplane_ctx *ctx, uint32_t flags) +{ + DPLANE_CTX_VALID(ctx); + + ctx->u.rinfo.zd_flags = flags; +} + uint32_t dplane_ctx_get_metric(const struct zebra_dplane_ctx *ctx) { DPLANE_CTX_VALID(ctx); @@ -2655,6 +2671,7 @@ int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, ctx->zd_table_id = re->table; + ctx->u.rinfo.zd_flags = re->flags; ctx->u.rinfo.zd_metric = re->metric; ctx->u.rinfo.zd_old_metric = re->metric; ctx->zd_vrf_id = re->vrf_id; diff --git a/zebra/zebra_dplane.h b/zebra/zebra_dplane.h index 8b239a9ba19f..aa7ecde9656d 100644 --- a/zebra/zebra_dplane.h +++ b/zebra/zebra_dplane.h @@ -375,6 +375,9 @@ route_tag_t dplane_ctx_get_old_tag(const struct zebra_dplane_ctx *ctx); uint16_t dplane_ctx_get_instance(const struct zebra_dplane_ctx *ctx); void dplane_ctx_set_instance(struct zebra_dplane_ctx *ctx, uint16_t instance); uint16_t dplane_ctx_get_old_instance(const struct zebra_dplane_ctx *ctx); +uint32_t dplane_ctx_get_flags(const struct zebra_dplane_ctx *ctx); +void dplane_ctx_set_flags(struct zebra_dplane_ctx *ctx, + uint32_t flags); uint32_t dplane_ctx_get_metric(const struct zebra_dplane_ctx *ctx); uint32_t dplane_ctx_get_old_metric(const struct zebra_dplane_ctx *ctx); uint32_t dplane_ctx_get_mtu(const struct zebra_dplane_ctx *ctx); From 6bdf101ddaa7fa2994de363baee7ba00e9c8382c Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 3 Oct 2022 13:22:22 -0400 Subject: [PATCH 6/8] zebra: Rearrange dplane_ctx_route_init In order for a future commit to abstract the dplane_ctx_route_init so that the kernel can use it, let's move some stuff around and add a dplane_ctx_route_init_basic that can be used by multiple different paths Signed-off-by: Donald Sharp create a dplane_ctx_route_init_basic so it can be used Signed-off-by: Donald Sharp --- zebra/zebra_dplane.c | 62 ++++++++++++++++++++++++++++++-------------- zebra/zebra_dplane.h | 6 +++++ 2 files changed, 48 insertions(+), 20 deletions(-) diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index 3b8ac65a9f35..8c4d55bed5c9 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -2631,24 +2631,15 @@ static int dplane_ctx_ns_init(struct zebra_dplane_ctx *ctx, return AOK; } -/* - * Initialize a context block for a route update from zebra data structs. - */ -int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, - struct route_node *rn, struct route_entry *re) +int dplane_ctx_route_init_basic(struct zebra_dplane_ctx *ctx, + enum dplane_op_e op, struct route_entry *re, + const struct prefix *p, + const struct prefix *src_p, afi_t afi, + safi_t safi) { int ret = EINVAL; - const struct route_table *table = NULL; - const struct rib_table_info *info; - const struct prefix *p, *src_p; - struct zebra_ns *zns; - struct zebra_vrf *zvrf; - struct nexthop *nexthop; - struct zebra_l3vni *zl3vni; - const struct interface *ifp; - struct dplane_intf_extra *if_extra; - if (!ctx || !rn || !re) + if (!ctx || !re) return ret; TAILQ_INIT(&ctx->u.rinfo.intf_extra_q); @@ -2659,9 +2650,6 @@ int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, ctx->u.rinfo.zd_type = re->type; ctx->u.rinfo.zd_old_type = re->type; - /* Prefixes: dest, and optional source */ - srcdest_rnode_prefixes(rn, &p, &src_p); - prefix_copy(&(ctx->u.rinfo.zd_dest), p); if (src_p) @@ -2682,11 +2670,45 @@ int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, ctx->u.rinfo.zd_old_tag = re->tag; ctx->u.rinfo.zd_distance = re->distance; + ctx->u.rinfo.zd_afi = afi; + ctx->u.rinfo.zd_safi = safi; + + return AOK; +} + +/* + * Initialize a context block for a route update from zebra data structs. + */ +int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, + struct route_node *rn, struct route_entry *re) +{ + int ret = EINVAL; + const struct route_table *table = NULL; + const struct rib_table_info *info; + const struct prefix *p, *src_p; + struct zebra_ns *zns; + struct zebra_vrf *zvrf; + struct nexthop *nexthop; + struct zebra_l3vni *zl3vni; + const struct interface *ifp; + struct dplane_intf_extra *if_extra; + + if (!ctx || !rn || !re) + return ret; + + /* + * Let's grab the data from the route_node + * so that we can call a helper function + */ + + /* Prefixes: dest, and optional source */ + srcdest_rnode_prefixes(rn, &p, &src_p); table = srcdest_rnode_table(rn); info = table->info; - ctx->u.rinfo.zd_afi = info->afi; - ctx->u.rinfo.zd_safi = info->safi; + if (dplane_ctx_route_init_basic(ctx, op, re, p, src_p, info->afi, + info->safi) != AOK) + return ret; /* Copy nexthops; recursive info is included too */ copy_nexthops(&(ctx->u.rinfo.zd_ng.nexthop), diff --git a/zebra/zebra_dplane.h b/zebra/zebra_dplane.h index aa7ecde9656d..369add400870 100644 --- a/zebra/zebra_dplane.h +++ b/zebra/zebra_dplane.h @@ -866,6 +866,12 @@ dplane_pbr_ipset_entry_delete(struct zebra_pbr_ipset_entry *ipset); int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, struct route_node *rn, struct route_entry *re); +int dplane_ctx_route_init_basic(struct zebra_dplane_ctx *ctx, + enum dplane_op_e op, struct route_entry *re, + const struct prefix *p, + const struct prefix *src_p, afi_t afi, + safi_t safi); + /* Encode next hop information into data plane context. */ int dplane_ctx_nexthop_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, struct nhg_hash_entry *nhe); From 9628b9ed197eda359b5a31bf1ba2f46e382a3305 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 4 Oct 2022 15:41:36 -0400 Subject: [PATCH 7/8] zebra: Add ctx to netlink message parsing Add the initial step of passing in a dplane context to reading route netlink messages. This code will be run in two contexts: a) The normal pthread for reading netlink messages from the kernel b) The dplane_fpm_nl pthread. The goal of this commit is too just allow a) to work b) will be filled in in the future. Effectively everything should still be working as it should pre this change. We will just possibly allow the passing of the context around( but not used ) Signed-off-by: Donald Sharp --- zebra/rt_netlink.c | 28 ++++++++++++++++++++++------ zebra/rt_netlink.h | 5 +++++ zebra/zebra_dplane.c | 11 +++++++++++ zebra/zebra_dplane.h | 10 ++++++++++ 4 files changed, 48 insertions(+), 6 deletions(-) diff --git a/zebra/rt_netlink.c b/zebra/rt_netlink.c index e883033d59c6..0c0e90775c1b 100644 --- a/zebra/rt_netlink.c +++ b/zebra/rt_netlink.c @@ -692,8 +692,10 @@ static uint8_t parse_multipath_nexthops_unicast(ns_id_t ns_id, } /* Looking up routing table by netlink interface. */ -static int netlink_route_change_read_unicast(struct nlmsghdr *h, ns_id_t ns_id, - int startup) +int +netlink_route_change_read_unicast_internal(struct nlmsghdr *h, ns_id_t ns_id, + int startup, + struct zebra_dplane_ctx *ctx) { int len; struct rtmsg *rtm; @@ -770,7 +772,7 @@ static int netlink_route_change_read_unicast(struct nlmsghdr *h, ns_id_t ns_id, if (!startup && selfroute && h->nlmsg_type == RTM_NEWROUTE - && !zrouter.asic_offloaded) { + && !zrouter.asic_offloaded && !ctx) { if (IS_ZEBRA_DEBUG_KERNEL) zlog_debug("Route type: %d Received that we think we have originated, ignoring", rtm->rtm_protocol); @@ -985,8 +987,8 @@ static int netlink_route_change_read_unicast(struct nlmsghdr *h, ns_id_t ns_id, } } if (nhe_id || ng) - rib_add_multipath(afi, SAFI_UNICAST, &p, &src_p, re, ng, - startup); + dplane_rib_add_multipath(afi, SAFI_UNICAST, &p, &src_p, + re, ng, startup, ctx); else { /* * I really don't see how this is possible @@ -1001,6 +1003,13 @@ static int netlink_route_change_read_unicast(struct nlmsghdr *h, ns_id_t ns_id, XFREE(MTYPE_RE, re); } } else { + if (ctx) { + zlog_err( + "%s: %pFX RTM_DELROUTE received but received a context as well", + __func__, &p); + return 0; + } + if (nhe_id) { rib_delete(afi, SAFI_UNICAST, vrf_id, proto, 0, flags, &p, &src_p, NULL, nhe_id, table, metric, @@ -1025,7 +1034,14 @@ static int netlink_route_change_read_unicast(struct nlmsghdr *h, ns_id_t ns_id, } } - return 0; + return 1; +} + +static int netlink_route_change_read_unicast(struct nlmsghdr *h, ns_id_t ns_id, + int startup) +{ + return netlink_route_change_read_unicast_internal(h, ns_id, startup, + NULL); } static struct mcast_route_data *mroute = NULL; diff --git a/zebra/rt_netlink.h b/zebra/rt_netlink.h index b1af4b20e18c..b3a7fe95c1cc 100644 --- a/zebra/rt_netlink.h +++ b/zebra/rt_netlink.h @@ -122,6 +122,11 @@ netlink_put_lsp_update_msg(struct nl_batch *bth, struct zebra_dplane_ctx *ctx); extern enum netlink_msg_status netlink_put_pw_update_msg(struct nl_batch *bth, struct zebra_dplane_ctx *ctx); +int netlink_route_change_read_unicast_internal(struct nlmsghdr *h, + ns_id_t ns_id, + int startup, + struct zebra_dplane_ctx *ctx); + #ifdef NETLINK_DEBUG const char *nlmsg_type2str(uint16_t type); const char *af_type2str(int type); diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index 8c4d55bed5c9..f0d5b3410ab9 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -5947,6 +5947,17 @@ kernel_dplane_process_ipset_entry(struct zebra_dplane_provider *prov, dplane_provider_enqueue_out_ctx(prov, ctx); } +void dplane_rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p, + struct prefix_ipv6 *src_p, struct route_entry *re, + struct nexthop_group *ng, int startup, + struct zebra_dplane_ctx *ctx) +{ + if (!ctx) + rib_add_multipath(afi, safi, p, src_p, re, ng, startup); + else { + } +} + /* * Kernel provider callback */ diff --git a/zebra/zebra_dplane.h b/zebra/zebra_dplane.h index 369add400870..42fc4734ed35 100644 --- a/zebra/zebra_dplane.h +++ b/zebra/zebra_dplane.h @@ -1037,6 +1037,16 @@ void zebra_dplane_pre_finish(void); void zebra_dplane_finish(void); void zebra_dplane_shutdown(void); +/* + * decision point for sending a routing update through the old + * straight to zebra master pthread or through the dplane to + * the master pthread for handling + */ +void dplane_rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p, + struct prefix_ipv6 *src_p, struct route_entry *re, + struct nexthop_group *ng, int startup, + struct zebra_dplane_ctx *ctx); + #ifdef __cplusplus } #endif From e091a750181886f87f92f3b89125ba9310b3534d Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 7 Oct 2022 08:02:44 -0400 Subject: [PATCH 8/8] zebra: Read from the dplane_fpm_nl a route update Read from the fpm dplane a route update that will include status about whether or not the asic was successfull in offloading the route. Have this data passed up to zebra for processing and disseminate this data as appropriate. Signed-off-by: Donald Sharp --- doc/developer/fpm.rst | 16 +++++++++++ zebra/dplane_fpm_nl.c | 65 +++++++++++++++++++++++++++++++++++++++++-- zebra/zebra_dplane.c | 10 +++++-- zebra/zebra_dplane.h | 2 +- zebra/zebra_rib.c | 16 +++++++++-- 5 files changed, 101 insertions(+), 8 deletions(-) diff --git a/doc/developer/fpm.rst b/doc/developer/fpm.rst index 98498691330f..56d33671d2dd 100644 --- a/doc/developer/fpm.rst +++ b/doc/developer/fpm.rst @@ -101,3 +101,19 @@ Data ^^^^ The netlink or protobuf message payload. + + +Route Status Notification from ASIC +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The dplane_fpm_nl has the ability to read route netlink messages +from the underlying fpm implementation that can tell zebra +whether or not the route has been Offloaded/Failed or Trapped. +The end developer must send the data up the same socket that has +been created to listen for FPM messages from Zebra. The data sent +must have a Frame Header with Version set to 1, Message Type set to 1 +and an appropriate message Length. The message data must contain +a RTM_NEWROUTE netlink message that sends the prefix and nexthops +associated with the route. Finally rtm_flags must contain +RTM_F_OFFLOAD, RTM_F_TRAP and or RTM_F_OFFLOAD_FAILED to signify +what has happened to the route in the ASIC. diff --git a/zebra/dplane_fpm_nl.c b/zebra/dplane_fpm_nl.c index f5c85dc3770d..ead48cbf1cb9 100644 --- a/zebra/dplane_fpm_nl.c +++ b/zebra/dplane_fpm_nl.c @@ -51,6 +51,7 @@ #include "zebra/kernel_netlink.h" #include "zebra/rt_netlink.h" #include "zebra/debug.h" +#include "fpm/fpm.h" #define SOUTHBOUND_DEFAULT_ADDR INADDR_LOOPBACK #define SOUTHBOUND_DEFAULT_PORT 2620 @@ -461,7 +462,11 @@ static void fpm_reconnect(struct fpm_nl_ctx *fnc) static void fpm_read(struct thread *t) { struct fpm_nl_ctx *fnc = THREAD_ARG(t); + fpm_msg_hdr_t fpm; ssize_t rv; + char buf[4096]; + struct nlmsghdr *hdr; + struct zebra_dplane_ctx *ctx; /* Let's ignore the input at the moment. */ rv = stream_read_try(fnc->ibuf, fnc->socket, @@ -496,11 +501,67 @@ static void fpm_read(struct thread *t) return; } - stream_reset(fnc->ibuf); - /* Account all bytes read. */ atomic_fetch_add_explicit(&fnc->counters.bytes_read, rv, memory_order_relaxed); + + while (rv) { + if (rv < (ssize_t)FPM_MSG_HDR_LEN) { + stream_reset(fnc->ibuf); + zlog_warn( + "%s: Received %zd bytes but cannot even read fpm header, ignoring", + __func__, rv); + return; + } + + fpm.version = stream_getc(fnc->ibuf); + fpm.msg_type = stream_getc(fnc->ibuf); + fpm.msg_len = stream_getw(fnc->ibuf); + + if (fpm.version != FPM_PROTO_VERSION && + fpm.msg_type != FPM_MSG_TYPE_NETLINK) { + stream_reset(fnc->ibuf); + zlog_warn( + "%s: Received version/msg_type %u/%u, expected 1/1", + __func__, fpm.version, fpm.msg_type); + return; + } + + if (fpm.msg_len != rv) { + stream_reset(fnc->ibuf); + zlog_warn( + "%s: Received message length %u but read only got %zd, ignoring message", + __func__, fpm.msg_len, rv); + } + + /* + * Place the data from the stream into a buffer + */ + hdr = (struct nlmsghdr *)buf; + stream_get(buf, fnc->ibuf, fpm.msg_len - FPM_MSG_HDR_LEN); + rv -= fpm.msg_len; + + switch (hdr->nlmsg_type) { + case RTM_NEWROUTE: + ctx = dplane_ctx_alloc(); + dplane_ctx_set_op(ctx, DPLANE_OP_ROUTE_NOTIFY); + if (netlink_route_change_read_unicast_internal( + hdr, 0, false, ctx) != 1) { + dplane_ctx_fini(&ctx); + stream_reset(fnc->ibuf); + return; + } + break; + default: + if (IS_ZEBRA_DEBUG_FPM) + zlog_debug( + "%s: Received message type %u which is not currently handled", + __func__, hdr->nlmsg_type); + break; + } + } + + stream_reset(fnc->ibuf); } static void fpm_write(struct thread *t) diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index f0d5b3410ab9..54d27e882e13 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -2634,7 +2634,7 @@ static int dplane_ctx_ns_init(struct zebra_dplane_ctx *ctx, int dplane_ctx_route_init_basic(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, struct route_entry *re, const struct prefix *p, - const struct prefix *src_p, afi_t afi, + const struct prefix_ipv6 *src_p, afi_t afi, safi_t safi) { int ret = EINVAL; @@ -2685,7 +2685,8 @@ int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, int ret = EINVAL; const struct route_table *table = NULL; const struct rib_table_info *info; - const struct prefix *p, *src_p; + const struct prefix *p; + const struct prefix_ipv6 *src_p; struct zebra_ns *zns; struct zebra_vrf *zvrf; struct nexthop *nexthop; @@ -2702,7 +2703,7 @@ int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, */ /* Prefixes: dest, and optional source */ - srcdest_rnode_prefixes(rn, &p, &src_p); + srcdest_rnode_prefixes(rn, &p, (const struct prefix **)&src_p); table = srcdest_rnode_table(rn); info = table->info; @@ -5955,6 +5956,9 @@ void dplane_rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p, if (!ctx) rib_add_multipath(afi, safi, p, src_p, re, ng, startup); else { + dplane_ctx_route_init_basic(ctx, dplane_ctx_get_op(ctx), re, p, + src_p, afi, safi); + dplane_provider_enqueue_to_zebra(ctx); } } diff --git a/zebra/zebra_dplane.h b/zebra/zebra_dplane.h index 42fc4734ed35..43da9918fb78 100644 --- a/zebra/zebra_dplane.h +++ b/zebra/zebra_dplane.h @@ -869,7 +869,7 @@ int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, int dplane_ctx_route_init_basic(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, struct route_entry *re, const struct prefix *p, - const struct prefix *src_p, afi_t afi, + const struct prefix_ipv6 *src_p, afi_t afi, safi_t safi); /* Encode next hop information into data plane context. */ diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index a5c1e354f06f..dd7139c7a6f3 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -2251,8 +2251,7 @@ static void rib_process_dplane_notify(struct zebra_dplane_ctx *ctx) } /* Ensure we clear the QUEUED flag */ - if (!zrouter.asic_offloaded) - UNSET_FLAG(re->status, ROUTE_ENTRY_QUEUED); + UNSET_FLAG(re->status, ROUTE_ENTRY_QUEUED); /* Is this a notification that ... matters? We mostly care about * the route that is currently selected for installation; we may also @@ -2295,6 +2294,19 @@ static void rib_process_dplane_notify(struct zebra_dplane_ctx *ctx) dplane_ctx_get_type(ctx))); } goto done; + } else { + uint32_t flags = dplane_ctx_get_flags(ctx); + + if (CHECK_FLAG(flags, ZEBRA_FLAG_OFFLOADED)) { + UNSET_FLAG(re->flags, ZEBRA_FLAG_OFFLOAD_FAILED); + SET_FLAG(re->flags, ZEBRA_FLAG_OFFLOADED); + } + if (CHECK_FLAG(flags, ZEBRA_FLAG_OFFLOAD_FAILED)) { + UNSET_FLAG(re->flags, ZEBRA_FLAG_OFFLOADED); + SET_FLAG(re->flags, ZEBRA_FLAG_OFFLOAD_FAILED); + } + if (CHECK_FLAG(flags, ZEBRA_FLAG_TRAPPED)) + SET_FLAG(re->flags, ZEBRA_FLAG_TRAPPED); } /* We'll want to determine whether the installation status of the