Skip to content

[Hw PFCWD feature] Refactor existing pfcwd code#4358

Open
abhishek-nexthop wants to merge 4 commits into
sonic-net:masterfrom
nexthop-ai:refactor-pfcwd
Open

[Hw PFCWD feature] Refactor existing pfcwd code#4358
abhishek-nexthop wants to merge 4 commits into
sonic-net:masterfrom
nexthop-ai:refactor-pfcwd

Conversation

@abhishek-nexthop

@abhishek-nexthop abhishek-nexthop commented Mar 18, 2026 •
edited
Loading

Copy link
Copy Markdown

What I did

  • Created new base class PfcWdBaseOrch in pfcwdorch.cpp containing common functionality shared between software and hardware PFC watchdog implementations
  • Moved software-specific PFC watchdog implementation (PfcWdSwOrch) to new file pfcwdsworch.cpp with corresponding header pfcwdsworch.h
  • Renamed template class PfcWdOrch to PfcWdBaseOrch to better reflect its role as a base class
  • Updated Makefile.am to include new pfcwdsworch.cpp source file
  • Updated includes in orchdaemon.h and test files to reference the new header structure
  • Maintained all existing functionality including Big Red Switch mode, storm detection/restoration, and warm-reboot support

Why I did it
Refactor PFC Watchdog code to improve maintainability by splitting base class and software implementation in different file. Base file now can be extended for hardware based implementation.

How I verified it
Build the orchagent and ran existing pfc test to check any change in behavior.

HLD: 2159

@mssonicbld

mssonicbld commented Mar 18, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

azure-pipelines Bot commented Mar 18, 2026

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@abhishek-nexthop abhishek-nexthop changed the title Refactor pfcwd orch code. [Hw PFCWD feature] Refactor existing pfcwd code Mar 18, 2026
@mssonicbld

mssonicbld commented Mar 18, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@abhishek-nexthop
abhishek-nexthop marked this pull request as draft March 18, 2026 17:04
@azure-pipelines

azure-pipelines Bot commented Mar 18, 2026

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld

mssonicbld commented Mar 18, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@abhishek-nexthop
abhishek-nexthop marked this pull request as ready for review March 18, 2026 17:22
@azure-pipelines

azure-pipelines Bot commented Mar 18, 2026

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

pinky-nexthop
pinky-nexthop reviewed Mar 19, 2026
View reviewed changes
Comment thread orchagent/pfcwdorch.cpp
@pinky-nexthop

pinky-nexthop commented Mar 19, 2026

Copy link
Copy Markdown
Contributor

@radha-danda
@kperumalbfn
Please review this PR. The next set of PRs will add the h/w recovery code but this is a pre-requisite to them.
This PR mostly refactors the code as per the implementation plan discussed in the HLD 2159

@abhishek-nexthop

abhishek-nexthop commented Mar 20, 2026

Copy link
Copy Markdown
Author

/azpw run

@mssonicbld

mssonicbld commented Mar 20, 2026

Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines

azure-pipelines Bot commented Mar 20, 2026

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld

mssonicbld commented Mar 28, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

azure-pipelines Bot commented Mar 28, 2026

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld

mssonicbld commented Mar 30, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

azure-pipelines Bot commented Mar 30, 2026

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@pinky-nexthop pinky-nexthop mentioned this pull request Apr 23, 2026
Open
@pinky-nexthop

pinky-nexthop commented May 26, 2026

Copy link
Copy Markdown
Contributor

/azwp retry

@abhishek-nexthop

abhishek-nexthop commented May 26, 2026

Copy link
Copy Markdown
Author

/azpw retry

@mssonicbld

mssonicbld commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Retrying failed(or canceled) jobs...

@mssonicbld

mssonicbld commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Build not found. Please close and reopen the PR or rebase your branch to trigger a new build.

@mssonicbld

mssonicbld commented May 27, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

azure-pipelines Bot commented May 27, 2026

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vmittal-msft
vmittal-msft requested a review from Copilot June 20, 2026 00:10
@mssonicbld

mssonicbld commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 16, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@pinky-nexthop

pinky-nexthop commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

report_pfc_restored

@vmittal-msft Latest commit fixes the comments. Kindly have a look. Thanks!

vmittal-msft
vmittal-msft reviewed Jul 17, 2026
View reviewed changes

@vmittal-msft vmittal-msft left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed this as a pure move-refactor (splitting the templated PfcWdOrch into a non-template base PfcWdBaseOrch in pfcwdorch.cpp/h and moving the software implementation PfcWdSwOrch into new pfcwdsworch.cpp/h). Verified faithfulness at the line level, not just by reading the diff.

LGTM — no behavioral changes, structurally sound.

What I verified

  • Function inventory — every function from the original pfcwdorch.cpp maps to a location in the new files; nothing dropped or duplicated. counterIdsToStr moved to the base (now an inline static template), report_pfc_storm moved to the base with a field-based signature, and report_pfc_restored / getLosslessTcsForPort were extracted from previously-inline code.
  • Line-level equivalence — after normalizing class names and doing a set-diff of old vs. new (base + sw combined), the only differences are the expected transformations. All other function bodies are byte-identical (indentation preserved). The report_pfc_storm event params still map 1:1 (entry->first→queueId, entry->second.portId→portId, entry->second.index→queueIndex, entry->second.portAlias→portAlias), and report_pfc_restored preserves the exact restore log.
  • Macros — PFC_WD_TC_MAX now has a single definition in orch.h (removed from countercheckorch.h, which still includes orch.h); config/timer macros moved to pfcwdorch.h; SW-only macros correctly live in pfcwdsworch.cpp. No duplicate/missing definitions.
  • Templates & linkage — all four explicit PfcWdSwOrch<...> instantiations are preserved at the tail of pfcwdsworch.cpp. The base .cpp include set is byte-identical to the original, so transitive types (SwitchOrch, handlers, flex-counter manager) resolve as before. No dangling PfcWdOrch / PfcDlrPacketAction references anywhere.
  • Tests — mock_orchagent_main.h adds #include "pfcwdsworch.h" inside both the #define private public and #define protected public blocks, so portsorch_ut / flexcounter_ut access to m_pfcwd_ports, m_entryMap, and m_platform still works; gPfcwdOrch remains typed as the still-templated PfcWdSwOrch<...>*. Makefiles updated.
  • Nice cleanup: moving packet_action_map out of the header into pfcwdorch.cpp removes an unused internal-linkage copy from every TU that included the header.

Minor nits (non-blocking)

  1. PFC_WD_POLL_TIMEOUT (pfcwdsworch.cpp) is defined but unused. It was dead in the original too, but since one of the commits is "remove unused locals and dead code," it would be consistent to drop it here as well.
  2. Tiny log-noise change: if getPortPfcWatchdogStatus fails inside the new getLosslessTcsForPort, registerInWdDb now additionally logs NOTICE "No lossless TC found..." after the helper's ERROR. The original returned immediately after the error without that NOTICE. Purely cosmetic.
  3. getLosslessTcsForPort isn't reused in the two similar pfcMask loops in enableBigRedSwitchMode — acceptable since their logic differs slightly, just noting it for a potential follow-up.

@vmittal-msft

vmittal-msft commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

@abhishek-nexthop @pinky-nexthop

Since this is important feature as well as touching critical code base: please check following for all HW PFCWD PRs -

  1. Make sure all sonic-mgmt/snappi tests pass after these changes related to PFCWD
  2. Make sure these tests pass for XGS/DNX both type of SKUs
  3. Make sure above tests work fine in both SW and HW based model.
  4. Can you please help me remember if there is any knob available to switch backto SW mode in case of any issues ?

@abhishek-nexthop @pinky-nexthop anything we can handle here. Also, please add tests for coverage shortage

@pinky-nexthop

pinky-nexthop commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

@abhishek-nexthop @pinky-nexthop
Since this is important feature as well as touching critical code base: please check following for all HW PFCWD PRs -

  1. Make sure all sonic-mgmt/snappi tests pass after these changes related to PFCWD
  2. Make sure these tests pass for XGS/DNX both type of SKUs
  3. Make sure above tests work fine in both SW and HW based model.
  4. Can you please help me remember if there is any knob available to switch backto SW mode in case of any issues ?

@abhishek-nexthop @pinky-nexthop anything we can handle here. Also, please add tests for coverage shortage

@vmittal-msft We have requested broadcom for a knob to disable h/w recovery mode. Currently s/w can be selected using
sai_pfc_dlr_init_capability=0.
we have another PR #4760 that can fix the missing coverage required here, so waiting for that to merge and then rebase this.

@abhishek-nexthop
Refactor pfcwd orch code.
4d5b5c1 
Signed-off-by: Abhishek <abhishek@nexthop.ai>
@abhishek-nexthop
Add helper function counterIdstoStr and comment stating common helper…
e7063cc 
… functions.

Signed-off-by: Abhishek <abhishek@nexthop.ai>
@mssonicbld

mssonicbld commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 24, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@abhishek-nexthop

abhishek-nexthop commented Jul 24, 2026

Copy link
Copy Markdown
Author

/azp run Azure.sonic-swss

@azure-pipelines

azure-pipelines Bot commented Jul 24, 2026

Copy link
Copy Markdown
Commenter does not have sufficient privileges for PR 4358 in repo sonic-net/sonic-swss

@abhishek-nexthop
Address review comments
54ba349 
Remove unused locals and dead code, consolidate shared macros in
pfcwdorch.h with PFC_WD_TC_MAX in orch.h, move packet_action_map to
pfcwdorch.cpp and rename PfcDlrPacketAction to m_pfcDlrPacketAction.

Signed-off-by: Abhishek <abhishek@nexthop.ai>
@mssonicbld

mssonicbld commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 24, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@pinky-nexthop

pinky-nexthop commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

/azpw retry

@mssonicbld

mssonicbld commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Retrying failed(or canceled) jobs...

@mssonicbld

mssonicbld commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Retrying failed(or canceled) stages in build 1173721:

✅Stage TestAsan:

  • Job vstest: retried.

✅Stage Test:

  • Job vstest: retried.
  • Job vstest: retried.

@mssonicbld

mssonicbld commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 25, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@mssonicbld

mssonicbld commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 25, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@abhishek-nexthop
Add pfcwd alert and forward action tests
d96f5b2 
Cycle drop, alert and forward actions in test_pfcwd_software_single_queue
and assert the APPL_DB PFC_WD_TABLE_INSTORM mirroring per storm cycle.
Add a BIG_RED_SWITCH invalid-value case and a no-lossless-TC port case.

Signed-off-by: Abhishek <abhishek@nexthop.ai>
@mssonicbld

mssonicbld commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 25, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmittal-msft vmittal-msft vmittal-msft left review comments
@saksarav-nokia saksarav-nokia saksarav-nokia left review comments
Copilot code review Copilot Copilot left review comments
@kperumalbfn kperumalbfn Awaiting requested review from kperumalbfn kperumalbfn is a code owner
@prsunny prsunny Awaiting requested review from prsunny prsunny is a code owner
@stephenxs stephenxs Awaiting requested review from stephenxs
+1 more reviewer
@pinky-nexthop pinky-nexthop pinky-nexthop approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@abhishek-nexthop @mssonicbld @pinky-nexthop @vmittal-msft @prsunny @saksarav-nokia