Skip to content

Commit 3d60b3e

Browse files
stepanblyschakqiluo-msft
stepanblyschak
authored and
qiluo-msft
committed
[portsorch] Portsorch simple improvements (sonic-net#718)
* [portsorch] throw runtime_error object instead of const char* * [portsorch] simple improvements for portsorch - Change 'p' varaible name to 'port' - Pass 'Port' struct object to set/update methods instead of SAI OID to avoid unncessary 'for' loops that search port object in m_portList - minor simple changes * [portsorch]] address comments * [portsorch] make setHostIntfsOperStatus/updateDbPortOperStatus const
1 parent 95c3739 commit 3d60b3e

File tree

4 files changed

+111
-95
lines changed

4 files changed

+111
-95
lines changed

‎orchagent/neighorch.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ bool NeighOrch::addNextHop(IpAddress ipAddress, string alias)
9090
// flag Should be set on it.
9191
// This scenario may happen under race condition where buffered neighbor event
9292
// is processed after incoming port is down.
93-
if (p.m_oper_status == SAI_PORT_OPER_STATUS_DOWN)
93+
if (p.m_oper_status != SAI_PORT_OPER_STATUS_UP)
9494
{
9595
if (setNextHopFlag(ipAddress, NHFLAGS_IFDOWN) == false)
9696
{

‎orchagent/port.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ class Port
8585
sai_object_id_t m_ingress_acl_table_group_id = 0;
8686
sai_object_id_t m_egress_acl_table_group_id = 0;
8787
vlan_members_t m_vlan_members;
88-
sai_port_oper_status_t m_oper_status;
88+
sai_port_oper_status_t m_oper_status = SAI_PORT_OPER_STATUS_UNKNOWN;
8989
std::set<std::string> m_members;
9090
std::vector<sai_object_id_t> m_queue_ids;
9191
std::vector<sai_object_id_t> m_priority_group_ids;

‎orchagent/portsorch.cpp‎

Lines changed: 106 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ PortsOrch::PortsOrch(DBConnector *db, vector<table_name_with_pri_t> &tableNames)
214214
if (status != SAI_STATUS_SUCCESS)
215215
{
216216
SWSS_LOG_ERROR("Failed to get CPU port, rv:%d", status);
217-
throw "PortsOrch initialization failure";
217+
throw runtime_error("PortsOrch initialization failure");
218218
}
219219

220220
m_cpuPort = Port("CPU", Port::CPU);
@@ -228,7 +228,7 @@ PortsOrch::PortsOrch(DBConnector *db, vector<table_name_with_pri_t> &tableNames)
228228
if (status != SAI_STATUS_SUCCESS)
229229
{
230230
SWSS_LOG_ERROR("Failed to get port number, rv:%d", status);
231-
throw "PortsOrch initialization failure";
231+
throw runtime_error("PortsOrch initialization failure");
232232
}
233233

234234
m_portCount = attr.value.u32;
@@ -246,7 +246,7 @@ PortsOrch::PortsOrch(DBConnector *db, vector<table_name_with_pri_t> &tableNames)
246246
if (status != SAI_STATUS_SUCCESS)
247247
{
248248
SWSS_LOG_ERROR("Failed to get port list, rv:%d", status);
249-
throw "PortsOrch initialization failure";
249+
throw runtime_error("PortsOrch initialization failure");
250250
}
251251

252252
/* Get port hardware lane info */
@@ -261,7 +261,7 @@ PortsOrch::PortsOrch(DBConnector *db, vector<table_name_with_pri_t> &tableNames)
261261
if (status != SAI_STATUS_SUCCESS)
262262
{
263263
SWSS_LOG_ERROR("Failed to get hardware lane list pid:%lx", port_list[i]);
264-
throw "PortsOrch initialization failure";
264+
throw runtime_error("PortsOrch initialization failure");
265265
}
266266

267267
set<int> tmp_lane_set;
@@ -290,7 +290,7 @@ PortsOrch::PortsOrch(DBConnector *db, vector<table_name_with_pri_t> &tableNames)
290290
if (status != SAI_STATUS_SUCCESS)
291291
{
292292
SWSS_LOG_ERROR("Failed to get default 1Q bridge and/or default VLAN, rv:%d", status);
293-
throw "PortsOrch initialization failure";
293+
throw runtime_error("PortsOrch initialization failure");
294294
}
295295

296296
m_default1QBridge = attrs[0].value.oid;
@@ -320,7 +320,7 @@ void PortsOrch::removeDefaultVlanMembers()
320320
if (status != SAI_STATUS_SUCCESS)
321321
{
322322
SWSS_LOG_ERROR("Failed to get VLAN member list in default VLAN, rv:%d", status);
323-
throw "PortsOrch initialization failure";
323+
throw runtime_error("PortsOrch initialization failure");
324324
}
325325

326326
/* Remove VLAN members in default VLAN */
@@ -330,7 +330,7 @@ void PortsOrch::removeDefaultVlanMembers()
330330
if (status != SAI_STATUS_SUCCESS)
331331
{
332332
SWSS_LOG_ERROR("Failed to remove VLAN member, rv:%d", status);
333-
throw "PortsOrch initialization failure";
333+
throw runtime_error("PortsOrch initialization failure");
334334
}
335335
}
336336

@@ -354,7 +354,7 @@ void PortsOrch::removeDefaultBridgePorts()
354354
if (status != SAI_STATUS_SUCCESS)
355355
{
356356
SWSS_LOG_ERROR("Failed to get bridge port list in default 1Q bridge, rv:%d", status);
357-
throw "PortsOrch initialization failure";
357+
throw runtime_error("PortsOrch initialization failure");
358358
}
359359

360360
auto bridge_port_count = attr.value.objlist.count;
@@ -369,15 +369,15 @@ void PortsOrch::removeDefaultBridgePorts()
369369
if (status != SAI_STATUS_SUCCESS)
370370
{
371371
SWSS_LOG_ERROR("Failed to get bridge port type, rv:%d", status);
372-
throw "PortsOrch initialization failure";
372+
throw runtime_error("PortsOrch initialization failure");
373373
}
374374
if (attr.value.s32 == SAI_BRIDGE_PORT_TYPE_PORT)
375375
{
376376
status = sai_bridge_api->remove_bridge_port(bridge_port_list[i]);
377377
if (status != SAI_STATUS_SUCCESS)
378378
{
379379
SWSS_LOG_ERROR("Failed to remove bridge port, rv:%d", status);
380-
throw "PortsOrch initialization failure";
380+
throw runtime_error("PortsOrch initialization failure");
381381
}
382382
}
383383
}
@@ -1168,55 +1168,36 @@ bool PortsOrch::setPortAutoNeg(sai_object_id_t id, int an)
11681168
return true;
11691169
}
11701170

1171-
bool PortsOrch::setHostIntfsOperStatus(sai_object_id_t port_id, bool up)
1171+
bool PortsOrch::setHostIntfsOperStatus(const Port& port, bool isUp) const
11721172
{
11731173
SWSS_LOG_ENTER();
11741174

1175-
for (auto it = m_portList.begin(); it != m_portList.end(); it++)
1175+
sai_attribute_t attr;
1176+
attr.id = SAI_HOSTIF_ATTR_OPER_STATUS;
1177+
attr.value.booldata = isUp;
1178+
1179+
sai_status_t status = sai_hostif_api->set_hostif_attribute(port.m_hif_id, &attr);
1180+
if (status != SAI_STATUS_SUCCESS)
11761181
{
1177-
if (it->second.m_port_id != port_id)
1178-
{
1179-
continue;
1180-
}
1182+
SWSS_LOG_WARN("Failed to set operation status %s to host interface %s",
1183+
isUp ? "UP" : "DOWN", port.m_alias.c_str());
1184+
return false;
1185+
}
11811186

1182-
sai_attribute_t attr;
1183-
attr.id = SAI_HOSTIF_ATTR_OPER_STATUS;
1184-
attr.value.booldata = up;
1187+
SWSS_LOG_NOTICE("Set operation status %s to host interface %s",
1188+
isUp ? "UP" : "DOWN", port.m_alias.c_str());
11851189

1186-
sai_status_t status = sai_hostif_api->set_hostif_attribute(it->second.m_hif_id, &attr);
1187-
if (status != SAI_STATUS_SUCCESS)
1188-
{
1189-
SWSS_LOG_WARN("Failed to set operation status %s to host interface %s",
1190-
up ? "UP" : "DOWN", it->second.m_alias.c_str());
1191-
return false;
1192-
}
1193-
SWSS_LOG_NOTICE("Set operation status %s to host interface %s",
1194-
up ? "UP" : "DOWN", it->second.m_alias.c_str());
1195-
if (gNeighOrch->ifChangeInformNextHop(it->second.m_alias, up) == false)
1196-
{
1197-
SWSS_LOG_WARN("Inform nexthop operation failed for interface %s",
1198-
it->second.m_alias.c_str());
1199-
}
1200-
return true;
1201-
}
1202-
return false;
1190+
return true;
12031191
}
12041192

1205-
void PortsOrch::updateDbPortOperStatus(sai_object_id_t id, sai_port_oper_status_t status)
1193+
void PortsOrch::updateDbPortOperStatus(const Port& port, sai_port_oper_status_t status) const
12061194
{
12071195
SWSS_LOG_ENTER();
12081196

1209-
for (auto it = m_portList.begin(); it != m_portList.end(); it++)
1210-
{
1211-
if (it->second.m_port_id == id)
1212-
{
1213-
vector<FieldValueTuple> tuples;
1214-
FieldValueTuple tuple("oper_status", oper_status_strings.at(status));
1215-
tuples.push_back(tuple);
1216-
m_portTable->set(it->first, tuples);
1217-
it->second.m_oper_status = status;
1218-
}
1219-
}
1197+
vector<FieldValueTuple> tuples;
1198+
FieldValueTuple tuple("oper_status", oper_status_strings.at(status));
1199+
tuples.push_back(tuple);
1200+
m_portTable->set(port.m_alias, tuples);
12201201
}
12211202

12221203
bool PortsOrch::addPort(const set<int> &lane_set, uint32_t speed, int an, string fec_mode)
@@ -1481,6 +1462,7 @@ void PortsOrch::doPortTask(Consumer &consumer)
14811462

14821463
it = consumer.m_toSync.erase(it);
14831464
return;
1465+
14841466
}
14851467

14861468
if (op == SET_COMMAND)
@@ -2344,69 +2326,64 @@ void PortsOrch::initializePriorityGroups(Port &port)
23442326
SWSS_LOG_INFO("Get priority groups for port %s", port.m_alias.c_str());
23452327
}
23462328

2347-
bool PortsOrch::initializePort(Port &p)
2329+
bool PortsOrch::initializePort(Port &port)
23482330
{
23492331
SWSS_LOG_ENTER();
23502332

2351-
SWSS_LOG_NOTICE("Initializing port alias:%s pid:%lx", p.m_alias.c_str(), p.m_port_id);
2333+
SWSS_LOG_NOTICE("Initializing port alias:%s pid:%lx", port.m_alias.c_str(), port.m_port_id);
23522334

2353-
initializePriorityGroups(p);
2354-
initializeQueues(p);
2335+
initializePriorityGroups(port);
2336+
initializeQueues(port);
23552337

23562338
/* Create host interface */
2357-
addHostIntfs(p, p.m_alias, p.m_hif_id);
2339+
if (!addHostIntfs(port, port.m_alias, port.m_hif_id))
2340+
{
2341+
SWSS_LOG_ERROR("Failed to create host interface for port %s", port.m_alias.c_str());
2342+
return false;
2343+
}
23582344

23592345
/* Check warm start states */
23602346
vector<FieldValueTuple> tuples;
2361-
bool exist = m_portTable->get(p.m_alias, tuples);
2362-
string adminStatus, operStatus;
2347+
bool exist = m_portTable->get(port.m_alias, tuples);
2348+
string operStatus;
23632349
if (exist)
23642350
{
23652351
for (auto i : tuples)
23662352
{
2367-
if (fvField(i) == "admin_status")
2368-
{
2369-
adminStatus = fvValue(i);
2370-
}
2371-
else if (fvField(i) == "oper_status")
2353+
if (fvField(i) == "oper_status")
23722354
{
23732355
operStatus = fvValue(i);
23742356
}
23752357
}
23762358
}
2377-
SWSS_LOG_DEBUG("initializePort %s with admin %s and oper %s", p.m_alias.c_str(), adminStatus.c_str(), operStatus.c_str());
2359+
SWSS_LOG_DEBUG("initializePort %s with oper %s", port.m_alias.c_str(), operStatus.c_str());
23782360

23792361
/**
23802362
* Create database port oper status as DOWN if attr missing
23812363
* This status will be updated upon receiving port_oper_status_notification.
23822364
*/
23832365
if (operStatus == "up")
23842366
{
2385-
p.m_oper_status = SAI_PORT_OPER_STATUS_UP;
2367+
port.m_oper_status = SAI_PORT_OPER_STATUS_UP;
23862368
}
23872369
else if (operStatus.empty())
23882370
{
2389-
p.m_oper_status = SAI_PORT_OPER_STATUS_DOWN;
2371+
port.m_oper_status = SAI_PORT_OPER_STATUS_DOWN;
23902372
/* Fill oper_status in db with default value "down" */
2391-
m_portTable->hset(p.m_alias, "oper_status", "down");
2373+
m_portTable->hset(port.m_alias, "oper_status", "down");
23922374
}
23932375
else
23942376
{
2395-
p.m_oper_status = SAI_PORT_OPER_STATUS_DOWN;
2377+
port.m_oper_status = SAI_PORT_OPER_STATUS_DOWN;
23962378
}
23972379

23982380
/*
23992381
* always initialize Port SAI_HOSTIF_ATTR_OPER_STATUS based on oper_status value in appDB.
24002382
*/
2401-
sai_attribute_t attr;
2402-
attr.id = SAI_HOSTIF_ATTR_OPER_STATUS;
2403-
attr.value.booldata = (p.m_oper_status == SAI_PORT_OPER_STATUS_UP);
2404-
2405-
sai_status_t status = sai_hostif_api->set_hostif_attribute(p.m_hif_id, &attr);
2406-
if (status != SAI_STATUS_SUCCESS)
2383+
if (!setHostIntfsOperStatus(port, port.m_oper_status))
24072384
{
24082385
SWSS_LOG_WARN("Failed to set operation status %s to host interface %s",
2409-
operStatus.c_str(), p.m_alias.c_str());
2386+
operStatus.c_str(), port.m_alias.c_str());
24102387
return false;
24112388
}
24122389

@@ -3100,12 +3077,17 @@ void PortsOrch::doTask(NotificationConsumer &consumer)
31003077
SWSS_LOG_NOTICE("Get port state change notification id:%lx status:%d", id, status);
31013078

31023079
Port port;
3080+
31033081
if (!getPort(id, port))
31043082
{
31053083
SWSS_LOG_ERROR("Failed to get port object for port id 0x%lx", id);
31063084
continue;
31073085
}
3086+
31083087
updatePortOperStatus(port, status);
3088+
3089+
/* update m_portList */
3090+
m_portList[port.m_alias] = port;
31093091
}
31103092

31113093
sai_deserialize_free_port_oper_status_ntf(count, portoperstatus);
@@ -3117,13 +3099,23 @@ void PortsOrch::updatePortOperStatus(Port &port, sai_port_oper_status_t status)
31173099
SWSS_LOG_NOTICE("Port %s oper state set from %s to %s",
31183100
port.m_alias.c_str(), oper_status_strings.at(port.m_oper_status).c_str(),
31193101
oper_status_strings.at(status).c_str());
3120-
if (status != port.m_oper_status)
3102+
if (status == port.m_oper_status)
31213103
{
3122-
this->updateDbPortOperStatus(port.m_port_id, status);
3123-
if (status == SAI_PORT_OPER_STATUS_UP || port.m_oper_status == SAI_PORT_OPER_STATUS_UP)
3124-
{
3125-
this->setHostIntfsOperStatus(port.m_port_id, status == SAI_PORT_OPER_STATUS_UP);
3126-
}
3104+
return ;
3105+
}
3106+
3107+
updateDbPortOperStatus(port, status);
3108+
port.m_oper_status = status;
3109+
3110+
bool isUp = status == SAI_PORT_OPER_STATUS_UP;
3111+
if (!setHostIntfsOperStatus(port, isUp))
3112+
{
3113+
SWSS_LOG_ERROR("Failed to set host interface %s operational status %s", port.m_alias.c_str(),
3114+
isUp ? "up" : "down");
3115+
}
3116+
if (!gNeighOrch->ifChangeInformNextHop(port.m_alias, isUp))
3117+
{
3118+
SWSS_LOG_WARN("Inform nexthop operation failed for interface %s", port.m_alias.c_str());
31273119
}
31283120
}
31293121

@@ -3144,21 +3136,44 @@ void PortsOrch::refreshPortStatus()
31443136

31453137
for (auto &it: m_portList)
31463138
{
3147-
auto &p = it.second;
3148-
if (p.m_type == Port::PHY)
3139+
auto &port = it.second;
3140+
if (port.m_type != Port::PHY)
31493141
{
3150-
sai_attribute_t attr;
3151-
attr.id = SAI_PORT_ATTR_OPER_STATUS;
3142+
continue;
3143+
}
31523144

3153-
sai_status_t ret = sai_port_api->get_port_attribute(p.m_port_id, 1, &attr);
3154-
if (ret != SAI_STATUS_SUCCESS)
3155-
{
3156-
SWSS_LOG_ERROR("Failed to get oper status for %s", p.m_alias.c_str());
3157-
throw "PortsOrch get port oper status failure";
3158-
}
3159-
sai_port_oper_status_t status = (sai_port_oper_status_t)attr.value.u32;
3160-
SWSS_LOG_INFO("%s oper status is %s", p.m_alias.c_str(), oper_status_strings.at(status).c_str());
3161-
updatePortOperStatus(p, status);
3145+
sai_port_oper_status_t status;
3146+
if (!getPortOperStatus(port, status))
3147+
{
3148+
throw runtime_error("PortsOrch get port oper status failure");
31623149
}
3150+
3151+
SWSS_LOG_INFO("%s oper status is %s", port.m_alias.c_str(), oper_status_strings.at(status).c_str());
3152+
updatePortOperStatus(port, status);
31633153
}
31643154
}
3155+
3156+
bool PortsOrch::getPortOperStatus(const Port& port, sai_port_oper_status_t& status) const
3157+
{
3158+
SWSS_LOG_ENTER();
3159+
3160+
if (port.m_type != Port::PHY)
3161+
{
3162+
return false;
3163+
}
3164+
3165+
sai_attribute_t attr;
3166+
attr.id = SAI_PORT_ATTR_OPER_STATUS;
3167+
3168+
sai_status_t ret = sai_port_api->get_port_attribute(port.m_port_id, 1, &attr);
3169+
if (ret != SAI_STATUS_SUCCESS)
3170+
{
3171+
SWSS_LOG_ERROR("Failed to get oper_status for %s", port.m_alias.c_str());
3172+
return false;
3173+
}
3174+
3175+
status = static_cast<sai_port_oper_status_t>(attr.value.u32);
3176+
3177+
return true;
3178+
}
3179+

0 commit comments

Comments
 (0)