Zebra: NHG event tracker — maximize NHG ID reuse during convergence#22383
Open
krishna-samy wants to merge 12 commits into
Open
Zebra: NHG event tracker — maximize NHG ID reuse during convergence#22383krishna-samy wants to merge 12 commits into
krishna-samy wants to merge 12 commits into
Conversation
krishna-samy
changed the title
krishna-samy
force-pushed
the
zebra-nhg-reuse
branch
3 times, most recently
from
8c6478d to
d6f2841
Compare
krishna-samy
force-pushed
the
zebra-nhg-reuse
branch
from
d6f2841 to
a356d83
Compare
krishna-samy
force-pushed
the
zebra-nhg-reuse
branch
8 times, most recently
from
4638343 to
0cbc97e
Compare
Greptile SummaryThis PR introduces a per-NHG event tracker to zebra that defers kernel nexthop-group installs across a configurable convergence window and maximises NHG ID reuse by reworking an installed NHG's content in-place rather than allocating a new ID. It is a large, architecturally significant change (~8 400 lines) touching the NHG resolution pipeline, the RIB processing loop, MPLS/RNH lookup, and interface state handling.
Confidence Score: 3/5The change reworks a core zebra subsystem with a multi-phase async flush state machine and in-place NHG content mutation; the unconditional INTF_DOWN tracker creation in zebra_nhg_check_valid without a validity-direction guard risks parking REs and suppressing kernel installs during interface UP, and development artifacts indicate the code was not fully cleaned up before submission. The two-phase flush with overlapping flushing/waiting tracker semantics, the intricate refcount bookkeeping in zebra_nhg_rebuild_depends, and the async tracker-ID bridge (tracker_parent_nhg_id) create ordering constraints that are difficult to verify statically and could surface under stress or unusual prefix/VRF configurations not covered by the new topotests. zebra/zebra_nhg.c (INTF_DOWN tracker in check_valid, TODO artifacts, nexthop_active_update winner logic), zebra/zebra_nhg_tracker.c (two-phase flush, tracker collapse, consolidation), zebra/zebra_rib.c (rib_link parking, rib_delnode bool flag) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Proto as Protocol (BGP/OSPF)
participant RIB as zebra_rib (rib_link)
participant Tracker as NHG Tracker
participant NHG as NHG Hash
participant Dplane as Dataplane
Note over Proto,Dplane: Interface DOWN event
Proto->>RIB: Route update (re)
RIB->>Tracker: zebra_nhg_tracker_create_for_event(INTF_DOWN)
Tracker->>Tracker: Snapshot parent NHG content
RIB->>Tracker: zebra_nhg_tracker_park_re(re)
Tracker->>Tracker: Park RE in matched/unmatched/deleted table
Tracker->>Tracker: Check flush_if_full(orig_re_count)
alt Batch saturated OR timer expires
Tracker->>NHG: zebra_nhg_rework_content_release(parent_nhe)
Note over Tracker: Phase 1: drain losers
Tracker->>RIB: rib_queue_add(loser RNs)
RIB->>Dplane: RTM_NEWROUTE (losers)
Dplane-->>RIB: dplane ack
RIB->>Tracker: tracker_flush_batch_route_dplane_ack(re)
Note over Tracker: Phase 2: release winners
Tracker->>RIB: rib_queue_add(winner RNs) [TRACKER_WINNER]
RIB->>NHG: "nexthop_active_update -> zebra_nhg_rework_in_place"
NHG->>NHG: Replace content, rebuild nhg_depends, rehash
NHG->>Dplane: RTM_NEWNEXTHOP (same ID, new content)
RIB->>Dplane: RTM_NEWROUTE (winners, same NHG ID)
Tracker->>Tracker: "tracker_flush_complete -> schedule consolidation"
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Proto as Protocol (BGP/OSPF)
participant RIB as zebra_rib (rib_link)
participant Tracker as NHG Tracker
participant NHG as NHG Hash
participant Dplane as Dataplane
Note over Proto,Dplane: Interface DOWN event
Proto->>RIB: Route update (re)
RIB->>Tracker: zebra_nhg_tracker_create_for_event(INTF_DOWN)
Tracker->>Tracker: Snapshot parent NHG content
RIB->>Tracker: zebra_nhg_tracker_park_re(re)
Tracker->>Tracker: Park RE in matched/unmatched/deleted table
Tracker->>Tracker: Check flush_if_full(orig_re_count)
alt Batch saturated OR timer expires
Tracker->>NHG: zebra_nhg_rework_content_release(parent_nhe)
Note over Tracker: Phase 1: drain losers
Tracker->>RIB: rib_queue_add(loser RNs)
RIB->>Dplane: RTM_NEWROUTE (losers)
Dplane-->>RIB: dplane ack
RIB->>Tracker: tracker_flush_batch_route_dplane_ack(re)
Note over Tracker: Phase 2: release winners
Tracker->>RIB: rib_queue_add(winner RNs) [TRACKER_WINNER]
RIB->>NHG: "nexthop_active_update -> zebra_nhg_rework_in_place"
NHG->>NHG: Replace content, rebuild nhg_depends, rehash
NHG->>Dplane: RTM_NEWNEXTHOP (same ID, new content)
RIB->>Dplane: RTM_NEWROUTE (winners, same NHG ID)
Tracker->>Tracker: "tracker_flush_complete -> schedule consolidation"
end
Prompt To Fix All With AIFix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
zebra/zebra_nhg.c:1179-1186
**Unconditional INTF_DOWN tracker creation fires during interface-UP path**
`zebra_nhg_check_valid` is called by the NHG validity cascade in both directions (valid→invalid and invalid→valid), but the new tracker-creation code always uses `NHG_TRACKER_EVENT_INTF_DOWN` regardless. If this code path is reached while the singleton is transitioning to valid (interface UP — e.g., via `nexthop_active_update` during the UP cascade or a direct call from reinit paths), `zebra_nhg_tracker_create_for_event` will walk all dependent NHGs and create INTF_DOWN trackers on them. Those trackers immediately start parking incoming REs and suppressing kernel installs, which is the exact opposite of what is needed when an interface comes up. A `if (!valid)` guard or an explicit event-direction parameter should be added before the `zebra_nhg_tracker_create_for_event` call.
### Issue 2 of 5
zebra/zebra_nhg.c:3802-3808
**Development TODO artifacts left in production code**
Two `/* todo: ... */` comments (one marked "remove this log", one marked "remove this check") and a `#ifdef NHG_TRK_VERBOSE_LOG` block are left in the production path. The `NHG_TRK_VERBOSE_LOG` guard is never defined in the build system, making the log permanently dead code; the TODO comments indicate both sites were intentionally left unfinished before submission.
```suggestion
if (!route_map_ok) {
/*
* Route-map denied some NHs for this RE only;
* leave REUSE set so another winner (without
* the route-map issue) can still trigger the rework.
*/
```
### Issue 3 of 5
zebra/zebra_nhg.c:3816-3829
The `#ifdef NHG_TRK_VERBOSE_LOG` guard is not defined anywhere in the build system and renders this `zlog_info` permanently dead; the accompanying TODO comment confirms this path was intended to be removed before merging.
```suggestion
} else if (shape_differs) {
/*
* Kernel rejects in-place RTM_NEWNEXTHOP across
* singleton<->group changes. Clear REUSE+
* REINSTALL so subsequent winners don't id-lock
* onto a not-yet-reworked parent_nhe.
*/
```
### Issue 4 of 5
zebra/rib.h:203
**`route_entry_cmp` defined `static` (not `static inline`) in a header**
Every translation unit that includes `rib.h` will receive its own non-inline copy of `route_entry_cmp`. Because the function is `static`, each copy is independent, resulting in unnecessary code duplication across the ~20+ files that include `rib.h`. GCC/clang may also emit `-Wunused-function` warnings for TUs that pull in the header but never invoke the RB-tree helpers. Marking it `static inline` matches the pattern used by the `DECLARE_RBTREE_UNIQ` macro's own generated helpers and eliminates the redundant copies.
```suggestion
static inline int route_entry_cmp(const struct route_entry *re1, const struct route_entry *re2)
```
### Issue 5 of 5
zebra/zebra_rib.c:4377
**`rib_delnode`'s new `bool flag` parameter has unclear semantics**
The parameter is named `flag`, which gives no hint of its meaning. Callers pass `false` for route-replacement (tracker parking skipped) and `true` for explicit user deletes (parking desired). A reader encountering `rib_delnode(rn, same, true)` or `rib_delnode(rn, re, false)` has no idea what `true`/`false` controls without reading the body. A descriptive name reduces the risk of future misuse at all four call sites.
```suggestion
void rib_delnode(struct route_node *rn, struct route_entry *re, bool allow_tracker_park)
```
Reviews (1): Last reviewed commit: "zebra: clear route INSTALLED state on NH..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
1179
to
+1186
| } | ||
|
|
||
| zebra_nhg_set_valid(nhe, valid); | ||
|
|
||
| /* | ||
| * Create tracker now - all the parent NHGs are updated with the valid state. | ||
| * Walk transitive dependents - catches recursively-dependent NHGs and | ||
| * creates trackers wherever required. |
There was a problem hiding this comment.
zebra_nhg_check_valid is called by the NHG validity cascade in both directions (valid→invalid and invalid→valid), but the new tracker-creation code always uses NHG_TRACKER_EVENT_INTF_DOWN regardless. If this code path is reached while the singleton is transitioning to valid (interface UP — e.g., via nexthop_active_update during the UP cascade or a direct call from reinit paths), zebra_nhg_tracker_create_for_event will walk all dependent NHGs and create INTF_DOWN trackers on them. Those trackers immediately start parking incoming REs and suppressing kernel installs, which is the exact opposite of what is needed when an interface comes up. A if (!valid) guard or an explicit event-direction parameter should be added before the zebra_nhg_tracker_create_for_event call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: zebra/zebra_nhg.c
Line: 1179-1186
Comment:
**Unconditional INTF_DOWN tracker creation fires during interface-UP path**
`zebra_nhg_check_valid` is called by the NHG validity cascade in both directions (valid→invalid and invalid→valid), but the new tracker-creation code always uses `NHG_TRACKER_EVENT_INTF_DOWN` regardless. If this code path is reached while the singleton is transitioning to valid (interface UP — e.g., via `nexthop_active_update` during the UP cascade or a direct call from reinit paths), `zebra_nhg_tracker_create_for_event` will walk all dependent NHGs and create INTF_DOWN trackers on them. Those trackers immediately start parking incoming REs and suppressing kernel installs, which is the exact opposite of what is needed when an interface comes up. A `if (!valid)` guard or an explicit event-direction parameter should be added before the `zebra_nhg_tracker_create_for_event` call.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+3802
to
+3808
| if (!route_map_ok) { | ||
| /* | ||
| * Route-map denied some NHs for this RE only; | ||
| * leave REUSE set so another winner (without | ||
| * the route-map issue) can still trigger the rework | ||
| * todo: logged for testing. remove this log. | ||
| */ |
There was a problem hiding this comment.
Two /* todo: ... */ comments (one marked "remove this log", one marked "remove this check") and a #ifdef NHG_TRK_VERBOSE_LOG block are left in the production path. The NHG_TRK_VERBOSE_LOG guard is never defined in the build system, making the log permanently dead code; the TODO comments indicate both sites were intentionally left unfinished before submission.
Suggested change
| if (!route_map_ok) { | |
| /* | |
| * Route-map denied some NHs for this RE only; | |
| * leave REUSE set so another winner (without | |
| * the route-map issue) can still trigger the rework | |
| * todo: logged for testing. remove this log. | |
| */ | |
| if (!route_map_ok) { | |
| /* | |
| * Route-map denied some NHs for this RE only; | |
| * leave REUSE set so another winner (without | |
| * the route-map issue) can still trigger the rework. | |
| */ |
Prompt To Fix With AI
This is a comment left during a code review.
Path: zebra/zebra_nhg.c
Line: 3802-3808
Comment:
**Development TODO artifacts left in production code**
Two `/* todo: ... */` comments (one marked "remove this log", one marked "remove this check") and a `#ifdef NHG_TRK_VERBOSE_LOG` block are left in the production path. The `NHG_TRK_VERBOSE_LOG` guard is never defined in the build system, making the log permanently dead code; the TODO comments indicate both sites were intentionally left unfinished before submission.
```suggestion
if (!route_map_ok) {
/*
* Route-map denied some NHs for this RE only;
* leave REUSE set so another winner (without
* the route-map issue) can still trigger the rework.
*/
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+3816
to
+3829
| } else if (shape_differs) { | ||
| /* | ||
| * Kernel rejects in-place RTM_NEWNEXTHOP across | ||
| * singleton<->group changes. Clear REUSE+ | ||
| * REINSTALL so subsequent winners don't id-lock | ||
| * onto a not-yet-reworked parent_nhe. | ||
| * todo: remove this check once zebra handles this limitation. | ||
| */ | ||
| #ifdef NHG_TRK_VERBOSE_LOG | ||
| zlog_info("%s: NHG reuse skipped for old NHG %u: shape mismatch (parent_singleton=%d curr_singleton=%d)", | ||
| __func__, parent_nhe->id, | ||
| ZEBRA_NHG_IS_SINGLETON(parent_nhe) ? 1 : 0, | ||
| ZEBRA_NHG_IS_SINGLETON(curr_nhe) ? 1 : 0); | ||
| #endif |
There was a problem hiding this comment.
#ifdef NHG_TRK_VERBOSE_LOG guard is not defined anywhere in the build system and renders this zlog_info permanently dead; the accompanying TODO comment confirms this path was intended to be removed before merging.
Suggested change
| } else if (shape_differs) { | |
| /* | |
| * Kernel rejects in-place RTM_NEWNEXTHOP across | |
| * singleton<->group changes. Clear REUSE+ | |
| * REINSTALL so subsequent winners don't id-lock | |
| * onto a not-yet-reworked parent_nhe. | |
| * todo: remove this check once zebra handles this limitation. | |
| */ | |
| #ifdef NHG_TRK_VERBOSE_LOG | |
| zlog_info("%s: NHG reuse skipped for old NHG %u: shape mismatch (parent_singleton=%d curr_singleton=%d)", | |
| __func__, parent_nhe->id, | |
| ZEBRA_NHG_IS_SINGLETON(parent_nhe) ? 1 : 0, | |
| ZEBRA_NHG_IS_SINGLETON(curr_nhe) ? 1 : 0); | |
| #endif | |
| } else if (shape_differs) { | |
| /* | |
| * Kernel rejects in-place RTM_NEWNEXTHOP across | |
| * singleton<->group changes. Clear REUSE+ | |
| * REINSTALL so subsequent winners don't id-lock | |
| * onto a not-yet-reworked parent_nhe. | |
| */ |
Prompt To Fix With AI
This is a comment left during a code review.
Path: zebra/zebra_nhg.c
Line: 3816-3829
Comment:
The `#ifdef NHG_TRK_VERBOSE_LOG` guard is not defined anywhere in the build system and renders this `zlog_info` permanently dead; the accompanying TODO comment confirms this path was intended to be removed before merging.
```suggestion
} else if (shape_differs) {
/*
* Kernel rejects in-place RTM_NEWNEXTHOP across
* singleton<->group changes. Clear REUSE+
* REINSTALL so subsequent winners don't id-lock
* onto a not-yet-reworked parent_nhe.
*/
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| struct nhe_re_tree_item re_item; | ||
| }; | ||
|
|
||
| static int route_entry_cmp(const struct route_entry *re1, const struct route_entry *re2) |
There was a problem hiding this comment.
route_entry_cmp defined static (not static inline) in a header
Every translation unit that includes rib.h will receive its own non-inline copy of route_entry_cmp. Because the function is static, each copy is independent, resulting in unnecessary code duplication across the ~20+ files that include rib.h. GCC/clang may also emit -Wunused-function warnings for TUs that pull in the header but never invoke the RB-tree helpers. Marking it static inline matches the pattern used by the DECLARE_RBTREE_UNIQ macro's own generated helpers and eliminates the redundant copies.
Suggested change
| static int route_entry_cmp(const struct route_entry *re1, const struct route_entry *re2) | |
| static inline int route_entry_cmp(const struct route_entry *re1, const struct route_entry *re2) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: zebra/rib.h
Line: 203
Comment:
**`route_entry_cmp` defined `static` (not `static inline`) in a header**
Every translation unit that includes `rib.h` will receive its own non-inline copy of `route_entry_cmp`. Because the function is `static`, each copy is independent, resulting in unnecessary code duplication across the ~20+ files that include `rib.h`. GCC/clang may also emit `-Wunused-function` warnings for TUs that pull in the header but never invoke the RB-tree helpers. Marking it `static inline` matches the pattern used by the `DECLARE_RBTREE_UNIQ` macro's own generated helpers and eliminates the redundant copies.
```suggestion
static inline int route_entry_cmp(const struct route_entry *re1, const struct route_entry *re2)
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| } | ||
|
|
||
| void rib_delnode(struct route_node *rn, struct route_entry *re) | ||
| void rib_delnode(struct route_node *rn, struct route_entry *re, bool flag) |
There was a problem hiding this comment.
rib_delnode's new bool flag parameter has unclear semantics
The parameter is named flag, which gives no hint of its meaning. Callers pass false for route-replacement (tracker parking skipped) and true for explicit user deletes (parking desired). A reader encountering rib_delnode(rn, same, true) or rib_delnode(rn, re, false) has no idea what true/false controls without reading the body. A descriptive name reduces the risk of future misuse at all four call sites.
Suggested change
| void rib_delnode(struct route_node *rn, struct route_entry *re, bool flag) | |
| void rib_delnode(struct route_node *rn, struct route_entry *re, bool allow_tracker_park) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: zebra/zebra_rib.c
Line: 4377
Comment:
**`rib_delnode`'s new `bool flag` parameter has unclear semantics**
The parameter is named `flag`, which gives no hint of its meaning. Callers pass `false` for route-replacement (tracker parking skipped) and `true` for explicit user deletes (parking desired). A reader encountering `rib_delnode(rn, same, true)` or `rib_delnode(rn, re, false)` has no idea what `true`/`false` controls without reading the body. A descriptive name reduces the risk of future misuse at all four call sites.
```suggestion
void rib_delnode(struct route_node *rn, struct route_entry *re, bool allow_tracker_park)
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
krishna-samy
force-pushed
the
zebra-nhg-reuse
branch
from
0cbc97e to
0348fda
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Ability to have a back ptr from re to rn. To be used by future commits Signed-off-by: Rajasekar Raja <rajasekarr@nvidia.com>
Introduce a per-NHE RB tree (nhe->re_head) that tracks every route_entry currently referencing the NHE. Populated and depopulated from route_entry_update_nhe() so the tree stays in lockstep with the NHE refcount. The tree is keyed by (vrf_id, type, instance, re pointer) via a new re_item embedded in struct route_entry, and is wired through a small helper nhe_add_or_del_re_tree(). This commit is pure infrastructure with no consumers; later work will use this back-pointer to enumerate the routes attached to a given NHE. Signed-off-by: Rajasekar Raja <rajasekarr@nvidia.com>
Adding a new entry to the existing show command "show nexthop-group rib <ID> routes" which will walk all the re's which uses that particular NHG and displays the route node, protocol and the installed status. Examples: r1# sh nexthop-group rib 40 routes Routes using nexthop group 40: Route Entry Status Protocol AFI SAFI ------------------------------ --------------- ---------- ------- ------- 4.5.6.10/32 installed static IPv4 unicast 4.5.6.11/32 installed static IPv4 unicast 4.5.6.15/32 not installed static IPv4 unicast 4.5.6.16/32 installed static IPv4 unicast 4.5.6.17/32 installed static IPv4 unicast Signed-off-by: Rajasekar Raja <rajasekarr@nvidia.com>
Introduces the data structures used by the NHG event tracker, a mechanism that defers NHG kernel reinstallation across interface and ECMP-change events so route entries (REs) can converge on the same nexthops without churn. All are temp caches that will be cleaned-up once the event is complete. - struct nhg_event_tracker, nhg_tracker_table, tracker_vrf_table: the tracker object and its per-VRF route_table storage that holds matched / unmatched / deleted prefixes during the convergence window. - struct tracker_prefix_map_entry: per-NHE hash entry mapping (prefix, type, instance, vrf_id) to the owning tracker so rib_link can route an incoming RE to the right tracker in O(1). - struct tracker_flush_nhg_group: per-incoming-NHG counter built during table flush to pick the winner group for the old NHG ownership. - NEXTHOP_GROUP_TRACKER_REUSE / NEXTHOP_GROUP_DUPLICATE: tell nexthop_active_update to rework an installed NHG in place, and mark reworked NHGs whose content collides with an existing NHG(dup) so the consolidation timer can merge them. - RIB status flags (rib.h): ROUTE_ENTRY_TRACKER marks an RE parked in a tracker; ROUTE_ENTRY_NHG_TRACKER_FLUSH_BATCH / _WINNER to mark phase-1 drain REs and phase-2 winner REs respectively. - nhg_hash_entry fields (zebra_nhg.h): tracker_list and tracker_prefix_map keep the active trackers and their prefix lookup; tracker_flush_batch_parent_nhg_id and tracker_pending_winners to process the dplane-ack path and phase-2 RE flush Also adds NEXTHOP_FLAG_PRETEND_ACTIVE in lib/nexthop.h (treats a nexthop as active during tracker NH comparison) for NHG comparision Signed-off-by: Krishnasamy R <krishnasamyr@nvidia.com>
Introduce zebra_nhg_tracker.c with the basic building blocks of the tracker module.
Top functionalities provided here:
1. Route-entry parking — zebra_nhg_tracker_park_re routes an incoming
RE into the matched / unmatched / deleted table of an existing
tracker based on how the RE's nexthop set matches the tracker
snapshot.
2. Route-entry eviction — zebra_nhg_tracker_evict_re and the
_evict_from_{matched,unmatched,deleted} helpers remove a previously
parked RE so tracker bookkeeping (prefix_map, re_count) stays
consistent across re-parking and tracker collapse.
3. NHG comparison — nhg_compare_nexthops and zebra_nhg_tracker_nhgs_equal
provide a resolution-tolerant compare between a tracker snapshot
and an incoming RE's NHG to drive the matched / unmatched decision.
Also some other functionalities like prefix-map, Per-VRF route table for
trackers, etc.
Signed-off-by: Eyal Nissim <enissim@nvidia.com>
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Signed-off-by: Krishnasamy R <krishnasamyr@nvidia.com>
Build on the basic tracker infrastructure with the advanced lifecycle and flush engine. some of the key functionalities include: Tracker table flush is done in 2 phases Tracker lifecycle (zebra/zebra_nhg_tracker.c): - Tracker lifetime: zebra_nhg_tracker_create / _free / _fini - Per-NHE sweep at shutdown: _sweep_entry / _sweep_all - Timer handling: nhg_tracker_timer_expiry - Collapse helper: zebra_nhg_tracker_collapse (calls _free) - Unique-RE counter: tracker_count_unique_res Flush + NHG rework engine (in same file): - Top-level flush: zebra_nhg_tracker_flush, _flush_if_full - Two-phase batch flush: tracker_flush_batch_start_phase1 / _phase2, tracker_flush_batch_process_table, _batch_finish - Loser tracking: tracker_track_loser_nhe, _clear_loser_parent_ids - Winner handling: tracker_winner_pre_remove, tracker_flush_batch_route_dplane_ack - Silent-RE handling: tracker_flush_enqueue_silent_res, _fire_silent_routes_phase2, tracker_count_silent_res - NHG group construction: tracker_flush_build_groups, _nhg_group_free, _free_groups Integration in existing zebra code: - zebra_nhg.c: tracker init/free hooks, defer kernel install when tracker active, NHG rework helpers (rework_in_place, rework_content_*, mark_duplicate/reuse, schedule_consolidate, dup-detection), consolidation event handler Signed-off-by: Krishnasamy R <krishnasamyr@nvidia.com> Signed-off-by: Eyal Nissim <enissim@nvidia.com>
Wire the NHG event tracker module into the RIB code paths: - rib_link: park incoming RE into the active tracker when one exists on the originating NHG (matched/unmatched/deleted table selection driven by snapshot match + REMOVED flag) - rib_unlink / rib_delnode: tracker-aware unlink so deleted REs land in the tracker's deleted_table instead of disappearing immediately - rib_process: skip best-path / install for REs currently parked in a tracker, and re-issue rib_queue_add when all the REs are converged. - rib_process_result: invoke 'dplane ack handler' when all the phase-1 REs are installed in dplane and ready to start phase-2/winner flush. - process_subq_route / early_route_add / early_route_delete: honour ROUTE_ENTRY_NHG_TRACKER_FLUSH_BATCH and WINNER flags so batch-flush REs aren't double-processed - tracker_winner_pre_remove call sites: cleanly drop winner status when the RE is unlinked / replaced before the flush completes Signed-off-by: Krishnasamy R <krishnasamyr@nvidia.com>
Add 'show' command coverage for the NHG event tracker. Add 'config' command for the tracker safety timer. - Extends 'show nexthop-group rib <id>' with a Trackers section listing each tracker (event, ifindex, matched/unmatched/deleted counts, snapshot NHG nexthops) + JSON. - New 'show zebra tracker' command surfacing the global zebra_nhg_tracker_counters (allocated/freed, collapsed, loop_detected, timer_expired, full/full_matched/full_unmatched /full_combined and the recent flush_event log ring) + JSON. Signed-off-by: Krishnasamy R <krishnasamyr@nvidia.com>
current sharpd code can not encode multiple nexthops in a single zapi_route message to zenra. adding that support. example: sharp install routes 45.1.1.4 nexthops 10.1.1.1,10.1.2.1,10.1.3.1 2 Signed-off-by: Krishnasamy R <krishnasamyr@nvidia.com>
Along with new tracker tests, we are also updating some existing topotests to take care of tracker behaviour. Signed-off-by: Krishnasamy R <krishnasamyr@nvidia.com>
Replace the synchronous per-table walks inside tracker_flush_batch_
start_phase{1,2} with a pause-capable iter driven by an event-loop
callback (nhg_tracker_flush_iter_event).
Signed-off-by: Krishnasamy R <krishnasamyr@nvidia.com>
When an NHG is reprogrammed to the kernel after events like quick interface flaps or kernel nexthop changes (ip nexthop del / flush), zebra clears NEXTHOP_GROUP_INSTALLED on the NHG but leaves ROUTE_ENTRY_INSTALLED set on the routes using it. rib_process then treats those routes as already installed and skips reprogramming them, leaving stale FIB state that no longer matches the freshly reinstalled NHG. Signed-off-by: Krishnasamy R <krishnasamyr@nvidia.com>
krishna-samy
force-pushed
the
zebra-nhg-reuse
branch
from
0348fda to
4363bbb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Zebra currently installs nexthop groups (NHGs) into the kernel as soon as any of their singleton dependencies changes state (e.g., a link up). The problems are:
Premature route/NHG install before the protocol converges. When a link flaps, zebra rewrites the parent NHG with the immediately-available subset of active singletons even though the upper-level protocol hasn't finished re-converging its own NH set yet. This premature NHG installation causes traffic drops.
NHG fragmentation / poor reuse. When the protocol re-advertises the same prefixes during convergence with even a slightly different NH set, zebra allocates a new NHG ID and migrates all the dependent routes onto it. Routes get re-installed against the new ID even when the content could have been carried in-place by the existing NHG. Reusing the NHG would help this churn in the dplane.
The goal of this solution is to:
Fix approach
The design introduces a per-NHG tracker object that snapshots a parent NHG at the moment of a convergence trigger, parks every route entry (RE) that arrives for that NHG during the convergence window, and then flushes the batch with either an in-place rework of the parent (the common case) or a clean migration onto a freshly resolved NHG (whenever required). Once all routes are processed, the tracker object is deleted and tracker is a temporary cache.
Implementation details:
1. Tracker creation
A tracker (
struct nhg_event_tracker) is a transient object attached to a parent NHG'stracker_list. It owns:matched_table— RE's incoming NH set matches the snapshot.unmatched_table— RE's incoming NH set differs from the snapshot.deleted_table— RE withdrawn during the tracker window.orig_re_count,routes_pending,tracker_pending_winners.The tracker enables NHG-ID reuse by (i) blocking the immediate kernel install of the parent during the convergence window via
NEXTHOP_GROUP_REINSTALL, (ii) collecting the protocol's full update stream into one batch, and (iii) driving a single rework + install at the end. The parent NHG ID never changes; only its content is mutated in place.Trackers are created from exactly three sites:
Please note that the tracker object will be created only for the installed Routes, the new routes will go through the regular flow, because new RE does not have any existing/installed NHG to track.
rib_link— when an incoming RE's NH set differs from the currently-installed NHG, a tracker is created on the installed (parent) NHG before the new RE is parked.Required conditions: must have at least one RE using it.
2. Parking REs and tracker active window
Once a tracker exists on a parent NHG, every RE arriving via
rib_linkthat would otherwise update the parent is parked in the tracker instead:ROUTE_ENTRY_TRACKER(+ROUTE_ENTRY_CHANGEDfor updates).matched_table,unmatched_table, ordeleted_tablebased on how its incoming NH set compares to the snapshot.NEXTHOP_GROUP_REINSTALLis set, causing any indirect install paths (interface UP, singleton dep install ack) to defer the kernel install for as long as the tracker exists.The tracker stays active until either of:
matched + unmatched + deleted == expected_re_count. Every RE that originally referenced the parent has accounted for itself, so tracker is processed once the count is met.Whichever happens first triggers the flush.
3. Tracker flushing
The flush runs in two phases over the tracker's tables.
Winner / loser selection at flush time
When the tracker becomes full, the parked REs are grouped by their incoming NH set: the matched table forms one group, the unmatched table forms one group per distinct incoming NHG ID, and the silent REs (those that never re-advertised) form another. The largest group wins (with deterministic tie-breakers); every other group is a loser. Phase 1 drains the losers; Phase 2 releases the winner group and lets it drive the parent-NHG rework as mentioned in below sections.
It is possible to have duplicate NHGs after tracker flush, So, duplicate-NHG consolidation scheduled during the flush runs once all winners have attached.
4. NHG rework and duplicate consolidation
This is where the NHG-ID reuse actually happens, inside
nexthop_active_updatewhen a winner RE is being resolved.At tracker arming, the parent NHG is marked with two flags:
NEXTHOP_GROUP_TRACKER_REUSE— "the first qualified winner should drive an in-place rework of my content."NEXTHOP_GROUP_REINSTALL— "after the rework, push me to the kernel as a single install op."This is done in 'nexthop_active_update'
Net effect: the parent NHG ID is preserved across the convergence event, every dependent route stays bound to the same kernel NHG ID, and the kernel sees exactly one
RTM_NEWNEXTHOPfor the parent (the rework) instead of one per intermediate state.5. Cooperative-yield table flush via pausable iterator
The tracker table flush is implemented as a resumable iterator + a time-sliced event to avoid starvations.
Tests
New
zebra_trackertopotest module undertests/topotests/zebra_tracker/, covering:route_map_ok == falsebranch.KEEP_AROUNDdelete-timer semantics.Existing
zebra_nhg_checkandall_protocol_startuptopotests are also extended to wait for tracker convergence and to validate the new behaviour.