[PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities when Asym PFC is enabled #1360

volodymyrsamotiy · 2020-07-23T08:04:36Z

Signed-off-by: Volodymyr Samotiy volodymyrs@mellanox.com

What I did
Add possibility for PFCWD to detect PFC storm on all priorities when asymmetric PFC is enabled.

According to requirements PFCWD should detect PFC storm on the lossy priorities for the port on which asymmetric PFC is enabled.
(when asymmetric PFC is enabled on some port that means such port is enabled for receiving PFC frames on all priorities)

Why I did it
To fix issue #1128.

How I verified it

Enable asymmetric PFC on test port.

pfc config asymmetric on <port>

Enable PFCWD on the test port.

pfcwd start --action drop --restoration-time 3000 <port> 1000
pfcwd interval 500

Run pause frames to the test port for all priorities.
Verify that PFCWD detected storm for all priorities.

pfcwd show stats

Details if related
N/A

liat-grozovik · 2020-07-27T12:25:28Z

retest vs please

liat-grozovik · 2020-07-28T17:32:06Z

retest this please

volodymyrsamotiy · 2020-07-29T07:07:45Z

retest this please

liat-grozovik · 2020-08-10T07:06:35Z

retest this please

… when Asym PFC is enabled Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

volodymyrsamotiy · 2020-08-12T08:22:38Z

retest this please

volodymyrsamotiy · 2020-08-12T09:07:12Z

retest this please

liat-grozovik · 2020-08-13T08:39:06Z

orchagent/portsorch.cpp

+        sai_status_t status = sai_port_api->set_port_attribute(portId, &attr);
+        if (status != SAI_STATUS_SUCCESS)
+        {
+            SWSS_LOG_ERROR("Failed to set PFC bitmask 0x%x for the port 0x%" PRIx64 " (rc:%d)",


not sure i understand this part.
you are using the tx pfc bitmask for setting flow control and the message in the log is still pfc bitmask which i can see later on you have one for rx and tx.

This logic is explained in the "Asymmentric PFC" HLD in the "2.2.1 PortsOrch" section
https://github.com/Azure/SONiC/wiki/Asymmetric-PFC-High-Level-Design#221-portsorch

liat-grozovik · 2020-08-13T08:42:40Z

orchagent/portsorch.cpp

+    }
+    else
+    {
+        pfc_rx_bitmask = pfc_tx_bitmask;


logic has changed, can you elaborate it in the code?

Logic is exactly the same but code was refactored according to changes for set/getPortPfc API.

liat-grozovik · 2020-08-13T08:43:24Z

orchagent/portsorch.cpp


    sai_status_t status = sai_port_api->set_port_attribute(port.m_port_id, &attr);
    if (status != SAI_STATUS_SUCCESS)
    {
-        SWSS_LOG_ERROR("Failed to set PFC mode %d to port id 0x%" PRIx64 " (rc:%d)", port.m_pfc_asym, port.m_port_id, status);
+        SWSS_LOG_ERROR("Failed to set PFC mode %d for the port 0x%" PRIx64 " (rc:%d)",


IMO you should remove 'the' after the 'for'

liat-grozovik · 2020-08-13T08:45:40Z

orchagent/qosorch.cpp

@@ -1329,7 +1329,7 @@ task_process_status QosOrch::handlePortQosMapTable(Consumer& consumer)

        if (pfc_enable)
        {
-            if (!gPortsOrch->setPortPfc(port.m_port_id, pfc_enable))
+            if (!gPortsOrch->setPortPfc(port.m_port_id, pfc_enable, pfc_enable))


the pfc_enable is on/off? if this is the case all other places are mask. can you please elaborate on that?

Yes, it is according to requirements, "Asym PFC" is "on"/"off" port attribute for the user but SAI requires PFC value to be as bit vector.
https://github.com/Azure/SONiC/wiki/Asymmetric-PFC-High-Level-Design#11-functional-requirements

liat-grozovik · 2020-09-30T07:48:06Z

retest this please

liat-grozovik · 2020-12-29T18:06:52Z

@volodymyrsamotiy please handle conflicts

…wd_asym_pfc Conflicts: orchagent/port.h orchagent/portsorch.h

… when Asym PFC is enabled * Correct error message for setting PFC mode Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

volodymyrsamotiy · 2021-02-23T11:41:38Z

@volodymyrsamotiy please handle conflicts

handled

… when Asym PFC is enabled * Fix LGTM failure Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

liat-grozovik · 2021-02-28T16:24:17Z

@neethajohn kindly reminder. this PR should go to 202012 as well.

dprital · 2021-03-05T00:40:32Z

@neethajohn Kindly Reminder, we are waiting for the merge of this PR. Thanks.

dprital · 2021-03-10T08:54:37Z

@neethajohn - Who can merge this PR ? Thanks.

neethajohn

Please add unit tests

orchagent/portsorch.cpp

orchagent/pfcwdorch.cpp

neethajohn · 2021-03-11T04:42:25Z

orchagent/pfcactionhandler.cpp

    {
        SWSS_LOG_ERROR("Failed to get PFC mask on port 0x%" PRIx64, port);
    }

-    pfcMask = static_cast<uint8_t>(pfcMask & ~(1 << queueId));
+    pfcTxMask = static_cast<uint8_t>(pfcTxMask & ~(1 << queueId));


Why do we need to modify Tx Mask? In any case, its a noop if pfcwd is triggered on a lossy priority because the txMask only contains priorities 3 and 4

Agree, we don' need it. Fixed to modify only Rx mask.

neethajohn · 2021-03-11T04:43:59Z

orchagent/pfcactionhandler.cpp

    {
        SWSS_LOG_ERROR("Failed to get PFC mask on port 0x%" PRIx64, getPort());
    }

-    pfcMask = static_cast<uint8_t>(pfcMask | (1 << getQueueId()));
+    pfcTxMask = static_cast<uint8_t>(pfcTxMask | (1 << getQueueId()));


We shouldn't be modifying Tx Mask since it contains only priorities 3 and 4. We will setting tx mask to incorrect values on pfcwd storm restore if pfcwd was triggered on a lossy priority

Agree, we don' need it. Fixed to modify only Rx mask.

Thinking about this more, we need to set the txmask but need to do it only for queues set in 'pfc_enable' field

Ok, I will change it back and verify

…wd_asym_pfc Conflicts: orchagent/port.h orchagent/portsorch.cpp

… when Asym PFC is enabled * Fix review comments Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

… when Asym PFC is enabled * Fix SAI errors handling for setPortPfc() Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

lgtm-com · 2021-12-22T12:20:20Z

This pull request fixes 1 alert when merging d710235 into a4c80c3 - view on LGTM.com

fixed alerts:

1 for Unused import

volodymyrsamotiy · 2021-12-24T09:36:20Z

/azpw run

mssonicbld · 2021-12-24T09:36:23Z

/AzurePipelines run

azure-pipelines · 2021-12-24T09:36:33Z

Azure Pipelines successfully started running 1 pipeline(s).

volodymyrsamotiy · 2021-12-28T08:27:18Z

/azpw run

mssonicbld · 2021-12-28T08:27:19Z

/AzurePipelines run

azure-pipelines · 2021-12-28T08:27:29Z

Azure Pipelines successfully started running 1 pipeline(s).

liat-grozovik · 2022-01-02T15:05:57Z

/azp run Azure.sonic-swss

azure-pipelines · 2022-01-02T15:06:07Z

Azure Pipelines successfully started running 1 pipeline(s).

…wd_asym_pfc Conflicts: tests/test_pfcwd.py

lgtm-com · 2022-01-12T09:51:29Z

This pull request fixes 1 alert when merging 6fd9206 into 8fd6e48 - view on LGTM.com

fixed alerts:

1 for Unused import

…witch" for deriving tor ipv4_address (sonic-net#1360) This PR provides the support for fixing the show muxcable config. There was a change in the Config_DB schema where there was an addition of key-value pair "peer_switch:tor_name" in DEVICE_METADATA|localhost table which actually gives the other TOR's name (hostname). This TOR name is then utilized in PEER_SWITCH|switchname table which has "address_ipv4: IPv4 address" key-value pair, which correctly gives the ip address for displaying on the show mux config What is the motivation for this PR? To add the fix the working for show muxcable config Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

…wd_asym_pfc

…orch Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

…w one Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

liat-grozovik · 2022-03-30T07:09:26Z

@neethajohn please note comments were addressed. please review and signoff.

dprital · 2022-04-13T09:45:10Z

@neethajohn - Can you please approve this PR ?

dprital · 2022-04-26T07:08:40Z

@neethajohn - as discussed yesterday, please review. Thanks.

liat-grozovik · 2023-05-29T09:01:21Z

@volodymyrsamotiy please handle conflicts and we will have it merged.

liat-grozovik · 2023-08-27T07:25:03Z

@volodymyrsamotiy kindly reminder to handle conflicts so I can merge

liat-grozovik · 2024-05-07T11:11:46Z

The issue was reported many many releases a go.
due to very low attention from the community and probably not in interest of others, i am closing this PR.
if at any time in the future the fix will be needed, we will need community attention. till then the issues is open

liat-grozovik requested a review from neethajohn August 4, 2020 16:32

volodymyrsamotiy force-pushed the pfcwd_asym_pfc branch from 8193edb to cb791cc Compare August 12, 2020 06:55

[PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities…

0f3e9f3

… when Asym PFC is enabled Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

volodymyrsamotiy force-pushed the pfcwd_asym_pfc branch from cb791cc to 0f3e9f3 Compare August 12, 2020 07:15

volodymyrsamotiy marked this pull request as ready for review August 12, 2020 09:06

liat-grozovik reviewed Aug 13, 2020

View reviewed changes

volodymyrsamotiy added 2 commits February 23, 2021 12:45

Merge branch 'master' of https://github.com/Azure/sonic-swss into pfc…

93fb08e

…wd_asym_pfc Conflicts: orchagent/port.h orchagent/portsorch.h

[PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities…

3ab862b

… when Asym PFC is enabled * Correct error message for setting PFC mode Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

[PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities…

571e991

… when Asym PFC is enabled * Fix LGTM failure Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

liat-grozovik changed the title ~~[PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities…~~ [PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities when Asym PFC is enabled Feb 25, 2021

liat-grozovik previously approved these changes Feb 25, 2021

View reviewed changes

liat-grozovik added the Request for 202012 Branch label Feb 28, 2021

neethajohn reviewed Mar 11, 2021

View reviewed changes

neethajohn mentioned this pull request Mar 11, 2021

[pfc] Introduce pfc config bitmask to track user-configured pfc enabled TCs on physical port #1612

Open

1 task

Merge branch 'master' of https://github.com/Azure/sonic-swss into pfc…

a6b31ea

…wd_asym_pfc Conflicts: orchagent/port.h orchagent/portsorch.cpp

volodymyrsamotiy dismissed liat-grozovik’s stale review via a6b31ea June 18, 2021 09:46

volodymyrsamotiy added 2 commits June 18, 2021 13:15

[PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities…

acf21f5

… when Asym PFC is enabled * Fix review comments Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

[PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities…

0d36d37

… when Asym PFC is enabled * Fix SAI errors handling for setPortPfc() Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

liat-grozovik added Request for 202111 Branch and removed Request for 202012 Branch labels Jan 2, 2022

Merge branch 'master' of https://github.com/Azure/sonic-swss into pfc…

6fd9206

…wd_asym_pfc Conflicts: tests/test_pfcwd.py

volodymyrsamotiy added 4 commits March 29, 2022 19:07

Merge branch 'master' of https://github.com/Azure/sonic-swss into pfc…

c7f26fc

…wd_asym_pfc

[PFCWD + Asym PFC]: Align the code according to latest changes in qos…

0c720df

…orch Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

[PFCWD + Asym PFC]: Remove old VS test which should be replaced by ne…

7990b32

…w one Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

[PFCWD + Asym PFC]: Add VS test for PFCWD with enabled asymetric PFC

13b56d8

Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

rlhui requested a review from vmittal-msft May 23, 2022 23:06

liat-grozovik added Request for 202211 Branch Request for 202305 Branch and removed Request for 202106 Branch Request for 202111 Branch labels Aug 27, 2023

liat-grozovik closed this May 7, 2024

[PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities when Asym PFC is enabled #1360

[PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities when Asym PFC is enabled #1360

Conversation

volodymyrsamotiy commented Jul 23, 2020 • edited by liat-grozovik Loading

liat-grozovik commented Jul 27, 2020

liat-grozovik commented Jul 28, 2020 • edited Loading

volodymyrsamotiy commented Jul 29, 2020

liat-grozovik commented Aug 10, 2020

volodymyrsamotiy commented Aug 12, 2020

volodymyrsamotiy commented Aug 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liat-grozovik commented Sep 30, 2020

liat-grozovik commented Dec 29, 2020

volodymyrsamotiy commented Feb 23, 2021

liat-grozovik commented Feb 28, 2021

dprital commented Mar 5, 2021

dprital commented Mar 10, 2021

neethajohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Dec 22, 2021

volodymyrsamotiy commented Dec 24, 2021

mssonicbld commented Dec 24, 2021

azure-pipelines bot commented Dec 24, 2021

volodymyrsamotiy commented Dec 28, 2021

mssonicbld commented Dec 28, 2021

azure-pipelines bot commented Dec 28, 2021

liat-grozovik commented Jan 2, 2022

azure-pipelines bot commented Jan 2, 2022

lgtm-com bot commented Jan 12, 2022

liat-grozovik commented Mar 30, 2022

dprital commented Apr 13, 2022

dprital commented Apr 26, 2022

liat-grozovik commented May 29, 2023

liat-grozovik commented Aug 27, 2023

liat-grozovik commented May 7, 2024

volodymyrsamotiy commented Jul 23, 2020 •

edited by liat-grozovik

Loading

liat-grozovik commented Jul 28, 2020 •

edited

Loading