Neighbor Next-hop optimization HLD - #2547
manamand2020 wants to merge 2 commits into
Conversation
Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
|
/azp run |
|
No pipelines are associated with this pull request. |
There was a problem hiding this comment.
🟡 Changes recommended
The mux creation path is internally inconsistent, and the debounce behavior and target release need clarification.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an HLD for conserving ASIC next-hop resources through delayed, on-demand neighbor next-hop creation.
Changes:
- Defines feature gating and next-hop lifecycle APIs.
- Covers mux, FG ECMP, BFD, CRM, warm restart, and failure handling.
- Documents operational trade-offs and testing plans.
File summaries
| File | Description |
|---|---|
doc/ip/delayed-neighbor-nexthop-creation-hld.md |
Specifies the delayed neighbor next-hop design. |
Review details
- Files reviewed: 1/3 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| <a id="72-muxorch"></a> | ||
| ### 7.2 MuxOrch | ||
| Mux neighbors in the default ("host-route") mode are plain (non `prefix_route`) neighbors and are therefore subject to delayed creation. Several mux code paths (`MuxNbrHandler::update()`/`enable()`, `MuxOrch::updateRoute()`) previously fetched a next hop's SAI id via the **pure** `getLocalNextHopId()` lookup, which would silently return `SAI_NULL_OBJECT_ID` for a deferred-but-not-yet-created next hop. Each of these call sites now calls `ensureNextHop()` first to force on-demand creation before the pure lookup. (`MuxPrefixBasedNbrHandler`, used for no-host-route mode, is unaffected -- it is always `prefix_route`, hence always eager.) |
There was a problem hiding this comment.
Agreed. ensureNextHop() is the wrong here: its existence check also consults
MuxOrch::getNextHopId(), so with a tunnel override present it returns success having created
nothing, and the following getLocalNextHopId() still returns SAI_NULL_OBJECT_ID.
MuxNbrHandler::enable() hits this on every standby->active transition, since entries in
neighbors_ still hold the tunnel id from the standby path. The MUX_STATE_ACTIVE case in
update() is safe only incidentally.
Section 7.2 now adds ensureLocalNextHop() -- identical to ensureNextHop() but checking
m_syncdNextHops alone. The mux sites that follow with getLocalNextHopId() use it; every other
consumer keeps ensureNextHop().
| - A `SET` (route install) after a `DEL` (route withdraw) for the same prefix, with the neighbor still resolved, calls `ensureNextHop()` again, finds `hasSyncdNextHop() == true`, and returns immediately -- no new `create_next_hop()`/`remove_next_hop()` SAI call is made, only `increaseNextHopRefCount()`/`decreaseNextHopRefCount()` bookkeeping. | ||
| - This means the one-time creation cost introduced by this feature is paid **once, at first reference**, not once per churn cycle. Steady-state route flapping is no more expensive after this change than before it; the only difference is *when* that one-time cost is paid (first route reference instead of neighbor resolution). | ||
| - This is a genuine change in behavior only for plain ARP/ND next hops. MPLS-labeled next hops (`RouteOrch::removeMplsNextHop()`, invoked whenever `getNextHopRefCount() == 0`) and overlay/VXLAN tunnel next hops already had a "create-on-reference, destroy-on-zero-refcount" lifecycle *before* this change, so they already pay a create/destroy cost on every churn cycle, unrelated to and unaffected by this feature. | ||
| - As a side effect, CRM `NEXTHOP` used-counters for plain neighbors stay flat across pure route churn instead of ticking down/up on every flap, which is a useful property when using those counters as a capacity signal (see [Section 7.6.1](#761-adjacency-resolution-no-longer-reserves-a-next-hop)) -- churn noise does not show up as CRM churn for the common case. |
There was a problem hiding this comment.
Correct, this conflicted with the debounced reclaim in section 7.6.3. The guarantee is now scoped to
re-references that land within the grace period. Past it the reclaim fires: the next hop is
destroyed once, CRM used drops, and a later SET pays the creation cost again. The
"CRM counters stay flat" bullet is qualified the same way.
| ## 1. Revision | ||
| | Rev | Date | Author | Change Description | | ||
| |:---:|:------------:|:----------------------:|:-------------------------:| | ||
| | 0.1 | 07/27/2026 | Manas Kumar Mandal | Initial draft. | |
| To bound that staleness without giving up the churn-free property above, reclaiming a next hop on reference count reaching zero is **debounced** rather than immediate or never: when a next hop's reference count drops to zero, its destruction is scheduled after a short, fixed grace period instead of happening right away. If any consumer references it again before the grace period elapses -- the ordinary churn case -- the pending destruction is simply cancelled and no SAI work happens at all, identical to today. Only a next hop that stays unreferenced for longer than the grace period is actually torn down, once, through the same teardown path `removeNeighbor()` already uses, so CRM decrement and `FgNhgOrch` invalidation ([Section 7.1](#71-fgnhgorch-fine-grained-ecmp)) stay consistent automatically, with no separate teardown logic to maintain. | ||
|
|
||
| This is deliberately narrow in scope: it only bounds staleness under ordinary churn, and does not attempt to reactively reclaim capacity under table-full pressure (a different problem, not addressed here). The grace period is a fixed constant -- long enough to absorb the flap/reconverge timescales discussed in [Section 7.6.2](#762-behavior-under-frr-route-churn-withdraw--re-inject), short enough that CRM `used` staleness is now bounded rather than unbounded. A neighbor removed while one of its next hop's pending destructions hasn't yet fired tears down cleanly through the existing `removeNeighbor()` path; the now-redundant pending destruction is simply dropped. |
There was a problem hiding this comment.
Specified in section 7.6.3: 5-second grace period, swept once per second, both fixed compile-time
constants (NEXTHOP_RECLAIM_GRACE_PERIOD_SECS, NEXTHOP_RECLAIM_SWEEP_INTERVAL_SECS), no CONFIG_DB
knob. A SelectableTimer owned by NeighOrch drives the sweep. Pending reclaims sit in a queue
ordered by due time plus a set of keys still due; cancelling removes the key from the set and
leaves the queue entry to be ignored when the sweep reaches it.
Warm restart was a genuine gap. Now specified: the queue does not survive a warm boot, and since
a reclaim is scheduled when a refcount reaches zero, a post-reconciliation sweep schedules one
for every plain next hop already sitting at zero.
|
§6.3 enumerates every internal That does not hold for all current callers. Of the 20
The remaining call sites outside |
|
|
||
| <a id="73-crm-and-reference-counting"></a> | ||
| ### 7.3 CRM and Reference Counting | ||
| - CRM next-hop counters (`CRM_IPV4_NEXTHOP` / `CRM_IPV6_NEXTHOP` / `CRM_MPLS_NEXTHOP`) are incremented/decremented exclusively inside `addNextHop()` / `removeNextHop()` / `removeMplsNextHop()` / `removeOverlayNextHop()`, unchanged. Only the *timing* of creation moves; the 1:1 accounting invariant is untouched, and counts now more accurately reflect real hardware usage than before. The debounced reclaim in [Section 7.6.3](#763-bounding-crm-staleness-debounced-reclaim-of-idle-next-hops) is a new *caller* of `removeNextHop()`, not a new decrement path, so this invariant still holds. |
There was a problem hiding this comment.
removeNextHop() only removes software bookkeeping; SAI removal and CRM accounting happen in removeNeighbor(), so calling it alone would leave the hardware object allocated. Please define a shared next-hop-only teardown path that frees the hardware and updates accounting without deleting the resolved neighbor.
| ### 7.1 FgNhgOrch (Fine-Grained ECMP) | ||
| Before this change, `addNextHop()` was the sole place that notified `FgNhgOrch::validNextHopInNextHopGroup()` that a next hop had become usable, and `removeNextHop()` was the sole place that notified `FgNhgOrch::invalidNextHopInNextHopGroup()` that it had gone away. Since neither is unconditionally called anymore -- both are skipped for a deferred neighbor whose SAI next hop was never created -- this add/remove pair had to be restored **symmetrically**, not just on the add side, otherwise a neighbor that resolves, is validated as an FG member, and is later removed without its next hop ever having been created would leave a stale FG member behind: | ||
|
|
||
| 1. **Add side.** `NeighOrch::addNeighbor()` explicitly calls `gFgNhgOrch->validNextHopInNextHopGroup(nhKey)` for every plain neighbor, restoring the "neighbor resolved" notification. `FgNhgOrch::validNextHopInNextHopGroup()` in turn calls `m_neighOrch->ensureNextHop(nexthop)` itself before activating a member, since being configured as an FG ECMP member is itself a legitimate reference that should trigger lazy creation (this call safely short-circuits reentrantly through `addNextHop()`'s own `validNextHopInNextHopGroup()` notification for the same key). As a direct consequence, any next hop that FG ECMP actually activates is guaranteed to exist as a SAI object from that point on, since activation is what creates it. |
There was a problem hiding this comment.
FG ECMP can skip a member when ensureNextHop() fails and complete the route successfully, leaving no retry pending when hardware capacity becomes available. Please define how resolved-but-resource-blocked members are tracked and retried until activation succeeds.
| #### 7.6.3 Bounding CRM Staleness: Debounced Reclaim of Idle Next Hops | ||
| Section 7.6.2 establishes that a plain next hop's SAI object is destroyed only when its neighbor is removed, never when its route/group reference count drops back to zero -- this is what makes steady-state route churn free. The flip side is that a next hop which becomes genuinely unreferenced (not just mid-flap, but never used again) can keep occupying a hardware slot indefinitely, for as long as the neighbor itself stays resolved. CRM `used`, accordingly, is a high-water mark of every next hop referenced at least once since resolution, not a live measure of current demand (see [Section 7.6.1](#761-adjacency-resolution-no-longer-reserves-a-next-hop)). | ||
|
|
||
| To bound that staleness without giving up the churn-free property above, reclaiming a next hop on reference count reaching zero is **debounced** rather than immediate or never: when a next hop's reference count drops to zero, its destruction is scheduled after a short, fixed grace period instead of happening right away. If any consumer references it again before the grace period elapses -- the ordinary churn case -- the pending destruction is simply cancelled and no SAI work happens at all, identical to today. Only a next hop that stays unreferenced for longer than the grace period is actually torn down, once, through the same teardown path `removeNeighbor()` already uses, so CRM decrement and `FgNhgOrch` invalidation ([Section 7.1](#71-fgnhgorch-fine-grained-ecmp)) stay consistent automatically, with no separate teardown logic to maintain. |
There was a problem hiding this comment.
A next hop can be created with zero references and remain unused if route installation fails, so its reference count never drops to zero to trigger reclamation. Please cover this case by scheduling reclamation at creation and cancelling it when a reference is acquired.
| ### 7.4 BFD-Down-Before-Creation Replay | ||
| A neighbor's SAI next hop may not exist yet when a `BFD` session for its peer transitions to `DOWN`. `updateNextHop()` cannot apply `NHFLAGS_IFDOWN` to an object that doesn't exist, so `NeighOrch` now tracks the last-known-down peer set (`m_bfdDownPeers`) independent of next-hop existence, and `addNextHop()` replays `NHFLAGS_IFDOWN` at lazy-creation time if the peer was already known to be down. | ||
|
|
||
| An entry in `m_bfdDownPeers` is removed in one of two ways, both required so this set cannot grow stale or unbounded: (1) `updateNextHop()` already erases it the moment the peer's BFD session reports `UP` again, unchanged from before this design; (2) `removeNeighbor()` additionally erases it once no resolved neighbor references that peer IP any more. (2) is necessary because a peer's BFD session can be torn down (e.g. the neighbor itself is removed) while its last-known state is still `DOWN` -- that session's teardown is not preceded by an `UP` transition, so (1) alone would never fire and the entry would linger indefinitely. Since `m_bfdDownPeers` is keyed purely by peer IP (matching `updateNextHop()`'s own matching semantics, which ignores alias/VRF), (2) only erases an entry once *every* neighbor sharing that IP is gone, not just the one being removed. |
There was a problem hiding this comment.
BFD sessions can outlive their neighbors, so deleting cached DOWN state on neighbor removal can incorrectly enable forwarding after ARP relearning. Please tie cache cleanup to BFD session lifecycle events rather than neighbor removal, including session deletion while the neighbor remains resolved.
Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
|
/azp run |
|
No pipelines are associated with this pull request. |
You're right, and both examples hold. The "mechanical, compiler-checked rename with no call-site New section 6.5 classifies every hasNextHop() call site outside neighorch.cpp before conversion:
Both of your examples are called out there: ~NextHopGroupMember() would create a SAI next hop and Mux turned out to need a third case, check section 7.2 and ensureLocalNextHop(). |
Ndancejic
left a comment
Ndancejic
left a comment
There was a problem hiding this comment.
Overall I think it's a good idea with practical benefits. I think the HLD could be a bit more concise and focused, it's very easy to get lost in the intricacies.
My main thoughts after reading:
- It sounds like CRM counts will follow real nexthop creation (will not increment during initial neighbor creation) If that's the case, what's preventing us from creating more neighbors than hardware nexthop limitations will allow (assuming that's something that's possible) and then breaking when all of these nexthops are referenced and created all at once? I'm assuming neighbor/nexthop limitations are the same for most but we should definitely confirm before making that assumption.
- I think (depending on time/complexity) implementing the bulk nexthop programming from the start is a good idea if we pursue this. It would take a bit more effort, but could save time in the future and be a big benefit if we just do things correctly from the start
Lets set up a meeting to review and align with if/when we want to do this and also make sure we're on the same page with the HLD
Optimization of neighbor next-hops by delaying the creation until referenced.