Skip to content

Commit

Permalink
[Dynamic Buffer Calc] Enhancement: coding style and LGTM alerts (soni…
Browse files Browse the repository at this point in the history 
…c-net#1693)

* Improve coding styles

- Add one space between "//" and the comments
- Use capital letter at the beginning of sentence
- refreshPriorityGroupsForPort => refreshPgsForPort

* Fix LGTM alerts

Signed-off-by: Stephen Sun <[email protected]>
  • Loading branch information
@stephenxs @raphaelt-nvidia
stephenxs authored and raphaelt-nvidia committed Oct 5, 2021
1 parent db779c8 commit e9fcc4a
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 38 deletions.
52 changes: 26 additions & 26 deletions cfgmgr/buffermgrdyn.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -659,9 +659,9 @@ void BufferMgrDynamic::releaseProfile(const string &profile_name)

bool BufferMgrDynamic::isHeadroomResourceValid(const string &port, const buffer_profile_t &profile, const string &new_pg = "")
{
//port: used to fetch the maximum headroom size
//profile: the profile referenced by the new_pg (if provided) or all PGs
//new_pg: which pg is newly added?
// port: used to fetch the maximum headroom size
// profile: the profile referenced by the new_pg (if provided) or all PGs
// new_pg: which pg is newly added?

if (!profile.lossless)
{
Expand Down Expand Up @@ -786,7 +786,7 @@ task_process_status BufferMgrDynamic::removeAllPgsFromPort(const string &port)
// 2. Update port's info: speed, cable length and mtu
// 3. If any of the PGs is updated, recalculate pool size
// 4. try release each profile in to-be-released profile set
task_process_status BufferMgrDynamic::refreshPriorityGroupsForPort(const string &port, const string &speed, const string &cable_length, const string &mtu, const string &exactly_matched_key = "")
task_process_status BufferMgrDynamic::refreshPgsForPort(const string &port, const string &speed, const string &cable_length, const string &mtu, const string &exactly_matched_key = "")
{
port_info_t &portInfo = m_portInfoLookup[port];
string &gearbox_model = portInfo.gearbox_model;
Expand Down Expand Up @@ -1025,7 +1025,7 @@ task_process_status BufferMgrDynamic::doUpdatePgTask(const string &pg_key, const
// Not having profile_name but both speed and cable length have been configured for that port
// This is because the first PG on that port is configured after speed, cable length configured
// Just regenerate the profile
task_status = refreshPriorityGroupsForPort(port, portInfo.speed, portInfo.cable_length, portInfo.mtu, pg_key);
task_status = refreshPgsForPort(port, portInfo.speed, portInfo.cable_length, portInfo.mtu, pg_key);
if (task_status != task_process_status::task_success)
return task_status;

Expand All @@ -1038,7 +1038,7 @@ task_process_status BufferMgrDynamic::doUpdatePgTask(const string &pg_key, const
}
else
{
task_status = refreshPriorityGroupsForPort(port, portInfo.speed, portInfo.cable_length, portInfo.mtu, pg_key);
task_status = refreshPgsForPort(port, portInfo.speed, portInfo.cable_length, portInfo.mtu, pg_key);
if (task_status != task_process_status::task_success)
return task_status;
}
Expand All @@ -1059,7 +1059,7 @@ task_process_status BufferMgrDynamic::doUpdatePgTask(const string &pg_key, const
return task_process_status::task_success;
}

//Remove the currently configured lossless pg
// Remove the currently configured lossless pg
task_process_status BufferMgrDynamic::doRemovePgTask(const string &pg_key, const string &port)
{
auto &bufferPgs = m_portPgLookup[port];
Expand Down Expand Up @@ -1114,7 +1114,7 @@ task_process_status BufferMgrDynamic::doUpdateBufferProfileForDynamicTh(buffer_p
SWSS_LOG_DEBUG("Checking PG %s for dynamic profile %s", key.c_str(), profileName.c_str());
portsChecked.insert(portName);

rc = refreshPriorityGroupsForPort(portName, port.speed, port.cable_length, port.mtu);
rc = refreshPgsForPort(portName, port.speed, port.cable_length, port.mtu);
if (task_process_status::task_success != rc)
{
SWSS_LOG_ERROR("Update the profile on %s failed", key.c_str());
Expand Down Expand Up @@ -1284,16 +1284,16 @@ task_process_status BufferMgrDynamic::handleCableLenTable(KeyOpFieldsValuesTuple

SWSS_LOG_INFO("Updating BUFFER_PG for port %s due to cable length updated", port.c_str());

//Try updating the buffer information
// Try updating the buffer information
switch (portInfo.state)
{
case PORT_INITIALIZING:
portInfo.state = PORT_READY;
task_status = refreshPriorityGroupsForPort(port, speed, cable_length, mtu);
task_status = refreshPgsForPort(port, speed, cable_length, mtu);
break;

case PORT_READY:
task_status = refreshPriorityGroupsForPort(port, speed, cable_length, mtu);
task_status = refreshPgsForPort(port, speed, cable_length, mtu);
break;

case PORT_ADMIN_DOWN:
Expand Down Expand Up @@ -1346,7 +1346,7 @@ task_process_status BufferMgrDynamic::handlePortTable(KeyOpFieldsValuesTuple &tu
string op = kfvOp(tuple);
bool speed_updated = false, mtu_updated = false, admin_status_updated = false, admin_up;

SWSS_LOG_DEBUG("processing command:%s PORT table key %s", op.c_str(), port.c_str());
SWSS_LOG_DEBUG("Processing command:%s PORT table key %s", op.c_str(), port.c_str());

port_info_t &portInfo = m_portInfoLookup[port];

Expand Down Expand Up @@ -1449,7 +1449,6 @@ task_process_status BufferMgrDynamic::handlePortTable(KeyOpFieldsValuesTuple &tu
SWSS_LOG_WARN("Cable length or speed 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 both cable length and speed are configured.
}

}

if (admin_status_updated)
Expand All @@ -1470,7 +1469,8 @@ task_process_status BufferMgrDynamic::handlePortTable(KeyOpFieldsValuesTuple &tu
need_remove_all_pgs = true;
}

SWSS_LOG_INFO("Recalculate shared buffer pool size due to port %s's admin_status updated", port.c_str());
SWSS_LOG_INFO("Recalculate shared buffer pool size due to port %s's admin_status updated to %s",
port.c_str(), (admin_up ? "up" : "down"));
}

// In case both need_remove_all_pgs and need_refresh_all_pgs are true, the need_remove_all_pgs will take effect.
Expand All @@ -1482,7 +1482,7 @@ task_process_status BufferMgrDynamic::handlePortTable(KeyOpFieldsValuesTuple &tu
}
else if (need_refresh_all_pgs)
{
task_status = refreshPriorityGroupsForPort(port, portInfo.speed, portInfo.cable_length, portInfo.mtu);
task_status = refreshPgsForPort(port, portInfo.speed, portInfo.cable_length, portInfo.mtu);
}
}

Expand All @@ -1496,7 +1496,7 @@ task_process_status BufferMgrDynamic::handleBufferPoolTable(KeyOpFieldsValuesTup
string op = kfvOp(tuple);
vector<FieldValueTuple> fvVector;

SWSS_LOG_DEBUG("processing command:%s table BUFFER_POOL key %s", op.c_str(), pool.c_str());
SWSS_LOG_DEBUG("Processing command:%s table BUFFER_POOL key %s", op.c_str(), pool.c_str());
if (op == SET_COMMAND)
{
// For set command:
Expand All @@ -1511,7 +1511,7 @@ task_process_status BufferMgrDynamic::handleBufferPoolTable(KeyOpFieldsValuesTup
string &field = fvField(*i);
string &value = fvValue(*i);

SWSS_LOG_DEBUG("field:%s, value:%s", field.c_str(), value.c_str());
SWSS_LOG_DEBUG("Field:%s, value:%s", field.c_str(), value.c_str());
if (field == buffer_size_field_name)
{
bufferPool.dynamic_size = false;
Expand Down Expand Up @@ -1595,12 +1595,12 @@ task_process_status BufferMgrDynamic::handleBufferProfileTable(KeyOpFieldsValues
string op = kfvOp(tuple);
vector<FieldValueTuple> fvVector;

SWSS_LOG_DEBUG("processing command:%s BUFFER_PROFILE table key %s", op.c_str(), profileName.c_str());
SWSS_LOG_DEBUG("Processing command:%s BUFFER_PROFILE table key %s", op.c_str(), profileName.c_str());
if (op == SET_COMMAND)
{
//For set command:
//1. Create the corresponding table entries in APPL_DB
//2. Record the table in the internal cache m_bufferProfileLookup
// For set command:
// 1. Create the corresponding table entries in APPL_DB
// 2. Record the table in the internal cache m_bufferProfileLookup
buffer_profile_t &profileApp = m_bufferProfileLookup[profileName];

profileApp.static_configured = true;
Expand All @@ -1615,7 +1615,7 @@ task_process_status BufferMgrDynamic::handleBufferProfileTable(KeyOpFieldsValues
const string &field = fvField(*i);
string value = fvValue(*i);

SWSS_LOG_DEBUG("field:%s, value:%s", field.c_str(), value.c_str());
SWSS_LOG_DEBUG("Field:%s, value:%s", field.c_str(), value.c_str());
if (field == buffer_pool_field_name)
{
if (!value.empty())
Expand Down Expand Up @@ -1765,7 +1765,7 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key,
vector<FieldValueTuple> fvVector;
buffer_pg_t &bufferPg = m_portPgLookup[port][key];

SWSS_LOG_DEBUG("processing command:%s table BUFFER_PG key %s", op.c_str(), key.c_str());
SWSS_LOG_DEBUG("Processing command:%s table BUFFER_PG key %s", op.c_str(), key.c_str());
if (op == SET_COMMAND)
{
bool ignored = false;
Expand All @@ -1789,7 +1789,7 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key,
const string &field = fvField(*i);
string value = fvValue(*i);

SWSS_LOG_DEBUG("field:%s, value:%s", field.c_str(), value.c_str());
SWSS_LOG_DEBUG("Field:%s, value:%s", field.c_str(), value.c_str());
if (field == buffer_profile_field_name && value != "NULL")
{
// Headroom override
Expand Down Expand Up @@ -1988,7 +1988,7 @@ task_process_status BufferMgrDynamic::doBufferTableTask(KeyOpFieldsValuesTuple &
string key = kfvKey(tuple);
const string &name = applTable.getTableName();

//transform the separator in key from "|" to ":"
// Transform the separator in key from "|" to ":"
transformSeperator(key);

string op = kfvOp(tuple);
Expand All @@ -2000,7 +2000,7 @@ task_process_status BufferMgrDynamic::doBufferTableTask(KeyOpFieldsValuesTuple &

for (auto i : kfvFieldsValues(tuple))
{
//transform the separator in values from "|" to ":"
// Transform the separator in values from "|" to ":"
if (fvField(i) == "pool")
transformReference(fvValue(i));
if (fvField(i) == "profile")
Expand Down
2 changes: 1 addition & 1 deletion cfgmgr/buffermgrdyn.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ class BufferMgrDynamic : public Orch

// Main flows
task_process_status removeAllPgsFromPort(const std::string &port);
task_process_status refreshPriorityGroupsForPort(const std::string &port, const std::string &speed, const std::string &cable_length, const std::string &mtu, const std::string &exactly_matched_key);
task_process_status refreshPgsForPort(const std::string &port, const std::string &speed, const std::string &cable_length, const std::string &mtu, const std::string &exactly_matched_key);
task_process_status doUpdatePgTask(const std::string &pg_key, const std::string &port);
task_process_status doRemovePgTask(const std::string &pg_key, const std::string &port);
task_process_status doAdminStatusTask(const std::string port, const std::string adminStatus);
Expand Down
4 changes: 0 additions & 4 deletions tests/test_buffer_dynamic.py
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,8 @@
import time
import json
import redis
import pytest
import re
import buffer_model

from pprint import pprint
from swsscommon import swsscommon
from dvslib.dvs_common import PollingConfig

@pytest.yield_fixture
Expand Down
7 changes: 0 additions & 7 deletions tests/test_buffer_mode.py
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,4 @@
import time
import json
import redis
import pytest
import re

from pprint import pprint
from swsscommon import swsscommon

class TestBufferModel(object):
def test_bufferModel(self, dvs, testlog):
Expand Down

0 comments on commit e9fcc4a

Please sign in to comment.