[202012] Fix issue: sometimes PFC WD unable to create zero buffer pool

orchagent/bufferorch.cpp

@@ -52,7 +52,11 @@ BufferOrch::BufferOrch(DBConnector *applDb, DBConnector *confDb, DBConnector *st
     m_flexCounterTable(new ProducerTable(m_flexCounterDb.get(), FLEX_COUNTER_TABLE)),
     m_flexCounterGroupTable(new ProducerTable(m_flexCounterDb.get(), FLEX_COUNTER_GROUP_TABLE)),
     m_countersDb(new DBConnector("COUNTERS_DB", 0)),
-    m_stateBufferMaximumValueTable(stateDb, STATE_BUFFER_MAXIMUM_VALUE_TABLE)
+    m_stateBufferMaximumValueTable(stateDb, STATE_BUFFER_MAXIMUM_VALUE_TABLE),
+    m_ingressZeroBufferPool(SAI_NULL_OBJECT_ID),
+    m_egressZeroBufferPool(SAI_NULL_OBJECT_ID),
+    m_ingressZeroPoolRefCount(0),
+    m_egressZeroPoolRefCount(0)
 {
     SWSS_LOG_ENTER();
     initTableHandlers();
@@ -351,6 +355,65 @@ const object_reference_map &BufferOrch::getBufferPoolNameOidMap(void)
     return *m_buffer_type_maps[APP_BUFFER_POOL_TABLE_NAME];
 }
 
+void BufferOrch::lockZeroBufferPool(bool ingress)
+{
+    if (ingress)
+        m_ingressZeroPoolRefCount++;
+    else
+        m_egressZeroPoolRefCount++;
+}
+
+void BufferOrch::unlockZeroBufferPool(bool ingress)
+{
+    sai_object_id_t pool = SAI_NULL_OBJECT_ID;
+    if (ingress)
+    {
+        if (--m_ingressZeroPoolRefCount <= 0)
+        {
+            pool = m_ingressZeroBufferPool;
+            m_ingressZeroBufferPool = SAI_NULL_OBJECT_ID;
+        }
+    }
+    else
+    {
+        if (--m_egressZeroPoolRefCount <= 0)
+        {
+            pool = m_egressZeroBufferPool;
+            m_egressZeroBufferPool = SAI_NULL_OBJECT_ID;
+        }
+    }
+
+    if (pool != SAI_NULL_OBJECT_ID)
+    {
+        auto sai_status = sai_buffer_api->remove_buffer_pool(pool);
+        if (SAI_STATUS_SUCCESS != sai_status)
+        {
+            SWSS_LOG_ERROR("Failed to remove buffer pool, rv:%d", sai_status);
+            task_process_status handle_status = handleSaiRemoveStatus(SAI_API_BUFFER, sai_status);
+            if (handle_status != task_process_status::task_success)
+            {
+                return;
+            }
+        }
+        else
+        {
+            SWSS_LOG_NOTICE("Zero buffer pool has been successfully removed");
+        }
+    }
+}
+
+void BufferOrch::setZeroBufferPool(bool ingress, sai_object_id_t pool)
+{
+    if (ingress)
+    {
+        m_ingressZeroBufferPool = pool;
+    }
+    else
+    {
+        m_egressZeroBufferPool = pool;
+    }
+}
+
 task_process_status BufferOrch::processBufferPool(KeyOpFieldsValuesTuple &tuple)
 {
     SWSS_LOG_ENTER();
@@ -359,6 +422,8 @@ task_process_status BufferOrch::processBufferPool(KeyOpFieldsValuesTuple &tuple)
     string map_type_name = APP_BUFFER_POOL_TABLE_NAME;
     string object_name = kfvKey(tuple);
     string op = kfvOp(tuple);
+    sai_buffer_pool_type_t pool_direction = SAI_BUFFER_POOL_TYPE_INGRESS;
+    bool creating_zero_pool = false;
 
     SWSS_LOG_DEBUG("object name:%s", object_name.c_str());
     if (m_buffer_type_maps[map_type_name]->find(object_name) != m_buffer_type_maps[map_type_name]->end())
@@ -367,6 +432,17 @@ task_process_status BufferOrch::processBufferPool(KeyOpFieldsValuesTuple &tuple)
         SWSS_LOG_DEBUG("found existing object:%s of type:%s", object_name.c_str(), map_type_name.c_str());
     }
     SWSS_LOG_DEBUG("processing command:%s", op.c_str());
+    if (object_name == "ingress_zero_pool")
+    {
+        creating_zero_pool = true;
+        pool_direction = SAI_BUFFER_POOL_TYPE_INGRESS;
+    }
+    else if (object_name == "egress_zero_pool")
+    {
+        creating_zero_pool = true;
+        pool_direction = SAI_BUFFER_POOL_TYPE_EGRESS;
+    }
+
     if (op == SET_COMMAND)
     {
         vector<sai_attribute_t> attribs;
@@ -409,6 +485,11 @@ task_process_status BufferOrch::processBufferPool(KeyOpFieldsValuesTuple &tuple)
                     return task_process_status::task_invalid_entry;
                 }
                 attr.id = SAI_BUFFER_POOL_ATTR_TYPE;
+                if (creating_zero_pool && pool_direction != static_cast<sai_buffer_pool_type_t>(attr.value.u32))
+                {
+                    SWSS_LOG_ERROR("Wrong pool direction for pool %s", object_name.c_str());
+                    return task_process_status::task_invalid_entry;
+                }
                 attribs.push_back(attr);
             }
             else if (field == buffer_pool_mode_field_name)
@@ -474,18 +555,53 @@ task_process_status BufferOrch::processBufferPool(KeyOpFieldsValuesTuple &tuple)
         }
         else
         {
-            sai_status = sai_buffer_api->create_buffer_pool(&sai_object, gSwitchId, (uint32_t)attribs.size(), attribs.data());
-            if (SAI_STATUS_SUCCESS != sai_status)
+            if (creating_zero_pool)
             {
-                SWSS_LOG_ERROR("Failed to create buffer pool %s with type %s, rv:%d", object_name.c_str(), map_type_name.c_str(), sai_status);
-                task_process_status handle_status = handleSaiCreateStatus(SAI_API_BUFFER, sai_status);
-                if (handle_status != task_process_status::task_success)
+                if (pool_direction == SAI_BUFFER_POOL_TYPE_INGRESS)
                 {
-                    return handle_status;
+                    sai_object = m_ingressZeroBufferPool;
+                }
+                else if (pool_direction == SAI_BUFFER_POOL_TYPE_EGRESS)
+                {
+                    sai_object = m_egressZeroBufferPool;
+                }
+            }
+
+            if (SAI_NULL_OBJECT_ID == sai_object)
+            {
+                sai_status = sai_buffer_api->create_buffer_pool(&sai_object, gSwitchId, (uint32_t)attribs.size(), attribs.data());
+                if (SAI_STATUS_SUCCESS != sai_status)
+                {
+                    SWSS_LOG_ERROR("Failed to create buffer pool %s with type %s, rv:%d", object_name.c_str(), map_type_name.c_str(), sai_status);
+                    task_process_status handle_status = handleSaiCreateStatus(SAI_API_BUFFER, sai_status);
+                    if (handle_status != task_process_status::task_success)
+                    {
+                        return handle_status;
+                    }
+                }
+
+                SWSS_LOG_NOTICE("Created buffer pool %s with type %s", object_name.c_str(), map_type_name.c_str());
+            }
+            else
+            {
+                SWSS_LOG_NOTICE("No need to create buffer pool %s since it has been created", object_name.c_str());
+            }
+
+            if (creating_zero_pool)
+            {
+                if (pool_direction == SAI_BUFFER_POOL_TYPE_INGRESS)
+                {
+                    m_ingressZeroPoolRefCount++;
+                    m_ingressZeroBufferPool = sai_object;
+                }
+                else if (pool_direction == SAI_BUFFER_POOL_TYPE_EGRESS)
+                {
+                    m_egressZeroPoolRefCount++;
+                    m_egressZeroBufferPool = sai_object;
                 }
             }
+
             (*(m_buffer_type_maps[map_type_name]))[object_name].m_saiObjectId = sai_object;
-            SWSS_LOG_NOTICE("Created buffer pool %s with type %s", object_name.c_str(), map_type_name.c_str());
             // Here we take the PFC watchdog approach to update the COUNTERS_DB metadata (e.g., PFC_WD_DETECTION_TIME per queue)
             // at initialization (creation and registration phase)
             // Specifically, we push the buffer pool name to oid mapping upon the creation of the oid
@@ -507,18 +623,40 @@ task_process_status BufferOrch::processBufferPool(KeyOpFieldsValuesTuple &tuple)
         if (SAI_NULL_OBJECT_ID != sai_object)
         {
             clearBufferPoolWatermarkCounterIdList(sai_object);
-            sai_status = sai_buffer_api->remove_buffer_pool(sai_object);
-            if (SAI_STATUS_SUCCESS != sai_status)
+            bool remove = true;
+            if (sai_object == m_ingressZeroBufferPool)
             {
-                SWSS_LOG_ERROR("Failed to remove buffer pool %s with type %s, rv:%d", object_name.c_str(), map_type_name.c_str(), sai_status);
-                task_process_status handle_status = handleSaiRemoveStatus(SAI_API_BUFFER, sai_status);
-                if (handle_status != task_process_status::task_success)
+                if (--m_ingressZeroPoolRefCount > 0)
+                    remove = false;
+                else
+                    m_ingressZeroBufferPool = SAI_NULL_OBJECT_ID;
+            }
+            else if (sai_object == m_egressZeroBufferPool)
+            {
+                if (--m_egressZeroPoolRefCount > 0)
+                    remove = false;
+                else
+                    m_egressZeroBufferPool = SAI_NULL_OBJECT_ID;
+            }
+            if (remove)
+            {
+                sai_status = sai_buffer_api->remove_buffer_pool(sai_object);
+                if (SAI_STATUS_SUCCESS != sai_status)
                 {
-                    return handle_status;
+                    SWSS_LOG_ERROR("Failed to remove buffer pool %s with type %s, rv:%d", object_name.c_str(), map_type_name.c_str(), sai_status);
+                    task_process_status handle_status = handleSaiRemoveStatus(SAI_API_BUFFER, sai_status);
+                    if (handle_status != task_process_status::task_success)
+                    {
+                        return handle_status;
+                    }
                 }
+                SWSS_LOG_NOTICE("Removed buffer pool %s with type %s", object_name.c_str(), map_type_name.c_str());
+            }
+            else
+            {
+                SWSS_LOG_NOTICE("Will not remove buffer pool %s since it is still referenced", object_name.c_str());
             }
         }
-        SWSS_LOG_NOTICE("Removed buffer pool %s with type %s", object_name.c_str(), map_type_name.c_str());
         auto it_to_delete = (m_buffer_type_maps[map_type_name])->find(object_name);
         (m_buffer_type_maps[map_type_name])->erase(it_to_delete);
         m_countersDb->hdel(COUNTERS_BUFFER_POOL_NAME_MAP, object_name);

orchagent/bufferorch.h

@@ -36,6 +36,14 @@ class BufferOrch : public Orch
     static type_map m_buffer_type_maps;
     void generateBufferPoolWatermarkCounterIdList(void);
     const object_reference_map &getBufferPoolNameOidMap(void);
+    sai_object_id_t getZeroBufferPool(bool ingress)
+    {
+        return ingress ? m_ingressZeroBufferPool : m_egressZeroBufferPool;
+    }
+
+    void lockZeroBufferPool(bool ingress);
+    void unlockZeroBufferPool(bool ingress);
+    void setZeroBufferPool(bool direction, sai_object_id_t pool);
 
 private:
     typedef task_process_status (BufferOrch::*buffer_table_handler)(KeyOpFieldsValuesTuple &tuple);
@@ -70,6 +78,11 @@ class BufferOrch : public Orch
     unique_ptr<DBConnector> m_countersDb;
 
     bool m_isBufferPoolWatermarkCounterIdListGenerated = false;
+
+    sai_object_id_t m_ingressZeroBufferPool;
+    sai_object_id_t m_egressZeroBufferPool;
+    int m_ingressZeroPoolRefCount;
+    int m_egressZeroPoolRefCount;
 };
 #endif /* SWSS_BUFFORCH_H */
 

orchagent/pfcactionhandler.cpp

@@ -3,6 +3,7 @@
 #include "logger.h"
 #include "sai_serialize.h"
 #include "portsorch.h"
+#include "bufferorch.h"
 #include <vector>
 #include <inttypes.h>
 
@@ -26,6 +27,7 @@
 extern sai_object_id_t gSwitchId;
 extern PortsOrch *gPortsOrch;
 extern AclOrch * gAclOrch;
+extern BufferOrch *gBufferOrch;
 extern sai_port_api_t *sai_port_api;
 extern sai_queue_api_t *sai_queue_api;
 extern sai_buffer_api_t *sai_buffer_api;
@@ -712,6 +714,25 @@ PfcWdZeroBufferHandler::ZeroBufferProfile &PfcWdZeroBufferHandler::ZeroBufferPro
     return instance;
 }
 
+sai_object_id_t& PfcWdZeroBufferHandler::ZeroBufferProfile::getPool(bool ingress)
+{
+    // If there is a cached zero buffer pool, just use it
+    // else fetch zero buffer pool from buffer orch
+    // If there is one, use it and increase the reference number.
+    // otherwise, just return NULL OID
+    // PfcWdZeroBufferHandler will create it later and notify buffer orch later
+    auto &poolId = ingress ? m_zeroIngressBufferPool : m_zeroEgressBufferPool;
+    if (poolId == SAI_NULL_OBJECT_ID)
+    {
+        poolId = gBufferOrch->getZeroBufferPool(ingress);
+        if (poolId != SAI_NULL_OBJECT_ID)
+        {
+            gBufferOrch->lockZeroBufferPool(ingress);
+        }
+    }
+    return poolId;
+}
+
 sai_object_id_t PfcWdZeroBufferHandler::ZeroBufferProfile::getZeroBufferProfile(bool ingress)
 {
     SWSS_LOG_ENTER();
@@ -730,29 +751,39 @@ void PfcWdZeroBufferHandler::ZeroBufferProfile::createZeroBufferProfile(bool ing
 
     sai_attribute_t attr;
     vector<sai_attribute_t> attribs;
+    sai_status_t status;
 
-    // Create zero pool
-    attr.id = SAI_BUFFER_POOL_ATTR_SIZE;
-    attr.value.u64 = 0;
-    attribs.push_back(attr);
+    auto &poolId = getPool(ingress);
 
-    attr.id = SAI_BUFFER_POOL_ATTR_TYPE;
-    attr.value.u32 = ingress ? SAI_BUFFER_POOL_TYPE_INGRESS : SAI_BUFFER_POOL_TYPE_EGRESS;
-    attribs.push_back(attr);
+    if (SAI_NULL_OBJECT_ID == poolId)
+    {
+        // Create zero pool
+        attr.id = SAI_BUFFER_POOL_ATTR_SIZE;
+        attr.value.u64 = 0;
+        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);
+        attr.id = SAI_BUFFER_POOL_ATTR_TYPE;
+        attr.value.u32 = ingress ? SAI_BUFFER_POOL_TYPE_INGRESS : 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_STATIC;
+        attribs.push_back(attr);
 
-    sai_status_t status = sai_buffer_api->create_buffer_pool(
-            &getPool(ingress),
+        status = sai_buffer_api->create_buffer_pool(
+            &poolId,
             gSwitchId,
             static_cast<uint32_t>(attribs.size()),
             attribs.data());
-    if (status != SAI_STATUS_SUCCESS)
-    {
-        SWSS_LOG_ERROR("Failed to create dynamic zero buffer pool for PFC WD: %d", status);
-        return;
+        if (status != SAI_STATUS_SUCCESS)
+        {
+            SWSS_LOG_ERROR("Failed to create dynamic zero buffer pool for PFC WD: %d", status);
+            return;
+        }
+
+        // Pass the ownership to BufferOrch
+        gBufferOrch->setZeroBufferPool(ingress, poolId);
+        gBufferOrch->lockZeroBufferPool(ingress);
     }
 
     // Create zero profile
@@ -763,15 +794,15 @@ void PfcWdZeroBufferHandler::ZeroBufferProfile::createZeroBufferProfile(bool ing
     attribs.push_back(attr);
 
     attr.id = SAI_BUFFER_PROFILE_ATTR_THRESHOLD_MODE;
-    attr.value.u32 = SAI_BUFFER_PROFILE_THRESHOLD_MODE_DYNAMIC;
+    attr.value.u32 = SAI_BUFFER_PROFILE_THRESHOLD_MODE_STATIC;
     attribs.push_back(attr);
 
     attr.id = SAI_BUFFER_PROFILE_ATTR_BUFFER_SIZE;
     attr.value.u64 = 0;
     attribs.push_back(attr);
 
-    attr.id = SAI_BUFFER_PROFILE_ATTR_SHARED_DYNAMIC_TH;
-    attr.value.s8 = -8; // ALPHA_0
+    attr.id = SAI_BUFFER_PROFILE_ATTR_SHARED_STATIC_TH;
+    attr.value.s8 = 0;
     attribs.push_back(attr);
 
     status = sai_buffer_api->create_buffer_profile(
@@ -790,16 +821,19 @@ void PfcWdZeroBufferHandler::ZeroBufferProfile::destroyZeroBufferProfile(bool in
 {
     SWSS_LOG_ENTER();
 
-    sai_status_t status = sai_buffer_api->remove_buffer_profile(getProfile(ingress));
-    if (status != SAI_STATUS_SUCCESS)
+    if (getProfile(ingress) != SAI_NULL_OBJECT_ID)
     {
-        SWSS_LOG_ERROR("Failed to remove static zero buffer profile for PFC WD: %d", status);
-        return;
+        sai_status_t status = sai_buffer_api->remove_buffer_profile(getProfile(ingress));
+        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));
-    if (status != SAI_STATUS_SUCCESS)
+    auto &pool = ingress ? m_zeroIngressBufferPool : m_zeroEgressBufferPool;
+    if (pool != SAI_NULL_OBJECT_ID)
     {
-        SWSS_LOG_ERROR("Failed to remove static zero buffer pool for PFC WD: %d", status);
+        gBufferOrch->unlockZeroBufferPool(ingress);
     }
 }