Skip to content

Fix issue: sometimes PFC WD unable to create zero buffer pool #2164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
neethajohn merged 7 commits into from
Mar 16, 2022
Merged

Fix issue: sometimes PFC WD unable to create zero buffer pool #2164

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7f0fb4e
Fix issue: PFCWD failed to apply zero buffer profile
stephenxs Feb 21, 2022
b0d421f
Solution 2: pfcwd track zero pool and profile
stephenxs Feb 24, 2022
f75df4a
3rd solution. PFCWD and bufferOrch share the zero pool
stephenxs Feb 24, 2022
ce6606e
Fix review comments
stephenxs Mar 2, 2022
7382b31
Fix review comments
stephenxs Mar 8, 2022
64f66dd
Merge branch 'master' into fix-pfcwd
stephenxs Mar 10, 2022
e7dfad1
Test zero profile and zero pool before removing them
stephenxs Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 153 additions & 15 deletions orchagent/bufferorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,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();
Expand Down Expand Up @@ -310,6 +314,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)
{
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();
Expand All @@ -318,6 +381,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())
Expand All @@ -326,6 +391,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;
Expand Down Expand Up @@ -372,6 +448,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)
Expand Down Expand Up @@ -437,18 +518,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
Expand All @@ -470,18 +586,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);
Expand Down
13 changes: 13 additions & 0 deletions orchagent/bufferorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,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);
Expand Down Expand Up @@ -71,6 +79,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 */

86 changes: 58 additions & 28 deletions orchagent/pfcactionhandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "logger.h"
#include "sai_serialize.h"
#include "portsorch.h"
#include "bufferorch.h"
#include <vector>
#include <inttypes.h>

Expand All @@ -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;
Expand Down Expand Up @@ -715,6 +717,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();
Expand All @@ -733,29 +754,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);

sai_status_t status = sai_buffer_api->create_buffer_pool(
&getPool(ingress),
attr.id = SAI_BUFFER_POOL_ATTR_THRESHOLD_MODE;
attr.value.u32 = SAI_BUFFER_POOL_THRESHOLD_MODE_STATIC;
attribs.push_back(attr);

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
Expand All @@ -766,15 +797,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(
Expand All @@ -793,16 +824,15 @@ 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 (sai_buffer_api != NULL)
{
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)
{
SWSS_LOG_ERROR("Failed to remove static zero buffer pool for PFC WD: %d", status);
gBufferOrch->unlockZeroBufferPool(ingress);
}
}
Loading