From 3d60b3ebcf1fd82722b53d82018a5ed3844b93ef Mon Sep 17 00:00:00 2001 From: stepanblyschak <38952541+stepanblyschak@users.noreply.github.com> Date: Mon, 3 Dec 2018 20:48:58 +0200 Subject: [PATCH] [portsorch] Portsorch simple improvements (#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 --- orchagent/neighorch.cpp | 2 +- orchagent/port.h | 2 +- orchagent/portsorch.cpp | 197 +++++++++++++++++++++------------------- orchagent/portsorch.h | 5 +- 4 files changed, 111 insertions(+), 95 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index b8d2aee13fdc..addcfa800d27 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -90,7 +90,7 @@ bool NeighOrch::addNextHop(IpAddress ipAddress, string alias) // flag Should be set on it. // This scenario may happen under race condition where buffered neighbor event // is processed after incoming port is down. - if (p.m_oper_status == SAI_PORT_OPER_STATUS_DOWN) + if (p.m_oper_status != SAI_PORT_OPER_STATUS_UP) { if (setNextHopFlag(ipAddress, NHFLAGS_IFDOWN) == false) { diff --git a/orchagent/port.h b/orchagent/port.h index d3cf76bcf230..454ecd68af82 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -85,7 +85,7 @@ class Port sai_object_id_t m_ingress_acl_table_group_id = 0; sai_object_id_t m_egress_acl_table_group_id = 0; vlan_members_t m_vlan_members; - sai_port_oper_status_t m_oper_status; + sai_port_oper_status_t m_oper_status = SAI_PORT_OPER_STATUS_UNKNOWN; std::set m_members; std::vector m_queue_ids; std::vector m_priority_group_ids; diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index df96d5099fa8..405e20b91d5f 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -214,7 +214,7 @@ PortsOrch::PortsOrch(DBConnector *db, vector &tableNames) if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to get CPU port, rv:%d", status); - throw "PortsOrch initialization failure"; + throw runtime_error("PortsOrch initialization failure"); } m_cpuPort = Port("CPU", Port::CPU); @@ -228,7 +228,7 @@ PortsOrch::PortsOrch(DBConnector *db, vector &tableNames) if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to get port number, rv:%d", status); - throw "PortsOrch initialization failure"; + throw runtime_error("PortsOrch initialization failure"); } m_portCount = attr.value.u32; @@ -246,7 +246,7 @@ PortsOrch::PortsOrch(DBConnector *db, vector &tableNames) if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to get port list, rv:%d", status); - throw "PortsOrch initialization failure"; + throw runtime_error("PortsOrch initialization failure"); } /* Get port hardware lane info */ @@ -261,7 +261,7 @@ PortsOrch::PortsOrch(DBConnector *db, vector &tableNames) if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to get hardware lane list pid:%lx", port_list[i]); - throw "PortsOrch initialization failure"; + throw runtime_error("PortsOrch initialization failure"); } set tmp_lane_set; @@ -290,7 +290,7 @@ PortsOrch::PortsOrch(DBConnector *db, vector &tableNames) if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to get default 1Q bridge and/or default VLAN, rv:%d", status); - throw "PortsOrch initialization failure"; + throw runtime_error("PortsOrch initialization failure"); } m_default1QBridge = attrs[0].value.oid; @@ -320,7 +320,7 @@ void PortsOrch::removeDefaultVlanMembers() if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to get VLAN member list in default VLAN, rv:%d", status); - throw "PortsOrch initialization failure"; + throw runtime_error("PortsOrch initialization failure"); } /* Remove VLAN members in default VLAN */ @@ -330,7 +330,7 @@ void PortsOrch::removeDefaultVlanMembers() if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to remove VLAN member, rv:%d", status); - throw "PortsOrch initialization failure"; + throw runtime_error("PortsOrch initialization failure"); } } @@ -354,7 +354,7 @@ void PortsOrch::removeDefaultBridgePorts() if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to get bridge port list in default 1Q bridge, rv:%d", status); - throw "PortsOrch initialization failure"; + throw runtime_error("PortsOrch initialization failure"); } auto bridge_port_count = attr.value.objlist.count; @@ -369,7 +369,7 @@ void PortsOrch::removeDefaultBridgePorts() if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to get bridge port type, rv:%d", status); - throw "PortsOrch initialization failure"; + throw runtime_error("PortsOrch initialization failure"); } if (attr.value.s32 == SAI_BRIDGE_PORT_TYPE_PORT) { @@ -377,7 +377,7 @@ void PortsOrch::removeDefaultBridgePorts() if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to remove bridge port, rv:%d", status); - throw "PortsOrch initialization failure"; + throw runtime_error("PortsOrch initialization failure"); } } } @@ -1168,55 +1168,36 @@ bool PortsOrch::setPortAutoNeg(sai_object_id_t id, int an) return true; } -bool PortsOrch::setHostIntfsOperStatus(sai_object_id_t port_id, bool up) +bool PortsOrch::setHostIntfsOperStatus(const Port& port, bool isUp) const { SWSS_LOG_ENTER(); - for (auto it = m_portList.begin(); it != m_portList.end(); it++) + sai_attribute_t attr; + attr.id = SAI_HOSTIF_ATTR_OPER_STATUS; + attr.value.booldata = isUp; + + sai_status_t status = sai_hostif_api->set_hostif_attribute(port.m_hif_id, &attr); + if (status != SAI_STATUS_SUCCESS) { - if (it->second.m_port_id != port_id) - { - continue; - } + SWSS_LOG_WARN("Failed to set operation status %s to host interface %s", + isUp ? "UP" : "DOWN", port.m_alias.c_str()); + return false; + } - sai_attribute_t attr; - attr.id = SAI_HOSTIF_ATTR_OPER_STATUS; - attr.value.booldata = up; + SWSS_LOG_NOTICE("Set operation status %s to host interface %s", + isUp ? "UP" : "DOWN", port.m_alias.c_str()); - sai_status_t status = sai_hostif_api->set_hostif_attribute(it->second.m_hif_id, &attr); - if (status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_WARN("Failed to set operation status %s to host interface %s", - up ? "UP" : "DOWN", it->second.m_alias.c_str()); - return false; - } - SWSS_LOG_NOTICE("Set operation status %s to host interface %s", - up ? "UP" : "DOWN", it->second.m_alias.c_str()); - if (gNeighOrch->ifChangeInformNextHop(it->second.m_alias, up) == false) - { - SWSS_LOG_WARN("Inform nexthop operation failed for interface %s", - it->second.m_alias.c_str()); - } - return true; - } - return false; + return true; } -void PortsOrch::updateDbPortOperStatus(sai_object_id_t id, sai_port_oper_status_t status) +void PortsOrch::updateDbPortOperStatus(const Port& port, sai_port_oper_status_t status) const { SWSS_LOG_ENTER(); - for (auto it = m_portList.begin(); it != m_portList.end(); it++) - { - if (it->second.m_port_id == id) - { - vector tuples; - FieldValueTuple tuple("oper_status", oper_status_strings.at(status)); - tuples.push_back(tuple); - m_portTable->set(it->first, tuples); - it->second.m_oper_status = status; - } - } + vector tuples; + FieldValueTuple tuple("oper_status", oper_status_strings.at(status)); + tuples.push_back(tuple); + m_portTable->set(port.m_alias, tuples); } bool PortsOrch::addPort(const set &lane_set, uint32_t speed, int an, string fec_mode) @@ -1481,6 +1462,7 @@ void PortsOrch::doPortTask(Consumer &consumer) it = consumer.m_toSync.erase(it); return; + } if (op == SET_COMMAND) @@ -2344,37 +2326,37 @@ void PortsOrch::initializePriorityGroups(Port &port) SWSS_LOG_INFO("Get priority groups for port %s", port.m_alias.c_str()); } -bool PortsOrch::initializePort(Port &p) +bool PortsOrch::initializePort(Port &port) { SWSS_LOG_ENTER(); - SWSS_LOG_NOTICE("Initializing port alias:%s pid:%lx", p.m_alias.c_str(), p.m_port_id); + SWSS_LOG_NOTICE("Initializing port alias:%s pid:%lx", port.m_alias.c_str(), port.m_port_id); - initializePriorityGroups(p); - initializeQueues(p); + initializePriorityGroups(port); + initializeQueues(port); /* Create host interface */ - addHostIntfs(p, p.m_alias, p.m_hif_id); + if (!addHostIntfs(port, port.m_alias, port.m_hif_id)) + { + SWSS_LOG_ERROR("Failed to create host interface for port %s", port.m_alias.c_str()); + return false; + } /* Check warm start states */ vector tuples; - bool exist = m_portTable->get(p.m_alias, tuples); - string adminStatus, operStatus; + bool exist = m_portTable->get(port.m_alias, tuples); + string operStatus; if (exist) { for (auto i : tuples) { - if (fvField(i) == "admin_status") - { - adminStatus = fvValue(i); - } - else if (fvField(i) == "oper_status") + if (fvField(i) == "oper_status") { operStatus = fvValue(i); } } } - SWSS_LOG_DEBUG("initializePort %s with admin %s and oper %s", p.m_alias.c_str(), adminStatus.c_str(), operStatus.c_str()); + SWSS_LOG_DEBUG("initializePort %s with oper %s", port.m_alias.c_str(), operStatus.c_str()); /** * Create database port oper status as DOWN if attr missing @@ -2382,31 +2364,26 @@ bool PortsOrch::initializePort(Port &p) */ if (operStatus == "up") { - p.m_oper_status = SAI_PORT_OPER_STATUS_UP; + port.m_oper_status = SAI_PORT_OPER_STATUS_UP; } else if (operStatus.empty()) { - p.m_oper_status = SAI_PORT_OPER_STATUS_DOWN; + port.m_oper_status = SAI_PORT_OPER_STATUS_DOWN; /* Fill oper_status in db with default value "down" */ - m_portTable->hset(p.m_alias, "oper_status", "down"); + m_portTable->hset(port.m_alias, "oper_status", "down"); } else { - p.m_oper_status = SAI_PORT_OPER_STATUS_DOWN; + port.m_oper_status = SAI_PORT_OPER_STATUS_DOWN; } /* * always initialize Port SAI_HOSTIF_ATTR_OPER_STATUS based on oper_status value in appDB. */ - sai_attribute_t attr; - attr.id = SAI_HOSTIF_ATTR_OPER_STATUS; - attr.value.booldata = (p.m_oper_status == SAI_PORT_OPER_STATUS_UP); - - sai_status_t status = sai_hostif_api->set_hostif_attribute(p.m_hif_id, &attr); - if (status != SAI_STATUS_SUCCESS) + if (!setHostIntfsOperStatus(port, port.m_oper_status)) { SWSS_LOG_WARN("Failed to set operation status %s to host interface %s", - operStatus.c_str(), p.m_alias.c_str()); + operStatus.c_str(), port.m_alias.c_str()); return false; } @@ -3100,12 +3077,17 @@ void PortsOrch::doTask(NotificationConsumer &consumer) SWSS_LOG_NOTICE("Get port state change notification id:%lx status:%d", id, status); Port port; + if (!getPort(id, port)) { SWSS_LOG_ERROR("Failed to get port object for port id 0x%lx", id); continue; } + updatePortOperStatus(port, status); + + /* update m_portList */ + m_portList[port.m_alias] = port; } sai_deserialize_free_port_oper_status_ntf(count, portoperstatus); @@ -3117,13 +3099,23 @@ void PortsOrch::updatePortOperStatus(Port &port, sai_port_oper_status_t status) SWSS_LOG_NOTICE("Port %s oper state set from %s to %s", port.m_alias.c_str(), oper_status_strings.at(port.m_oper_status).c_str(), oper_status_strings.at(status).c_str()); - if (status != port.m_oper_status) + if (status == port.m_oper_status) { - this->updateDbPortOperStatus(port.m_port_id, status); - if (status == SAI_PORT_OPER_STATUS_UP || port.m_oper_status == SAI_PORT_OPER_STATUS_UP) - { - this->setHostIntfsOperStatus(port.m_port_id, status == SAI_PORT_OPER_STATUS_UP); - } + return ; + } + + updateDbPortOperStatus(port, status); + port.m_oper_status = status; + + bool isUp = status == SAI_PORT_OPER_STATUS_UP; + if (!setHostIntfsOperStatus(port, isUp)) + { + SWSS_LOG_ERROR("Failed to set host interface %s operational status %s", port.m_alias.c_str(), + isUp ? "up" : "down"); + } + if (!gNeighOrch->ifChangeInformNextHop(port.m_alias, isUp)) + { + SWSS_LOG_WARN("Inform nexthop operation failed for interface %s", port.m_alias.c_str()); } } @@ -3144,21 +3136,44 @@ void PortsOrch::refreshPortStatus() for (auto &it: m_portList) { - auto &p = it.second; - if (p.m_type == Port::PHY) + auto &port = it.second; + if (port.m_type != Port::PHY) { - sai_attribute_t attr; - attr.id = SAI_PORT_ATTR_OPER_STATUS; + continue; + } - sai_status_t ret = sai_port_api->get_port_attribute(p.m_port_id, 1, &attr); - if (ret != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to get oper status for %s", p.m_alias.c_str()); - throw "PortsOrch get port oper status failure"; - } - sai_port_oper_status_t status = (sai_port_oper_status_t)attr.value.u32; - SWSS_LOG_INFO("%s oper status is %s", p.m_alias.c_str(), oper_status_strings.at(status).c_str()); - updatePortOperStatus(p, status); + sai_port_oper_status_t status; + if (!getPortOperStatus(port, status)) + { + throw runtime_error("PortsOrch get port oper status failure"); } + + SWSS_LOG_INFO("%s oper status is %s", port.m_alias.c_str(), oper_status_strings.at(status).c_str()); + updatePortOperStatus(port, status); } } + +bool PortsOrch::getPortOperStatus(const Port& port, sai_port_oper_status_t& status) const +{ + SWSS_LOG_ENTER(); + + if (port.m_type != Port::PHY) + { + return false; + } + + sai_attribute_t attr; + attr.id = SAI_PORT_ATTR_OPER_STATUS; + + sai_status_t ret = sai_port_api->get_port_attribute(port.m_port_id, 1, &attr); + if (ret != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to get oper_status for %s", port.m_alias.c_str()); + return false; + } + + status = static_cast(attr.value.u32); + + return true; +} + diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index d55485c7a35b..265c1d8d2bc1 100644 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -70,8 +70,8 @@ class PortsOrch : public Orch, public Subject bool getVlanByVlanId(sai_vlan_id_t vlan_id, Port &vlan); bool getAclBindPortId(string alias, sai_object_id_t &port_id); - bool setHostIntfsOperStatus(sai_object_id_t id, bool up); - void updateDbPortOperStatus(sai_object_id_t id, sai_port_oper_status_t status); + bool setHostIntfsOperStatus(const Port& port, bool up) const; + void updateDbPortOperStatus(const Port& port, sai_port_oper_status_t status) const; bool bindAclTable(sai_object_id_t id, sai_object_id_t table_oid, sai_object_id_t &group_member_oid, acl_stage_type_t acl_stage = ACL_STAGE_INGRESS); bool getPortPfc(sai_object_id_t portId, uint8_t *pfc_bitmask); @@ -182,6 +182,7 @@ class PortsOrch : public Orch, public Subject bool setPortAutoNeg(sai_object_id_t id, int an); bool setPortFecMode(sai_object_id_t id, int fec); + bool getPortOperStatus(const Port& port, sai_port_oper_status_t& status) const; void updatePortOperStatus(Port &port, sai_port_oper_status_t status); }; #endif /* SWSS_PORTSORCH_H */