Repository navigation
[Hw PFCWD feature] Implementation for H/w based PFCWD - #4412
abhishek-nexthop wants to merge 15 commits into
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| bool SwitchOrch::isHwPfcWdSupportedSku() const | ||
| { | ||
| static const std::vector<std::regex> patterns = { | ||
| std::regex("nh-4010.*", std::regex_constants::icase) |
There was a problem hiding this comment.
Can we use capability query instead of SKU list?
There was a problem hiding this comment.
@kperumalbfn This is a temporary implementation for now. We’re keeping a fallback option in place for certain SKUs if needed. Over time, we’ll move it to the default approach, removing the whitelist logic and relying entirely on capabilities.
There was a problem hiding this comment.
@abhishek-nexthop there are lot of SKUs that support hardware based PFCWD, so it will be better to use the capability model.
There was a problem hiding this comment.
@kperumalbfn We are doing this intentionally so that it is not enabled for SKUs that haven’t been tested yet. Once a SKU qualifies the workflow, the whitelist can be updated. Using regex helps qualify a large number of chips with a single rule. After we have a sufficient number of qualified SKUs, we can enable it by default based on capability alone.
There is already a branch based on DLR init capability, which would be disabled if we removed this check.
We received the same suggestion from @vmittal-msft here
There was a problem hiding this comment.
@abhishek-nexthop , @vmittal-msft , responded to the comment on HLD, this should not be sku check
There was a problem hiding this comment.
@abhishek-nexthop - did you have any progress with changes related to this feature ?
There was a problem hiding this comment.
@eddyk-nvidia @noaOrMlnx We’re working with Broadcom to determine how to remove it. It won’t be part of the final PR. We’ll provide an update once we have a fix.
There was a problem hiding this comment.
thanks @abhishek-nexthop
do you have an ETA for the fix?
There was a problem hiding this comment.
@kperumalbfn @noaOrMlnx sorry for leaving this open so long.
On the substance: the selection is capability-based and there is no SKU list in it. SwitchOrch queries SAI_QUEUE_ATTR_ENABLE_PFC_DLDR and SAI_QUEUE_ATTR_PFC_DLR_INIT and publishes both as PFC_DLDR_CAPABLE / PFC_DLR_INIT_CAPABLE under SWITCH_CAPABILITY|switch; orchdaemon then picks the hardware orch, the hybrid software orch with the DLR handler, or the generic software watchdog from those two answers alone. A platform that advertises neither stays on the existing software path unchanged.
| static const std::vector<std::regex> patterns = { | ||
| std::regex("nh-4010.*", std::regex_constants::icase) | ||
| }; | ||
|
|
There was a problem hiding this comment.
@pinky-nexthop @abhishek-nexthop also for global PFCWD timer, which module sets SAI_SWITCH_ATTR_PFC_TC_DLD_INTERVAL and SAI_SWITCH_ATTR_PFC_TC_DLR_INTERVAL. There are high radix switches where global DLD/DLR makes sense instead of per-port config.
There was a problem hiding this comment.
@kperumalbfn Right now, we are not using global PFCWD timer. Is there any CLI support currently available for configuring global counters as part of a global workflow? Our current implementation is based on port-level timer support. While it is possible to extend this to switch-level support, we do not have the required chip available to validate it.
If switch-level/global support is needed, this can be used as a reference and extended further based on specific workflow requirements.
There was a problem hiding this comment.
@abhishek-nexthop We could also go per-capability. If some ASICs don't have per-port timer, then we should go with global timer. There is a global scope in PFCWD config
There was a problem hiding this comment.
@eddyk-nvidia What is the model in Spectrum ASICs? Does it support both port and global mode??
There was a problem hiding this comment.
@kperumalbfn Right now, we are not using global PFCWD timer. Is there any CLI support currently available for configuring global counters as part of a global workflow? Our current implementation is based on port-level timer support. While it is possible to extend this to switch-level support, we do not have the required chip available to validate it. If switch-level/global support is needed, this can be used as a reference and extended further based on specific workflow requirements.
@abhishek-nexthop - It should not be neccessary the HW capability. If for some deployment it is decided to use a single DLD and single DLR intervals (even if the HW platform supports the port-level granularity) we should allow it. Like in QOS configuration where we have some configuration global but then it can be overriden with per-port/per-tunnel settings
There was a problem hiding this comment.
@eddyk-nvidia This is a valid point. However, the current goal is to bring hardware-based recovery to parity with software-based recovery and support the workflows that exist today. We can track this separately by opening an issue and extend the logic later to support global-level configuration.
There was a problem hiding this comment.
@abhishek-nexthop - if you refer to SW-based recovery there is no such mechanism using DLD/R today in SONIC. So, parity is a but vague term here. The only PFC WD solution in SONIC today is using the Lua script and it is using the global timers (same for all ports/queues)
IMO - the design should support both Global and Per-Queue (overriding the global one) setting of intervals
There was a problem hiding this comment.
The only PFC WD solution in SONIC today is using the Lua script and it is using the global timers (same for all ports/queues)
@eddyk-nvidia Could we double-check this before we build on it? My understanding is that the software watchdog sets detection and restoration times per port rather than globally, is that not the case here? It seems like the only thing shared across all ports is the polling interval, which governs how often we check rather than the actual detect/restore timing. Could these two be getting mixed up?
Along the same lines, is there actually a way today to set a single global timer across all interfaces? As far as I can tell there's no command that does that, and the one command that applies to every port at once(start_default) doesn't take a timer at all. If that's right, can "parity with the software solution" really mean a global timer, given the software side never offered one?
If so, wouldn't a global timer on the hardware side be a new feature rather than parity? It might be worth framing it that way so we're aligned on expectations. Happy to go through it together if that would help.
cc: @kperumalbfn
| } | ||
|
|
||
| // Let SAI/SDK manage recovery automatically | ||
| notification.app_managed_recovery = false; |
There was a problem hiding this comment.
Is there any place where "true" is set ? I recall from HLD that "true" is supposed to trigger SAI_QUEUE_ATTR_PFC_DLR_INIT
Is this field set here later read by vendor SAI to understand which mode of work is expected ?
There was a problem hiding this comment.
@eddyk-nvidia This is to ensure we do not enter the manual recovery workflow. Currently, only auto-recovery is supported, where both detection and recovery are handled by the hardware. SAI_QUEUE_ATTR_PFC_DLR_INIT is required only for manual recovery workflows, where it must be set to true for the packet action to take effect, otherwise, no action occurs. It also needs to be reset back to false to allow recovery to proceed.
There was a problem hiding this comment.
@eddyk-nvidia following up, because the answer given above no longer matches the code.
The app_managed_recovery = false assignment has since been removed entirely. It never had any effect: the notification arrives over the ASIC_DB NOTIFICATIONS channel, so the struct is a local copy freed when the handler returns, and the value could not travel back to SAI. So to your original question — no, true was never set anywhere, and setting it there would not have reached the vendor SAI in any case.
Auto-recovery remains the only supported mode, but it is selected at configuration time by SAI_QUEUE_ATTR_ENABLE_PFC_DLDR in configureHwWatchdog(), not through this field.
| { | ||
| SWSS_LOG_INFO("Queue level PFC DLR INIT configuration is not supported"); | ||
| m_PfcDlrInitEnable = false; | ||
| fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_PFC_DLR_INIT_CAPABLE, "false"); |
There was a problem hiding this comment.
This SAI attribute might be supported ONLY if SAI_QUEUE_ATTR_PFC_DLR_INIT is used by the vendor SAI for triggering the recovery. It happens in case of "app_managed_recovery=true". When "app_managed_recovery=false" this SAI attribute might be not used. So, its check for capability query of PFC WD DLDR support is not ideal
There was a problem hiding this comment.
@eddyk-nvidia This workflow is currently applicable only to Broadcom chips. For Broadcom, SAI_QUEUE_ATTR_PFC_DLR_INIT indicates whether the PFCWD hardware is enabled. The workflow can be extended in the future to support other chipsets based on their respective use cases.
There was a problem hiding this comment.
@abhishek-nexthop - I am not sure it is the right approach. You mentioned earlier that SAI_QUEUE_ATTR_PFC_DLR_INIT is required only for "manual" triggering of recovery and now you say that it is the BRCM way to work. I think the design in generic SONIC should be vendor-agnostic
@kperumalbfn - FYI
There was a problem hiding this comment.
Gating hardware mode on SAI_QUEUE_ATTR_PFC_DLR_INIT conflated two different things: DLR_INIT is the manual-recovery trigger, not a statement that the ASIC implements deadlock detection and recovery. Using it as the capability check was vendor-specific reasoning in generic code, which is what you were objecting to.
The selection now gates on SAI_QUEUE_ATTR_ENABLE_PFC_DLDR, which is the attribute that actually describes DLDR support:
pfcDldrEnable = gSwitchOrch->checkPfcDldrEnable(); // SAI_QUEUE_ATTR_ENABLE_PFC_DLDR
pfcDlrInit = gSwitchOrch->checkPfcDlrInitEnable();
if (pfcDldrEnable) -> PfcWdHwOrch (full hardware)
else if (pfcDlrInit) -> PfcWdSwOrch<PfcWdDlrHandler, ...> (hybrid)
else -> generic software watchdog
DLR_INIT is kept only as the hybrid fallback, for platforms that advertise it without DLDR — those keep software detection with hardware-assisted recovery, rather than being pushed onto a hardware path they cannot serve.
SWITCH_CAPABILITY|switch advertises both PFC_DLDR_CAPABLE and PFC_DLR_INIT_CAPABLE, so a platform reporting neither stays on the generic software path with no SKU knowledge anywhere in the decision.
@kperumalbfn this also answers the capability-query point you raised — there is no SKU list in the selection path.
460658c to
66e2bd9
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks! ---Powered by SONiC BuildBot
|
Signed-off-by: Abhishek <abhishek@nexthop.ai>
Signed-off-by: Abhishek <abhishek@nexthop.ai>
Signed-off-by: Abhishek <abhishek@nexthop.ai>
…onality. Signed-off-by: Abhishek <abhishek@nexthop.ai>
Signed-off-by: Abhishek <abhishek@nexthop.ai>
…bility Sync pfcwdhworch.cpp/h to the implementation running on internal master: the start and disable paths log after the lossless-TC check rather than before it, and the no-lossless-TC case is handled by a shared helper. Add that helper to the base class, which upstream does not have yet: handleStartWdOnPortFailure() defers a port that is not PFC-ready and logs the deferral once, clearPfcWdPending() clears it, and m_pfcwdPendingPorts holds the set. Select the hardware watchdog on SAI_QUEUE_ATTR_ENABLE_PFC_DLDR rather than on an HWSKU match, and check it before PFC_DLR_INIT: a platform that owns detection and recovery in hardware can report both, and starting a software handler there would duplicate the hardware. PfcWdHwStats is already declared at namespace scope in pfcactionhandler.h, so drop the local copy. PfcWdQueueStats stays local: the one in PfcWdActionHandler is private and PfcWdHwOrch does not derive from it. Drop roundUpToValidInterval(), declared but never defined or called. Signed-off-by: arawat-nexthop <arawat@nexthop.ai>
5bc836d to
4c5333a
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
rebuild-source: sonic-net/pull/4412 @ nexthop-ai/sonic-swss 9e808c5 [case: upstream:open]
rebuild-source: sonic-net/pull/4412 @ nexthop-ai/sonic-swss 9e808c5 [case: upstream:open]
|
/azp run |
|
Commenter does not have sufficient privileges for PR 4412 in repo sonic-net/sonic-swss |
rebuild-source: sonic-net/pull/4412 @ nexthop-ai/sonic-swss 9e808c5 [case: upstream:open]
|
/azpw retry |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1225773: ✅Stage Test:
|
vmittal-msft
left a comment
There was a problem hiding this comment.
Re-reviewed at 9e808c5a. Thanks @abhishek-nexthop / @arawat-nexthop — my earlier inline comments are all addressed, and the capability-based selection (SAI_QUEUE_ATTR_ENABLE_PFC_DLDR in switchorch, now in master) resolves the SKU-whitelist concern from the earlier rounds. Approving.
Confirmed fixed (in 9e808c5a):
deleteEntrynow short-circuitsPFC_WD|GLOBAL→task_successbefore the port lookup, so a routineDELno longer logsInvalid port interface GLOBAL. MatchescreateEntryand the base class.pfc_stat_history=enableis now warn-and-accept (logs a clear "not supported in hardware mode, ignored" naming the port) instead of being silently dropped. Good call keeping the entry valid so default configs don't disable the watchdog.- The no-op
notification.app_managed_recovery = false;write is removed and replaced with an accurate comment about the ASIC_DB-forwarding path.
Builds (amd64/arm64/armhf/trixie/asan), CodeQL, Semgrep, DCO, CLA are green, and the unit suite (25 cases incl. the SAI-failure paths) is solid.
One request before merge (non-blocking):
- Please trim the PR description to match the code. It still lists "FlexCounter registration, COUNTERS_DB initialization, TX/RX statistics collection, delta calculation, detect/restore count tracking", but counters are deferred to the follow-up PR (the
c_portStatIds/c_queueStatIds/c_queueAttrIdsmembers are intentionally unused for now). A one-line "counters handled in a follow-up" note avoids misleadingshow pfcwd statsexpectations.
Minor nits (optional):
- Leftover empty comments at
pfcwdhworch.cpp:18(// Global instance pointer for SAI callback) and:36(// Set global instance pointer) — no such global pointer exists anymore; please remove. deleteEntryis tab-indented while the rest of the file uses 4 spaces; and there's a trailing-whitespace line inpfcwdorch.hafterupdateDlrPacketActionInStateTable().- Consider a direct unit test for the two newest fixes (
deleteEntry("GLOBAL") == task_successand thepfc_stat_historywarn path);DeleteEntryValidationcurrently only covers the non-existent-port case.
CI note: the red Test vstest is unrelated flakiness — test_V6AclRuleArbitraryIpv6Mask, test_SampledMirrorRejectsWhenSflowBound, and the P4RT test_RemovePrunedWcmpGroupMember/test_LargeWatchportOperations — all passed on rerun, TestAsan vstest is fully green, and no pfcwd test failed. A re-run should clear it.
(Note: the formal CHANGES_REQUESTED on record is @eddyk-nvidia's from the earlier design discussion — the per-queue interval + capability-based selection work since then should address it, but it needs their re-review to dismiss.)
rebuild-source: sonic-net/pull/4412 @ nexthop-ai/sonic-swss 9e808c5 [case: upstream:open]
|
/azp run |
|
Commenter does not have sufficient privileges for PR 4412 in repo sonic-net/sonic-swss |
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1225773: ✅Stage Test:
|
|
@vmittal-msft could I request you to re-run /azp run, I am not allowed to and this build is failing dvs tests? If you could, we can try closing and re-opening the Pr as well. thanks! |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1237219: ✅Stage Test:
|
rebuild-source: sonic-net/pull/4412 @ nexthop-ai/sonic-swss 9e808c5 [case: upstream:open]
rebuild-source: sonic-net/pull/4412 @ nexthop-ai/sonic-swss 9e808c5 [case: upstream:open]
|
@vmittal-msft I cannot change the PR description since PR was opened by abhishek-nexthop (stale handle) |
|
@arawat-nexthop please attach a file with updated description. i can try to update. |
|
pr4412-description.md |
What I did
Added hardware PFC watchdog support to orchagent, using the ASIC's native deadlock detection and recovery (DLDR) in place of the software polling loop.
New
PfcWdHwOrch, selected at daemon start purely from SAI capability — there is no SKU or platform list in the decision:SAI_QUEUE_ATTR_ENABLE_PFC_DLDRadvertised → hardware watchdogSAI_QUEUE_ATTR_PFC_DLR_INITadvertised → software watchdog with the DLR handler (hybrid: software detection, hardware-assisted recovery)Both capabilities are published under
STATE_DB SWITCH_CAPABILITY|switchasPFC_DLDR_CAPABLEandPFC_DLR_INIT_CAPABLE.Configuration: validates detection and restoration times against the ranges queried from the ASIC (
SAI_SWITCH_ATTR_PFC_TC_DLD_INTERVAL_RANGE/..._DLR_INTERVAL_RANGE), enforces a single switch-wide DLR packet action across all configured ports, programs the per-port timer intervals andSAI_QUEUE_ATTR_ENABLE_PFC_DLDRon each lossless TC, and reads the programmed values back.State reporting:
PFC_WD_STATE_TABLE|PFC_WD—RECOVERY_MECHANISM, the supported timer ranges, andDLR_PACKET_ACTIONPFC_WD_HW_STATE|<port>— per-port status, plus both the configured and the actually-programmed timer values, so timer granularity rounding by the ASIC is visible to the operatorTeardown: refuses configuration changes and deletes while a lossless queue on the port is stormed, clears DLDR and the timer intervals, removes the per-port state, and resets the switch-wide action when the last port is removed.
Storm reporting: the SAI deadlock notification is forwarded to the ASIC_DB
NOTIFICATIONSchannel and consumed on the orchagent thread through aNotificationConsumer, so no DB work happens on the libsairedis callback thread. Detect and restore are each reported once per transition.Warm reboot: configuration is replayed from CONFIG_DB on start.
Unit tests for the hardware orchestration path.
Counter handling is deliberately not part of this change — it is the follow-up in #4854.
Why I did it
ASIC-native deadlock detection removes the dependency on the software polling loop: detection keeps working while orchagent is busy, the action is applied in hardware rather than through an ACL, and the detection interval is bounded by the ASIC rather than by the counter poll period. The implementation shares the existing base class so that platforms without the capability keep today's software behaviour unchanged.
How I verified it
PfcWdHwOrchTest) covering configuration validation, timer-range enforcement, switch-wide action consistency, per-port state lifecycle, DLDR set and clear, and the SAI failure paths.PFC_WD|GLOBALkey cases (test_pfcwd_global_key_del_no_crash,test_pfcwd_global_hdel_last_field_no_crash,test_pfcwd_del_unknown_port_no_crash).Details if related
BIG_RED_SWITCH,pfcwd intervalandpfc_stat_historyare software-watchdog concepts with no hardware equivalent.pfc_stat_historyis accepted and logged as ignored rather than rejected, so an existing configuration that sets it does not lose its watchdog.PFC_WD|GLOBALentry, since the poll interval and big-red-switch fields do not apply.