Skip to content

Commit

Permalink
Enhance dynamic buffer calculation and bug fixes (sonic-net#1601)
Browse files Browse the repository at this point in the history 
* 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 <stephens@nvidia.com>
  • Loading branch information
@stephenxs
stephenxs authored Jan 22, 2021
1 parent e800c9f commit f4c3579
Showing 1 changed file with 54 additions and 39 deletions.
93 changes: 54 additions & 39 deletions cfgmgr/buffermgrdyn.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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());
Expand All @@ -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());
}

Expand All @@ -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
{
Expand All @@ -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)
{
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit f4c3579

Please sign in to comment.