[201911][pfcwd] Avoid ingress drop by not attaching zero profiles when pfc storm is detected (#2279)

What I did
According to the current pfcwd detection logic on certain platforms, when wd fires, we create an ingress zero pool, ingress zero profile, egress zero pool, egress zero profile (if not already created) and then attach the ingress zero profile to the ingress pg and egress zero profile to the egress queue. As a result traffic ingressing that port/pg and egressing that port/queue will get dropped.

The current changes are done to avoid dropping traffic that is ingressing the port/pg that is in storm. The code changes in this PR avoid creating the ingress zero pool and profile and not attach any zero profile to the ingress pg when pfcwd is triggered

How I verified it
Modified the pfcwd func tests (sonic-net/sonic-mgmt#5665) and testcase passed on Mellanox platform

Signed-off-by: Neetha John <[email protected]>

Author: neethajohn

orchagent/bufferorch.cpp

@@ -728,11 +728,6 @@ task_process_status BufferOrch::processPriorityGroup(Consumer &consumer)
                 SWSS_LOG_ERROR("Invalid pg index specified:%zd", ind);
                 return task_process_status::task_invalid_entry;
             }
-            if (port.m_priority_group_lock[ind])
-            {
-                SWSS_LOG_WARN("Priority group %zd on port %s is locked, will retry", ind, port_name.c_str());
-                return task_process_status::task_need_retry;
-            }
             pg_id = port.m_priority_group_ids[ind];
             SWSS_LOG_DEBUG("Applying buffer profile:0x%" PRIx64 " to port:%s pg index:%zd, pg sai_id:0x%" PRIx64, sai_buffer_profile, port_name.c_str(), ind, pg_id);
             sai_status_t sai_status = sai_buffer_api->set_ingress_priority_group_attribute(pg_id, &attr);

orchagent/pfcactionhandler.cpp

@@ -441,7 +441,7 @@ PfcWdZeroBufferHandler::PfcWdZeroBufferHandler(sai_object_id_t port,
         return;
     }
 
-    setPriorityGroupAndQueueLockFlag(portInstance, true);
+    setQueueLockFlag(portInstance, true);
 
     sai_attribute_t attr;
     attr.id = SAI_QUEUE_ATTR_BUFFER_PROFILE_ID;
@@ -457,7 +457,7 @@ PfcWdZeroBufferHandler::PfcWdZeroBufferHandler(sai_object_id_t port,
     sai_object_id_t oldQueueProfileId = attr.value.oid;
 
     attr.id = SAI_QUEUE_ATTR_BUFFER_PROFILE_ID;
-    attr.value.oid = ZeroBufferProfile::getZeroBufferProfile(false);
+    attr.value.oid = ZeroBufferProfile::getZeroBufferProfile();
 
     // Set our zero buffer profile
     status = sai_queue_api->set_queue_attribute(queue, &attr);
@@ -469,35 +469,6 @@ PfcWdZeroBufferHandler::PfcWdZeroBufferHandler(sai_object_id_t port,
 
     // Save original buffer profile
     m_originalQueueBufferProfile = oldQueueProfileId;
-
-    // Get PG
-    sai_object_id_t pg = portInstance.m_priority_group_ids[static_cast <size_t> (queueId)];
-
-    attr.id = SAI_INGRESS_PRIORITY_GROUP_ATTR_BUFFER_PROFILE;
-
-    // Get PG's buffer profile
-    status = sai_buffer_api->get_ingress_priority_group_attribute(pg, 1, &attr);
-    if (status != SAI_STATUS_SUCCESS)
-    {
-        SWSS_LOG_ERROR("Failed to get buffer profile ID on PG 0x%" PRIx64 ": %d", pg, status);
-        return;
-    }
-
-    // Set zero profile to PG
-    sai_object_id_t oldPgProfileId = attr.value.oid;
-
-    attr.id = SAI_INGRESS_PRIORITY_GROUP_ATTR_BUFFER_PROFILE;
-    attr.value.oid = ZeroBufferProfile::getZeroBufferProfile(true);
-
-    status = sai_buffer_api->set_ingress_priority_group_attribute(pg, &attr);
-    if (status != SAI_STATUS_SUCCESS)
-    {
-        SWSS_LOG_ERROR("Failed to set buffer profile ID on pg 0x%" PRIx64 ": %d", pg, status);
-        return;
-    }
-
-    // Save original buffer profile
-    m_originalPgBufferProfile = oldPgProfileId;
 }
 
 PfcWdZeroBufferHandler::~PfcWdZeroBufferHandler(void)
@@ -523,26 +494,12 @@ PfcWdZeroBufferHandler::~PfcWdZeroBufferHandler(void)
         return;
     }
 
-    sai_object_id_t pg = portInstance.m_priority_group_ids[size_t(getQueueId())];
-
-    attr.id = SAI_INGRESS_PRIORITY_GROUP_ATTR_BUFFER_PROFILE;
-    attr.value.oid = m_originalPgBufferProfile;
-
-    // Set our zero buffer profile
-    status = sai_buffer_api->set_ingress_priority_group_attribute(pg, &attr);
-    if (status != SAI_STATUS_SUCCESS)
-    {
-        SWSS_LOG_ERROR("Failed to set buffer profile ID on queue 0x%" PRIx64 ": %d", getQueue(), status);
-        return;
-    }
-
-    setPriorityGroupAndQueueLockFlag(portInstance, false);
+    setQueueLockFlag(portInstance, false);
 }
 
-void PfcWdZeroBufferHandler::setPriorityGroupAndQueueLockFlag(Port& port, bool isLocked) const
+void PfcWdZeroBufferHandler::setQueueLockFlag(Port& port, bool isLocked) const
 {
-    // set lock bits on PG and queue
-    port.m_priority_group_lock[static_cast<size_t>(getQueueId())] = isLocked;
+    // set lock bits on queue
     for (size_t i = 0; i < port.m_queue_ids.size(); ++i)
     {
         if (port.m_queue_ids[i] == getQueue())
@@ -562,9 +519,8 @@ PfcWdZeroBufferHandler::ZeroBufferProfile::~ZeroBufferProfile(void)
 {
     SWSS_LOG_ENTER();
 
-    // Destory ingress and egress prifiles and pools
-    destroyZeroBufferProfile(true);
-    destroyZeroBufferProfile(false);
+    // Destory egress profiles and pools
+    destroyZeroBufferProfile();
 }
 
 PfcWdZeroBufferHandler::ZeroBufferProfile &PfcWdZeroBufferHandler::ZeroBufferProfile::getInstance(void)
@@ -576,19 +532,19 @@ PfcWdZeroBufferHandler::ZeroBufferProfile &PfcWdZeroBufferHandler::ZeroBufferPro
     return instance;
 }
 
-sai_object_id_t PfcWdZeroBufferHandler::ZeroBufferProfile::getZeroBufferProfile(bool ingress)
+sai_object_id_t PfcWdZeroBufferHandler::ZeroBufferProfile::getZeroBufferProfile(void)
 {
     SWSS_LOG_ENTER();
 
-    if (getInstance().getProfile(ingress) == SAI_NULL_OBJECT_ID)
+    if (getInstance().getProfile() == SAI_NULL_OBJECT_ID)
     {
-        getInstance().createZeroBufferProfile(ingress);
+        getInstance().createZeroBufferProfile();
     }
 
-    return getInstance().getProfile(ingress);
+    return getInstance().getProfile();
 }
 
-void PfcWdZeroBufferHandler::ZeroBufferProfile::createZeroBufferProfile(bool ingress)
+void PfcWdZeroBufferHandler::ZeroBufferProfile::createZeroBufferProfile(void)
 {
     SWSS_LOG_ENTER();
 
@@ -601,15 +557,15 @@ void PfcWdZeroBufferHandler::ZeroBufferProfile::createZeroBufferProfile(bool ing
     attribs.push_back(attr);
 
     attr.id = SAI_BUFFER_POOL_ATTR_TYPE;
-    attr.value.u32 = ingress ? SAI_BUFFER_POOL_TYPE_INGRESS : SAI_BUFFER_POOL_TYPE_EGRESS;
+    attr.value.u32 = SAI_BUFFER_POOL_TYPE_EGRESS;
     attribs.push_back(attr);
 
     attr.id = SAI_BUFFER_POOL_ATTR_THRESHOLD_MODE;
     attr.value.u32 = SAI_BUFFER_POOL_THRESHOLD_MODE_DYNAMIC;
     attribs.push_back(attr);
 
     sai_status_t status = sai_buffer_api->create_buffer_pool(
-            &getPool(ingress),
+            &getPool(),
             gSwitchId,
             static_cast<uint32_t>(attribs.size()),
             attribs.data());
@@ -623,7 +579,7 @@ void PfcWdZeroBufferHandler::ZeroBufferProfile::createZeroBufferProfile(bool ing
     attribs.clear();
 
     attr.id = SAI_BUFFER_PROFILE_ATTR_POOL_ID;
-    attr.value.oid = getPool(ingress);
+    attr.value.oid = getPool();
     attribs.push_back(attr);
 
     attr.id = SAI_BUFFER_PROFILE_ATTR_THRESHOLD_MODE;
@@ -639,7 +595,7 @@ void PfcWdZeroBufferHandler::ZeroBufferProfile::createZeroBufferProfile(bool ing
     attribs.push_back(attr);
 
     status = sai_buffer_api->create_buffer_profile(
-            &getProfile(ingress),
+            &getProfile(),
             gSwitchId,
             static_cast<uint32_t>(attribs.size()),
             attribs.data());
@@ -650,18 +606,18 @@ void PfcWdZeroBufferHandler::ZeroBufferProfile::createZeroBufferProfile(bool ing
     }
 }
 
-void PfcWdZeroBufferHandler::ZeroBufferProfile::destroyZeroBufferProfile(bool ingress)
+void PfcWdZeroBufferHandler::ZeroBufferProfile::destroyZeroBufferProfile(void)
 {
     SWSS_LOG_ENTER();
 
-    sai_status_t status = sai_buffer_api->remove_buffer_profile(getProfile(ingress));
+    sai_status_t status = sai_buffer_api->remove_buffer_profile(getProfile());
     if (status != SAI_STATUS_SUCCESS)
     {
         SWSS_LOG_ERROR("Failed to remove static zero buffer profile for PFC WD: %d", status);
         return;
     }
 
-    status = sai_buffer_api->remove_buffer_pool(getPool(ingress));
+    status = sai_buffer_api->remove_buffer_pool(getPool());
     if (status != SAI_STATUS_SUCCESS)
     {
         SWSS_LOG_ERROR("Failed to remove static zero buffer pool for PFC WD: %d", status);

orchagent/pfcactionhandler.h

@@ -124,42 +124,39 @@ class PfcWdZeroBufferHandler: public PfcWdLossyHandler
 
     private:
         /*
-         * Sets lock bits on port's priority group and queue
+         * Sets lock bits on port's queue
          * to protect them from beeing changed by other Orch's
          */
-        void setPriorityGroupAndQueueLockFlag(Port& port, bool isLocked) const;
+        void setQueueLockFlag(Port& port, bool isLocked) const;
 
         // Singletone class for keeping shared data - zero buffer profiles
         class ZeroBufferProfile
         {
             public:
                 ~ZeroBufferProfile(void);
-                static sai_object_id_t getZeroBufferProfile(bool ingress);
+                static sai_object_id_t getZeroBufferProfile(void);
 
             private:
                 ZeroBufferProfile(void);
                 static ZeroBufferProfile &getInstance(void);
-                void createZeroBufferProfile(bool ingress);
-                void destroyZeroBufferProfile(bool ingress);
+                void createZeroBufferProfile(void);
+                void destroyZeroBufferProfile(void);
 
-                sai_object_id_t& getProfile(bool ingress)
+                sai_object_id_t& getProfile(void)
                 {
-                    return ingress ? m_zeroIngressBufferProfile : m_zeroEgressBufferProfile;
+                    return m_zeroEgressBufferProfile;
                 }
 
-                sai_object_id_t& getPool(bool ingress)
+                sai_object_id_t& getPool(void)
                 {
-                    return ingress ? m_zeroIngressBufferPool : m_zeroEgressBufferPool;
+                    return m_zeroEgressBufferPool;
                 }
 
-                sai_object_id_t m_zeroIngressBufferPool = SAI_NULL_OBJECT_ID;
                 sai_object_id_t m_zeroEgressBufferPool = SAI_NULL_OBJECT_ID;
-                sai_object_id_t m_zeroIngressBufferProfile = SAI_NULL_OBJECT_ID;
                 sai_object_id_t m_zeroEgressBufferProfile = SAI_NULL_OBJECT_ID;
         };
 
         sai_object_id_t m_originalQueueBufferProfile = SAI_NULL_OBJECT_ID;
-        sai_object_id_t m_originalPgBufferProfile = SAI_NULL_OBJECT_ID;
 };
 
 #endif

orchagent/port.h

@@ -100,14 +100,13 @@ class Port
     uint32_t m_nat_zone_id = 0;
 
     /*
-     * Following two bit vectors are used to lock
-     * the PG/queue from being changed in BufferOrch.
+     * Following bit vector is used to lock
+     * the queue from being changed in BufferOrch.
      * The use case scenario is when PfcWdZeroBufferHandler
-     * sets zero buffer profile it should protect PG/queue
+     * sets zero buffer profile it should protect queue
      * from being overwritten in BufferOrch.
      */
     std::vector<bool> m_queue_lock;
-    std::vector<bool> m_priority_group_lock;
 
     bool m_fec_cfg = false;
     bool m_an_cfg = false;

orchagent/portsorch.cpp

@@ -2835,7 +2835,6 @@ void PortsOrch::initializePriorityGroups(Port &port)
     SWSS_LOG_INFO("Get %d priority groups for port %s", attr.value.u32, port.m_alias.c_str());
 
     port.m_priority_group_ids.resize(attr.value.u32);
-    port.m_priority_group_lock.resize(attr.value.u32);
 
     if (attr.value.u32 == 0)
     {

tests/mock_tests/portsorch_ut.cpp

@@ -318,7 +318,7 @@ namespace portsorch_test
     TEST_F(PortsOrchTest, PfcZeroBufferHandlerLocksPortPgAndQueue)
     {
         Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);
-        Table pgTable = Table(m_config_db.get(), CFG_BUFFER_PG_TABLE_NAME);
+        Table queueTable = Table(m_config_db.get(), CFG_BUFFER_QUEUE_TABLE_NAME);
         Table profileTable = Table(m_config_db.get(), CFG_BUFFER_PROFILE_TABLE_NAME);
         Table poolTable = Table(m_config_db.get(), CFG_BUFFER_POOL_TABLE_NAME);
 
@@ -390,15 +390,13 @@ namespace portsorch_test
         poolTable.set(
             "test_pool",
             {
-                { "type", "ingress" },
+                { "type", "egress" },
                 { "mode", "dynamic" },
                 { "size", "4200000" },
             });
 
         // Create test buffer profile
         profileTable.set("test_profile", { { "pool", "[BUFFER_POOL|test_pool]" },
-                                           { "xon", "14832" },
-                                           { "xoff", "14832" },
                                            { "size", "35000" },
                                            { "dynamic_th", "0" } });
 
@@ -407,29 +405,29 @@ namespace portsorch_test
         {
             std::ostringstream oss;
             oss << it.first << "|3-4";
-            pgTable.set(oss.str(), { { "profile", "[BUFFER_PROFILE|test_profile]" } });
+            queueTable.set(oss.str(), { { "profile", "[BUFFER_PROFILE|test_profile]" } });
         }
-        gBufferOrch->addExistingData(&pgTable);
+        gBufferOrch->addExistingData(&queueTable);
         gBufferOrch->addExistingData(&poolTable);
         gBufferOrch->addExistingData(&profileTable);
 
-        // process pool, profile and PGs
+        // process pool, profile and queues
         static_cast<Orch *>(gBufferOrch)->doTask();
 
-        auto pgConsumer = static_cast<Consumer*>(gBufferOrch->getExecutor(CFG_BUFFER_PG_TABLE_NAME));
-        pgConsumer->dumpPendingTasks(ts);
-        ASSERT_FALSE(ts.empty()); // PG is skipped
+        auto queueConsumer = static_cast<Consumer*>(gBufferOrch->getExecutor(CFG_BUFFER_QUEUE_TABLE_NAME));
+        queueConsumer->dumpPendingTasks(ts);
+        ASSERT_FALSE(ts.empty()); // Queue is skipped
         ts.clear();
 
         // release zero buffer drop handler
         dropHandler.reset();
 
-        // process PGs
+        // process queue
         static_cast<Orch *>(gBufferOrch)->doTask();
 
-        pgConsumer = static_cast<Consumer*>(gBufferOrch->getExecutor(CFG_BUFFER_PG_TABLE_NAME));
-        pgConsumer->dumpPendingTasks(ts);
-        ASSERT_TRUE(ts.empty()); // PG should be proceesed now
+        queueConsumer = static_cast<Consumer*>(gBufferOrch->getExecutor(CFG_BUFFER_QUEUE_TABLE_NAME));
+        queueConsumer->dumpPendingTasks(ts);
+        ASSERT_TRUE(ts.empty()); // Queue should be proceesed now
         ts.clear();
     }