Enhance dynamic buffer calculation and bug fixes (#1601)

* Enhancement and bug fixes for dynamic buffer calculation

What I did
- Remove make_pair when calling emplace_back.
- The pool size isn't recalculated when a port with headroom override but without speed or cable length configured being shutdown.
- Remove the current PG from the old referenced profile only if its name isn't empty, otherwise an entry with empty name will be inserted into m_bufferProfileLookup
- Don't try removing statically configured profiles with dynamic headroom from the APPL_DB because they were not programmed to APPL_DB.
- Setting a buffer PG to the same profile as it was causes the referenced dynamic profile unable to be removed once it isn't referenced any more

How I verified it
Run regression and vs test

Signed-off-by: Stephen Sun <[email protected]>

Author: stephenxs

cfgmgr/buffermgrdyn.cpp

@@ -384,7 +384,7 @@ void BufferMgrDynamic::checkSharedBufferPoolSize()
             if (m_firstTimeCalculateBufferPool)
             {
                 // It's something like a placeholder especially for warm reboot flow
-                // without all buffer pools created, buffer profiles are unable to be cureated,
+                // without all buffer pools created, buffer profiles are unable to be created,
                 // which in turn causes buffer pgs and buffer queues unable to be created,
                 // which prevents the port from being ready and eventually fails the warm reboot
                 // After the buffer pools are created for the first time, we won't touch it
@@ -435,14 +435,14 @@ void BufferMgrDynamic::updateBufferProfileToDb(const string &name, const buffer_
         m_applBufferProfileTable.getTableNameSeparator() +
         INGRESS_LOSSLESS_PG_POOL_NAME;
 
-    fvVector.emplace_back(make_pair("xon", profile.xon));
+    fvVector.emplace_back("xon", profile.xon);
     if (!profile.xon_offset.empty()) {
-        fvVector.emplace_back(make_pair("xon_offset", profile.xon_offset));
+        fvVector.emplace_back("xon_offset", profile.xon_offset);
     }
-    fvVector.emplace_back(make_pair("xoff", profile.xoff));
-    fvVector.emplace_back(make_pair("size", profile.size));
-    fvVector.emplace_back(make_pair("pool", "[" + pg_pool_reference + "]"));
-    fvVector.emplace_back(make_pair(mode, profile.threshold));
+    fvVector.emplace_back("xoff", profile.xoff);
+    fvVector.emplace_back("size", profile.size);
+    fvVector.emplace_back("pool", "[" + pg_pool_reference + "]");
+    fvVector.emplace_back(mode, profile.threshold);
 
     m_applBufferProfileTable.set(name, fvVector);
     m_stateBufferProfileTable.set(name, fvVector);
@@ -721,18 +721,20 @@ task_process_status BufferMgrDynamic::refreshPriorityGroupsForPort(const string
         {
             // Need to remove the reference to the old profile
             // and create the reference to the new one
-            m_bufferProfileLookup[oldProfile].port_pgs.erase(key);
             m_bufferProfileLookup[newProfile].port_pgs.insert(key);
             SWSS_LOG_INFO("Move profile reference for %s from [%s] to [%s]", key.c_str(), oldProfile.c_str(), newProfile.c_str());
 
-            // buffer pg needs to be updated as well
-            portPg.running_profile_name = newProfile;
-
             // Add the old profile to "to be removed" set
             if (!oldProfile.empty())
+            {
                 profilesToBeReleased.insert(oldProfile);
+                m_bufferProfileLookup[oldProfile].port_pgs.erase(key);
+            }
         }
 
+        // buffer pg needs to be updated as well
+        portPg.running_profile_name = newProfile;
+
         //appl_db Database operation: set item BUFFER_PG|<port>|<pg>
         updateBufferPgToDb(key, newProfile, true);
         isHeadroomUpdated = true;
@@ -811,12 +813,6 @@ task_process_status BufferMgrDynamic::doUpdatePgTask(const string &pg_key, const
         return task_process_status::task_success;
     }
 
-    if (bufferPg.static_configured && bufferPg.dynamic_calculated)
-    {
-        auto &profile = m_bufferProfileLookup[bufferPg.configured_profile_name];
-        profile.port_pgs.insert(pg_key);
-    }
-
     return task_process_status::task_success;
 }
 
@@ -843,12 +839,6 @@ task_process_status BufferMgrDynamic::doRemovePgTask(const string &pg_key, const
     SWSS_LOG_NOTICE("try removing the original profile %s", bufferPg.running_profile_name.c_str());
     releaseProfile(bufferPg.running_profile_name);
 
-    if (bufferPg.static_configured && bufferPg.dynamic_calculated)
-    {
-        auto &profile = m_bufferProfileLookup[bufferPg.configured_profile_name];
-        profile.port_pgs.erase(pg_key);
-    }
-
     return task_process_status::task_success;
 }
 
@@ -1099,15 +1089,18 @@ task_process_status BufferMgrDynamic::handlePortTable(KeyOpFieldsValuesTuple &tu
         string &mtu = portInfo.mtu;
         string &speed = portInfo.speed;
 
-        if (cable_length.empty() || speed.empty())
-        {
-            SWSS_LOG_WARN("Cable length for %s hasn't been configured yet, unable to calculate headroom", port.c_str());
-            // We don't retry here because it doesn't make sense until the cable length is configured.
-            return task_process_status::task_success;
-        }
-
         if (speed_updated || mtu_updated)
         {
+            if (cable_length.empty() || speed.empty())
+            {
+                // we still need to update pool size when port with headroom override is shut down
+                // even if its cable length or speed isn't configured
+                // so cable length and speed isn't tested for shutdown
+                SWSS_LOG_WARN("Cable length for %s hasn't been configured yet, unable to calculate headroom", port.c_str());
+                // We don't retry here because it doesn't make sense until the cable length is configured.
+                return task_process_status::task_success;
+            }
+
             SWSS_LOG_INFO("Updating BUFFER_PG for port %s due to speed or port updated", port.c_str());
 
             //Try updating the buffer information
@@ -1295,7 +1288,7 @@ task_process_status BufferMgrDynamic::handleBufferProfileTable(KeyOpFieldsValues
                     profileApp.ingress = true;
                 }
             }
-            fvVector.emplace_back(FieldValueTuple(field, value));
+            fvVector.emplace_back(field, value);
             SWSS_LOG_INFO("Inserting BUFFER_PROFILE table field %s value %s", field.c_str(), value.c_str());
         }
         // don't insert dynamically calculated profiles into APPL_DB
@@ -1353,13 +1346,20 @@ task_process_status BufferMgrDynamic::handleBufferProfileTable(KeyOpFieldsValues
                     return task_process_status::task_invalid_entry;
                 }
             }
-        }
 
-        m_applBufferProfileTable.del(profileName);
-        m_stateBufferProfileTable.del(profileName);
+            if (!profile.static_configured || !profile.dynamic_calculated)
+            {
+                m_applBufferProfileTable.del(profileName);
+                m_stateBufferProfileTable.del(profileName);
+            }
 
-        m_bufferProfileLookup.erase(profileName);
-        m_bufferProfileIgnored.erase(profileName);
+            m_bufferProfileLookup.erase(profileName);
+            m_bufferProfileIgnored.erase(profileName);
+        }
+        else
+        {
+            SWSS_LOG_ERROR("Profile %s not found while being removed", profileName.c_str());
+        }
     }
     else
     {
@@ -1387,6 +1387,12 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key,
 
         bufferPg.dynamic_calculated = true;
         bufferPg.static_configured = false;
+        if (!bufferPg.configured_profile_name.empty())
+        {
+            m_bufferProfileLookup[bufferPg.configured_profile_name].port_pgs.erase(key);
+            bufferPg.configured_profile_name = "";
+        }
+
         for (auto i = kfvFieldsValues(tuple).begin(); i != kfvFieldsValues(tuple).end(); i++)
         {
             const string &field = fvField(*i);
@@ -1418,7 +1424,7 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key,
                     }
                     else
                     {
-                        // In this case, we shouldn't set the dynamc calculated flat to true
+                        // In this case, we shouldn't set the dynamic calculated flag to true
                         // It will be updated when its profile configured.
                         bufferPg.dynamic_calculated = false;
                         SWSS_LOG_WARN("Profile %s hasn't been configured yet, skip", profileName.c_str());
@@ -1435,7 +1441,7 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key,
                 bufferPg.static_configured = true;
                 bufferPg.configured_profile_name = profileName;
             }
-            fvVector.emplace_back(FieldValueTuple(field, value));
+            fvVector.emplace_back(field, value);
             SWSS_LOG_INFO("Inserting BUFFER_PG table field %s value %s", field.c_str(), value.c_str());
         }
 
@@ -1449,6 +1455,11 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key,
         if (!ignored && bufferPg.lossless)
         {
             doUpdatePgTask(key, port);
+
+            if (!bufferPg.configured_profile_name.empty())
+            {
+                m_bufferProfileLookup[bufferPg.configured_profile_name].port_pgs.insert(key);
+            }
         }
         else
         {
@@ -1464,6 +1475,10 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key,
         string &profileName = bufferPg.running_profile_name;
 
         m_bufferProfileLookup[profileName].port_pgs.erase(key);
+        if (!bufferPg.configured_profile_name.empty())
+        {
+            m_bufferProfileLookup[bufferPg.configured_profile_name].port_pgs.erase(key);
+        }
 
         if (bufferPg.lossless)
         {
@@ -1589,7 +1604,7 @@ task_process_status BufferMgrDynamic::doBufferTableTask(KeyOpFieldsValuesTuple &
                 transformReference(fvValue(i));
             if (fvField(i) == "profile_list")
                 transformReference(fvValue(i));
-            fvVector.emplace_back(FieldValueTuple(fvField(i), fvValue(i)));
+            fvVector.emplace_back(fvField(i), fvValue(i));
             SWSS_LOG_INFO("Inserting field %s value %s", fvField(i).c_str(), fvValue(i).c_str());
         }
         applTable.set(key, fvVector);