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

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();
     }