From 047841e3b942d6065107a0ddecf927d1805128bf Mon Sep 17 00:00:00 2001 From: Dhanasekar Rathinavel Date: Wed, 31 Mar 2021 12:33:11 -0700 Subject: [PATCH 01/11] PR1 --- orchagent/fdborch.cpp | 35 +++++++--- orchagent/port.h | 1 + orchagent/portsorch.cpp | 145 +++++++++++++++++++++++----------------- orchagent/portsorch.h | 5 ++ 4 files changed, 116 insertions(+), 70 deletions(-) diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index cd4888bc1c..c5880bf174 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -305,6 +305,12 @@ void FdbOrch::update(sai_fdb_event_t type, { update.port.m_fdb_count--; m_portsOrch->setPort(update.port.m_alias, update.port); + + // this is not posted in last PR but its present in gerrit + if ((update.port.m_fdb_count == 0) && (update.port.m_vlan_members.empty())) + { + m_portsOrch->removeBridgePort(update.port); + } } if (!vlan.m_alias.empty()) { @@ -686,16 +692,15 @@ void FdbOrch::doTask(NotificationConsumer& consumer) std::string data; std::vector values; + Port vlan; + Port port; + consumer.pop(op, data, values); if (&consumer == m_flushNotificationsConsumer) { if (op == "ALL") { - /* - * so far only support flush all the FDB entries - * flush per port and flush per vlan will be added later. - */ status = sai_fdb_api->flush_fdb_entries(gSwitchId, 0, NULL); if (status != SAI_STATUS_SUCCESS) { @@ -706,14 +711,22 @@ void FdbOrch::doTask(NotificationConsumer& consumer) } else if (op == "PORT") { - /*place holder for flush port fdb*/ - SWSS_LOG_ERROR("Received unsupported flush port fdb request"); + if (!m_portsOrch->getPort(data, port)) + { + SWSS_LOG_ERROR("could not locate port from data %s", data.c_str()); + return; + } + flushFDBEntries(port.m_bridge_port_id, SAI_NULL_OBJECT_ID); return; } else if (op == "VLAN") { - /*place holder for flush vlan fdb*/ - SWSS_LOG_ERROR("Received unsupported flush vlan fdb request"); + if (!m_portsOrch->getPort(data, vlan)) + { + SWSS_LOG_ERROR("could not locate vlan from data %s", data.c_str()); + return; + } + flushFDBEntries(SAI_NULL_OBJECT_ID, vlan.m_vlan_info.vlan_oid); return; } else @@ -726,6 +739,12 @@ void FdbOrch::doTask(NotificationConsumer& consumer) { uint32_t count; sai_fdb_event_notification_data_t *fdbevent = nullptr; + extern void handle_fdb_event(_In_ const std::string &data); + /* The sai_redis fdb handling is moved here so that both + * reference count updates (sai_redis and portsOrch) + * happen in same context to avoid any mismatch. + */ + handle_fdb_event(data); sai_deserialize_fdb_event_ntf(data, count, &fdbevent); diff --git a/orchagent/port.h b/orchagent/port.h index 86e84a1636..e7f4eb2331 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -108,6 +108,7 @@ class Port VlanInfo m_vlan_info; MacAddress m_mac; sai_object_id_t m_bridge_port_id = 0; // TODO: port could have multiple bridge port IDs + sai_object_id_t m_bridge_port_admin_state = 0; // TODO: port could have multiple bridge port IDs sai_vlan_id_t m_port_vlan_id = DEFAULT_PORT_VLAN_ID; // Port VLAN ID sai_object_id_t m_rif_id = 0; sai_object_id_t m_vr_id = 0; diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index d776ee84e7..58040571b8 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -611,35 +611,12 @@ bool PortsOrch::getPort(sai_object_id_t id, Port &port) { SWSS_LOG_ENTER(); - for (const auto& portIter: m_portList) - { - switch (portIter.second.m_type) - { - case Port::PHY: - case Port::SYSTEM: - if(portIter.second.m_port_id == id) - { - port = portIter.second; - return true; - } - break; - case Port::LAG: - if(portIter.second.m_lag_id == id) - { - port = portIter.second; - return true; - } - break; - case Port::VLAN: - if (portIter.second.m_vlan_info.vlan_oid == id) - { - port = portIter.second; - return true; - } - break; - default: - continue; - } + auto itr = portOidToName.find(id); + if (itr == portOidToName.end()) + return false; + else { + getPort(itr->second, port); + return true; } return false; @@ -661,15 +638,13 @@ bool PortsOrch::getPortByBridgePortId(sai_object_id_t bridge_port_id, Port &port { SWSS_LOG_ENTER(); - for (auto &it: m_portList) - { - if (it.second.m_bridge_port_id == bridge_port_id) - { - port = it.second; - return true; - } + auto itr = portOidToName.find(bridge_port_id); + if (itr == portOidToName.end()) + return false; + else { + getPort(itr->second, port); + return true; } - return false; } @@ -2073,6 +2048,7 @@ bool PortsOrch::initPort(const string &alias, const int index, const set &l /* Add port to port list */ m_portList[alias] = p; + portOidToName[id] = alias; m_port_ref_count[alias] = 0; m_portOidToIndex[id] = index; @@ -3034,24 +3010,24 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer) } else if (op == DEL_COMMAND) { + int ret = true; + if (vlan.m_members.find(port_alias) != vlan.m_members.end()) { - if (removeVlanMember(vlan, port)) - { - if (port.m_vlan_members.empty()) - { - removeBridgePort(port); - } - it = consumer.m_toSync.erase(it); - } - else - { - it++; - } + ret = removeVlanMember(vlan, port); } - else - /* Cannot locate the VLAN */ + if ((ret == true) && port.m_vlan_members.empty()) + { + ret = removeBridgePort(port); + } + if (ret == true) + { it = consumer.m_toSync.erase(it); + } + else + { + it++; + } } else { @@ -3131,8 +3107,7 @@ void PortsOrch::doLagTask(Consumer &consumer) switch_id = -1; } - // Create a new LAG when the new alias comes - if (m_portList.find(alias) == m_portList.end()) + if (!addLag(alias)) { if (!addLag(alias, lag_id, switch_id)) { @@ -3671,6 +3646,36 @@ bool PortsOrch::addBridgePort(Port &port) if (port.m_bridge_port_id != SAI_NULL_OBJECT_ID) { + /* If the port is being added to the first VLAN, + * set bridge port admin status to UP. + * This can happen if the port was just removed from + * last VLAN and fdb flush is still in progress. + */ + if (port.m_vlan_members.empty()) + { + if (port.m_fdb_count > 0) + { + return false; + } + sai_attribute_t attr; + attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE; + attr.value.booldata = true; + sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to set bridge port %s admin status to UP, rv:%d", + port.m_alias.c_str(), status); + return false; + } + port.m_bridge_port_admin_state = true; + m_portList[port.m_alias] = port; + if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP)) + { + SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", + hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_KEEP], port.m_alias.c_str()); + return false; + } + } return true; } @@ -3748,6 +3753,8 @@ bool PortsOrch::addBridgePort(Port &port) } } + port.m_bridge_port_admin_state = true; + if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP)) { SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", @@ -3755,6 +3762,7 @@ bool PortsOrch::addBridgePort(Port &port) return false; } m_portList[port.m_alias] = port; + portOidToName[port.m_bridge_port_id] = port.m_alias; SWSS_LOG_NOTICE("Add bridge port %s to default 1Q bridge", port.m_alias.c_str()); return true; @@ -3768,13 +3776,7 @@ bool PortsOrch::removeBridgePort(Port &port) { return true; } - /* Set bridge port admin status to DOWN */ - sai_attribute_t attr; - attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE; - attr.value.booldata = false; - - sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr); - if (status != SAI_STATUS_SUCCESS) + if (port.m_bridge_port_admin_state) { SWSS_LOG_ERROR("Failed to set bridge port %s admin status to DOWN, rv:%d", port.m_alias.c_str(), status); @@ -3797,7 +3799,7 @@ bool PortsOrch::removeBridgePort(Port &port) SWSS_LOG_INFO("Flush FDB entries for port %s", port.m_alias.c_str()); /* Remove bridge port */ - status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id); + sai_status_t status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id); if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to remove bridge port %s from default 1Q bridge, rv:%d", @@ -3808,6 +3810,7 @@ bool PortsOrch::removeBridgePort(Port &port) return parseHandleSaiStatusFailure(handle_status); } } + portOidToName.erase(port.m_bridge_port_id); port.m_bridge_port_id = SAI_NULL_OBJECT_ID; SWSS_LOG_NOTICE("Remove bridge port %s from default 1Q bridge", port.m_alias.c_str()); @@ -3876,7 +3879,7 @@ bool PortsOrch::addVlan(string vlan_alias) } } - SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu", vlan_alias.c_str(), vlan_id); + SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu vlan_oid 0x%lx", vlan_alias.c_str(), vlan_id, vlan_oid); Port vlan(vlan_alias, Port::VLAN); vlan.m_vlan_info.vlan_oid = vlan_oid; @@ -3884,6 +3887,7 @@ bool PortsOrch::addVlan(string vlan_alias) vlan.m_members = set(); m_portList[vlan_alias] = vlan; m_port_ref_count[vlan_alias] = 0; + portOidToName[vlan_oid] = vlan_alias; return true; } @@ -3891,6 +3895,15 @@ bool PortsOrch::addVlan(string vlan_alias) bool PortsOrch::removeVlan(Port vlan) { SWSS_LOG_ENTER(); + + /* If there are still fdb entries associated with the VLAN, + return false for retry */ + if (vlan.m_fdb_count > 0) + { + SWSS_LOG_NOTICE("VLAN %s still has assiciated FDB entries", vlan.m_alias.c_str()); + return false; + } + if (m_port_ref_count[vlan.m_alias] > 0) { SWSS_LOG_ERROR("Failed to remove ref count %d VLAN %s", @@ -3931,6 +3944,7 @@ bool PortsOrch::removeVlan(Port vlan) SWSS_LOG_NOTICE("Remove VLAN %s vid:%hu", vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id); + portOidToName.erase(vlan.m_vlan_info.vlan_oid); m_portList.erase(vlan.m_alias); m_port_ref_count.erase(vlan.m_alias); @@ -4127,6 +4141,7 @@ bool PortsOrch::addLag(string lag_alias, uint32_t spa_id, int32_t switch_id) lag.m_members = set(); m_portList[lag_alias] = lag; m_port_ref_count[lag_alias] = 0; + portOidToName[lag_id] = lag_alias; PortUpdate update = { lag, true }; notify(SUBJECT_TYPE_PORT_CHANGE, static_cast(&update)); @@ -4181,6 +4196,11 @@ bool PortsOrch::removeLag(Port lag) return false; } + if (lag.m_bridge_port_id != SAI_NULL_OBJECT_ID) + { + return false; + } + sai_status_t status = sai_lag_api->remove_lag(lag.m_lag_id); if (status != SAI_STATUS_SUCCESS) { @@ -4194,6 +4214,7 @@ bool PortsOrch::removeLag(Port lag) SWSS_LOG_NOTICE("Remove LAG %s lid:%" PRIx64, lag.m_alias.c_str(), lag.m_lag_id); + portOidToName.erase(lag.m_lag_id); m_portList.erase(lag.m_alias); m_port_ref_count.erase(lag.m_alias); diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index bcd5b5d38d..b6e78f6374 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -207,6 +207,11 @@ class PortsOrch : public Orch, public Subject map, sai_object_id_t> m_portListLaneMap; map, tuple> m_lanesAliasSpeedMap; map m_portList; + /* mapping from SAI object ID to Name for faster + * retrieval of Port/VLAN from object ID for events + * coming from SAI + */ + unordered_map portOidToName; unordered_map m_portOidToIndex; map m_port_ref_count; unordered_set m_pendingPortSet; From b19d1c627d2dd6cb14d86bf0b6664cc38bf831e9 Mon Sep 17 00:00:00 2001 From: Dhanasekar Rathinavel Date: Mon, 5 Apr 2021 09:40:48 -0700 Subject: [PATCH 02/11] Revert "PR1" This reverts commit 047841e3b942d6065107a0ddecf927d1805128bf. --- orchagent/fdborch.cpp | 35 +++------- orchagent/port.h | 1 - orchagent/portsorch.cpp | 145 +++++++++++++++++----------------------- orchagent/portsorch.h | 5 -- 4 files changed, 70 insertions(+), 116 deletions(-) diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index c5880bf174..cd4888bc1c 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -305,12 +305,6 @@ void FdbOrch::update(sai_fdb_event_t type, { update.port.m_fdb_count--; m_portsOrch->setPort(update.port.m_alias, update.port); - - // this is not posted in last PR but its present in gerrit - if ((update.port.m_fdb_count == 0) && (update.port.m_vlan_members.empty())) - { - m_portsOrch->removeBridgePort(update.port); - } } if (!vlan.m_alias.empty()) { @@ -692,15 +686,16 @@ void FdbOrch::doTask(NotificationConsumer& consumer) std::string data; std::vector values; - Port vlan; - Port port; - consumer.pop(op, data, values); if (&consumer == m_flushNotificationsConsumer) { if (op == "ALL") { + /* + * so far only support flush all the FDB entries + * flush per port and flush per vlan will be added later. + */ status = sai_fdb_api->flush_fdb_entries(gSwitchId, 0, NULL); if (status != SAI_STATUS_SUCCESS) { @@ -711,22 +706,14 @@ void FdbOrch::doTask(NotificationConsumer& consumer) } else if (op == "PORT") { - if (!m_portsOrch->getPort(data, port)) - { - SWSS_LOG_ERROR("could not locate port from data %s", data.c_str()); - return; - } - flushFDBEntries(port.m_bridge_port_id, SAI_NULL_OBJECT_ID); + /*place holder for flush port fdb*/ + SWSS_LOG_ERROR("Received unsupported flush port fdb request"); return; } else if (op == "VLAN") { - if (!m_portsOrch->getPort(data, vlan)) - { - SWSS_LOG_ERROR("could not locate vlan from data %s", data.c_str()); - return; - } - flushFDBEntries(SAI_NULL_OBJECT_ID, vlan.m_vlan_info.vlan_oid); + /*place holder for flush vlan fdb*/ + SWSS_LOG_ERROR("Received unsupported flush vlan fdb request"); return; } else @@ -739,12 +726,6 @@ void FdbOrch::doTask(NotificationConsumer& consumer) { uint32_t count; sai_fdb_event_notification_data_t *fdbevent = nullptr; - extern void handle_fdb_event(_In_ const std::string &data); - /* The sai_redis fdb handling is moved here so that both - * reference count updates (sai_redis and portsOrch) - * happen in same context to avoid any mismatch. - */ - handle_fdb_event(data); sai_deserialize_fdb_event_ntf(data, count, &fdbevent); diff --git a/orchagent/port.h b/orchagent/port.h index e7f4eb2331..86e84a1636 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -108,7 +108,6 @@ class Port VlanInfo m_vlan_info; MacAddress m_mac; sai_object_id_t m_bridge_port_id = 0; // TODO: port could have multiple bridge port IDs - sai_object_id_t m_bridge_port_admin_state = 0; // TODO: port could have multiple bridge port IDs sai_vlan_id_t m_port_vlan_id = DEFAULT_PORT_VLAN_ID; // Port VLAN ID sai_object_id_t m_rif_id = 0; sai_object_id_t m_vr_id = 0; diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 58040571b8..d776ee84e7 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -611,12 +611,35 @@ bool PortsOrch::getPort(sai_object_id_t id, Port &port) { SWSS_LOG_ENTER(); - auto itr = portOidToName.find(id); - if (itr == portOidToName.end()) - return false; - else { - getPort(itr->second, port); - return true; + for (const auto& portIter: m_portList) + { + switch (portIter.second.m_type) + { + case Port::PHY: + case Port::SYSTEM: + if(portIter.second.m_port_id == id) + { + port = portIter.second; + return true; + } + break; + case Port::LAG: + if(portIter.second.m_lag_id == id) + { + port = portIter.second; + return true; + } + break; + case Port::VLAN: + if (portIter.second.m_vlan_info.vlan_oid == id) + { + port = portIter.second; + return true; + } + break; + default: + continue; + } } return false; @@ -638,13 +661,15 @@ bool PortsOrch::getPortByBridgePortId(sai_object_id_t bridge_port_id, Port &port { SWSS_LOG_ENTER(); - auto itr = portOidToName.find(bridge_port_id); - if (itr == portOidToName.end()) - return false; - else { - getPort(itr->second, port); - return true; + for (auto &it: m_portList) + { + if (it.second.m_bridge_port_id == bridge_port_id) + { + port = it.second; + return true; + } } + return false; } @@ -2048,7 +2073,6 @@ bool PortsOrch::initPort(const string &alias, const int index, const set &l /* Add port to port list */ m_portList[alias] = p; - portOidToName[id] = alias; m_port_ref_count[alias] = 0; m_portOidToIndex[id] = index; @@ -3010,24 +3034,24 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer) } else if (op == DEL_COMMAND) { - int ret = true; - if (vlan.m_members.find(port_alias) != vlan.m_members.end()) { - ret = removeVlanMember(vlan, port); - } - if ((ret == true) && port.m_vlan_members.empty()) - { - ret = removeBridgePort(port); - } - if (ret == true) - { - it = consumer.m_toSync.erase(it); + if (removeVlanMember(vlan, port)) + { + if (port.m_vlan_members.empty()) + { + removeBridgePort(port); + } + it = consumer.m_toSync.erase(it); + } + else + { + it++; + } } else - { - it++; - } + /* Cannot locate the VLAN */ + it = consumer.m_toSync.erase(it); } else { @@ -3107,7 +3131,8 @@ void PortsOrch::doLagTask(Consumer &consumer) switch_id = -1; } - if (!addLag(alias)) + // Create a new LAG when the new alias comes + if (m_portList.find(alias) == m_portList.end()) { if (!addLag(alias, lag_id, switch_id)) { @@ -3646,36 +3671,6 @@ bool PortsOrch::addBridgePort(Port &port) if (port.m_bridge_port_id != SAI_NULL_OBJECT_ID) { - /* If the port is being added to the first VLAN, - * set bridge port admin status to UP. - * This can happen if the port was just removed from - * last VLAN and fdb flush is still in progress. - */ - if (port.m_vlan_members.empty()) - { - if (port.m_fdb_count > 0) - { - return false; - } - sai_attribute_t attr; - attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE; - attr.value.booldata = true; - sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr); - if (status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to set bridge port %s admin status to UP, rv:%d", - port.m_alias.c_str(), status); - return false; - } - port.m_bridge_port_admin_state = true; - m_portList[port.m_alias] = port; - if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP)) - { - SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", - hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_KEEP], port.m_alias.c_str()); - return false; - } - } return true; } @@ -3753,8 +3748,6 @@ bool PortsOrch::addBridgePort(Port &port) } } - port.m_bridge_port_admin_state = true; - if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP)) { SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", @@ -3762,7 +3755,6 @@ bool PortsOrch::addBridgePort(Port &port) return false; } m_portList[port.m_alias] = port; - portOidToName[port.m_bridge_port_id] = port.m_alias; SWSS_LOG_NOTICE("Add bridge port %s to default 1Q bridge", port.m_alias.c_str()); return true; @@ -3776,7 +3768,13 @@ bool PortsOrch::removeBridgePort(Port &port) { return true; } - if (port.m_bridge_port_admin_state) + /* Set bridge port admin status to DOWN */ + sai_attribute_t attr; + attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE; + attr.value.booldata = false; + + sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr); + if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to set bridge port %s admin status to DOWN, rv:%d", port.m_alias.c_str(), status); @@ -3799,7 +3797,7 @@ bool PortsOrch::removeBridgePort(Port &port) SWSS_LOG_INFO("Flush FDB entries for port %s", port.m_alias.c_str()); /* Remove bridge port */ - sai_status_t status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id); + status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id); if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to remove bridge port %s from default 1Q bridge, rv:%d", @@ -3810,7 +3808,6 @@ bool PortsOrch::removeBridgePort(Port &port) return parseHandleSaiStatusFailure(handle_status); } } - portOidToName.erase(port.m_bridge_port_id); port.m_bridge_port_id = SAI_NULL_OBJECT_ID; SWSS_LOG_NOTICE("Remove bridge port %s from default 1Q bridge", port.m_alias.c_str()); @@ -3879,7 +3876,7 @@ bool PortsOrch::addVlan(string vlan_alias) } } - SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu vlan_oid 0x%lx", vlan_alias.c_str(), vlan_id, vlan_oid); + SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu", vlan_alias.c_str(), vlan_id); Port vlan(vlan_alias, Port::VLAN); vlan.m_vlan_info.vlan_oid = vlan_oid; @@ -3887,7 +3884,6 @@ bool PortsOrch::addVlan(string vlan_alias) vlan.m_members = set(); m_portList[vlan_alias] = vlan; m_port_ref_count[vlan_alias] = 0; - portOidToName[vlan_oid] = vlan_alias; return true; } @@ -3895,15 +3891,6 @@ bool PortsOrch::addVlan(string vlan_alias) bool PortsOrch::removeVlan(Port vlan) { SWSS_LOG_ENTER(); - - /* If there are still fdb entries associated with the VLAN, - return false for retry */ - if (vlan.m_fdb_count > 0) - { - SWSS_LOG_NOTICE("VLAN %s still has assiciated FDB entries", vlan.m_alias.c_str()); - return false; - } - if (m_port_ref_count[vlan.m_alias] > 0) { SWSS_LOG_ERROR("Failed to remove ref count %d VLAN %s", @@ -3944,7 +3931,6 @@ bool PortsOrch::removeVlan(Port vlan) SWSS_LOG_NOTICE("Remove VLAN %s vid:%hu", vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id); - portOidToName.erase(vlan.m_vlan_info.vlan_oid); m_portList.erase(vlan.m_alias); m_port_ref_count.erase(vlan.m_alias); @@ -4141,7 +4127,6 @@ bool PortsOrch::addLag(string lag_alias, uint32_t spa_id, int32_t switch_id) lag.m_members = set(); m_portList[lag_alias] = lag; m_port_ref_count[lag_alias] = 0; - portOidToName[lag_id] = lag_alias; PortUpdate update = { lag, true }; notify(SUBJECT_TYPE_PORT_CHANGE, static_cast(&update)); @@ -4196,11 +4181,6 @@ bool PortsOrch::removeLag(Port lag) return false; } - if (lag.m_bridge_port_id != SAI_NULL_OBJECT_ID) - { - return false; - } - sai_status_t status = sai_lag_api->remove_lag(lag.m_lag_id); if (status != SAI_STATUS_SUCCESS) { @@ -4214,7 +4194,6 @@ bool PortsOrch::removeLag(Port lag) SWSS_LOG_NOTICE("Remove LAG %s lid:%" PRIx64, lag.m_alias.c_str(), lag.m_lag_id); - portOidToName.erase(lag.m_lag_id); m_portList.erase(lag.m_alias); m_port_ref_count.erase(lag.m_alias); diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index b6e78f6374..bcd5b5d38d 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -207,11 +207,6 @@ class PortsOrch : public Orch, public Subject map, sai_object_id_t> m_portListLaneMap; map, tuple> m_lanesAliasSpeedMap; map m_portList; - /* mapping from SAI object ID to Name for faster - * retrieval of Port/VLAN from object ID for events - * coming from SAI - */ - unordered_map portOidToName; unordered_map m_portOidToIndex; map m_port_ref_count; unordered_set m_pendingPortSet; From 2ca19c3439dbdc8277b65ada8b4f6b6999fc9f63 Mon Sep 17 00:00:00 2001 From: Dhanasekar Rathinavel Date: Wed, 31 Mar 2021 13:02:39 -0700 Subject: [PATCH 03/11] L2 forwarding enhancements - Invoke flush Fdb entries per port/vlan - Instead of maintaining the vlan-members inside Port structure, added a new portVlanmap for vlan members. /* Performance issue are seen during mac event processing with large number of vlans on a * port. Large data should not be added to port structure. */ - created a portOidToName map to improve performace - delay lag delete until bridge port is deleted - delay bridge port removal until all asociated fdb entries are removed. delete bridge port from portsorch instead of fdborch --- orchagent/fdborch.cpp | 27 +++-- orchagent/port.h | 11 ++- orchagent/portsorch.cpp | 211 ++++++++++++++++++++++++---------------- orchagent/portsorch.h | 7 ++ 4 files changed, 163 insertions(+), 93 deletions(-) diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index cd4888bc1c..9c9cdd2490 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -686,16 +686,15 @@ void FdbOrch::doTask(NotificationConsumer& consumer) std::string data; std::vector values; + Port vlan; + Port port; + consumer.pop(op, data, values); if (&consumer == m_flushNotificationsConsumer) { if (op == "ALL") { - /* - * so far only support flush all the FDB entries - * flush per port and flush per vlan will be added later. - */ status = sai_fdb_api->flush_fdb_entries(gSwitchId, 0, NULL); if (status != SAI_STATUS_SUCCESS) { @@ -706,14 +705,22 @@ void FdbOrch::doTask(NotificationConsumer& consumer) } else if (op == "PORT") { - /*place holder for flush port fdb*/ - SWSS_LOG_ERROR("Received unsupported flush port fdb request"); + if (!m_portsOrch->getPort(data, port)) + { + SWSS_LOG_ERROR("could not locate port from data %s", data.c_str()); + return; + } + flushFDBEntries(port.m_bridge_port_id, SAI_NULL_OBJECT_ID); return; } else if (op == "VLAN") { - /*place holder for flush vlan fdb*/ - SWSS_LOG_ERROR("Received unsupported flush vlan fdb request"); + if (!m_portsOrch->getPort(data, vlan)) + { + SWSS_LOG_ERROR("could not locate vlan from data %s", data.c_str()); + return; + } + flushFDBEntries(SAI_NULL_OBJECT_ID, vlan.m_vlan_info.vlan_oid); return; } else @@ -839,7 +846,9 @@ void FdbOrch::updatePortOperState(const PortOperStateUpdate& update) // Get BVID of each VLAN that this port is a member of // and call notifyObserversFDBFlush - for (const auto& vlan_member: p.m_vlan_members) + vlan_members_t vlan_members; + m_portsOrch->getPortVlanMembers(p, vlan_members); + for (const auto& vlan_member: vlan_members) { swss::Port vlan; string vlan_alias = VLAN_PREFIX + to_string(vlan_member.first); diff --git a/orchagent/port.h b/orchagent/port.h index 86e84a1636..8eb00abe10 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -108,6 +108,7 @@ class Port VlanInfo m_vlan_info; MacAddress m_mac; sai_object_id_t m_bridge_port_id = 0; // TODO: port could have multiple bridge port IDs + sai_object_id_t m_bridge_port_admin_state = 0; // TODO: port could have multiple bridge port IDs sai_vlan_id_t m_port_vlan_id = DEFAULT_PORT_VLAN_ID; // Port VLAN ID sai_object_id_t m_rif_id = 0; sai_object_id_t m_vr_id = 0; @@ -117,7 +118,15 @@ class Port sai_object_id_t m_tunnel_id = 0; 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; + /* there can be a large number of vlan members + * which will cause performance impact, as + * getPort copies the whole structure. + * Performance issue are seen during mac event + * processing with large number of vlans on a + * port. Large data should not be added to this + * structure. + */ + //vlan_members_t m_vlan_members; sai_object_id_t m_parent_port_id = 0; uint32_t m_dependency_bitmap = 0; sai_port_oper_status_t m_oper_status = SAI_PORT_OPER_STATUS_UNKNOWN; diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index d776ee84e7..34c70806cb 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -611,36 +611,13 @@ bool PortsOrch::getPort(sai_object_id_t id, Port &port) { SWSS_LOG_ENTER(); - for (const auto& portIter: m_portList) - { - switch (portIter.second.m_type) - { - case Port::PHY: - case Port::SYSTEM: - if(portIter.second.m_port_id == id) - { - port = portIter.second; - return true; - } - break; - case Port::LAG: - if(portIter.second.m_lag_id == id) - { - port = portIter.second; - return true; - } - break; - case Port::VLAN: - if (portIter.second.m_vlan_info.vlan_oid == id) - { - port = portIter.second; - return true; - } - break; - default: - continue; - } - } + auto itr = portOidToName.find(id); + if (itr == portOidToName.end()) + return false; + else { + getPort(itr->second, port); + return true; + } return false; } @@ -661,13 +638,12 @@ bool PortsOrch::getPortByBridgePortId(sai_object_id_t bridge_port_id, Port &port { SWSS_LOG_ENTER(); - for (auto &it: m_portList) - { - if (it.second.m_bridge_port_id == bridge_port_id) - { - port = it.second; - return true; - } + auto itr = portOidToName.find(bridge_port_id); + if (itr == portOidToName.end()) + return false; + else { + getPort(itr->second, port); + return true; } return false; @@ -2073,6 +2049,7 @@ bool PortsOrch::initPort(const string &alias, const int index, const set &l /* Add port to port list */ m_portList[alias] = p; + portOidToName[id] = alias; m_port_ref_count[alias] = 0; m_portOidToIndex[id] = index; @@ -3034,24 +3011,23 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer) } else if (op == DEL_COMMAND) { + int ret = true; if (vlan.m_members.find(port_alias) != vlan.m_members.end()) { - if (removeVlanMember(vlan, port)) - { - if (port.m_vlan_members.empty()) - { - removeBridgePort(port); - } - it = consumer.m_toSync.erase(it); - } - else - { - it++; - } + ret = removeVlanMember(vlan, port); } - else - /* Cannot locate the VLAN */ + if ((ret == true) && m_portVlanMember[port.m_alias].empty()) + { + ret = removeBridgePort(port); + } + if (ret == true) + { it = consumer.m_toSync.erase(it); + } + else + { + it++; + } } else { @@ -3131,14 +3107,10 @@ void PortsOrch::doLagTask(Consumer &consumer) switch_id = -1; } - // Create a new LAG when the new alias comes - if (m_portList.find(alias) == m_portList.end()) + if (!addLag(alias, lag_id, switch_id)) { - if (!addLag(alias, lag_id, switch_id)) - { - it++; - continue; - } + it++; + continue; } // Process attributes @@ -3671,6 +3643,36 @@ bool PortsOrch::addBridgePort(Port &port) if (port.m_bridge_port_id != SAI_NULL_OBJECT_ID) { + /* If the port is being added to the first VLAN, + * set bridge port admin status to UP. + * This can happen if the port was just removed from + * last VLAN and fdb flush is still in progress. + */ + if (m_portVlanMember[port.m_alias].empty()) + { + if (port.m_fdb_count > 0) + { + return false; + } + sai_attribute_t attr; + attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE; + attr.value.booldata = true; + sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to set bridge port %s admin status to UP, rv:%d", + port.m_alias.c_str(), status); + return false; + } + port.m_bridge_port_admin_state = true; + m_portList[port.m_alias] = port; + if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP)) + { + SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", + hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_KEEP], port.m_alias.c_str()); + return false; + } + } return true; } @@ -3748,6 +3750,8 @@ bool PortsOrch::addBridgePort(Port &port) } } + port.m_bridge_port_admin_state = true; + if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP)) { SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", @@ -3755,6 +3759,7 @@ bool PortsOrch::addBridgePort(Port &port) return false; } m_portList[port.m_alias] = port; + portOidToName[port.m_bridge_port_id] = port.m_alias; SWSS_LOG_NOTICE("Add bridge port %s to default 1Q bridge", port.m_alias.c_str()); return true; @@ -3768,28 +3773,31 @@ bool PortsOrch::removeBridgePort(Port &port) { return true; } - /* Set bridge port admin status to DOWN */ - sai_attribute_t attr; - attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE; - attr.value.booldata = false; - - sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr); - if (status != SAI_STATUS_SUCCESS) + if (port.m_bridge_port_admin_state) { - SWSS_LOG_ERROR("Failed to set bridge port %s admin status to DOWN, rv:%d", - port.m_alias.c_str(), status); - task_process_status handle_status = handleSaiSetStatus(SAI_API_BRIDGE, status); - if (handle_status != task_success) + /* Set bridge port admin status to DOWN */ + sai_attribute_t attr; + attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE; + attr.value.booldata = false; + + sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr); + if (status != SAI_STATUS_SUCCESS) { - return parseHandleSaiStatusFailure(handle_status); + SWSS_LOG_ERROR("Failed to set bridge port %s admin status to DOWN, rv:%d", + port.m_alias.c_str(), status); + task_process_status handle_status = handleSaiSetStatus(SAI_API_BRIDGE, status); + if (handle_status != task_success) + { + return parseHandleSaiStatusFailure(handle_status); + } } - } - if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_STRIP)) - { - SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", - hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_STRIP], port.m_alias.c_str()); - return false; + if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_STRIP)) + { + SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", + hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_STRIP], port.m_alias.c_str()); + return false; + } } //Flush the FDB entires corresponding to the port @@ -3797,7 +3805,7 @@ bool PortsOrch::removeBridgePort(Port &port) SWSS_LOG_INFO("Flush FDB entries for port %s", port.m_alias.c_str()); /* Remove bridge port */ - status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id); + sai_status_t status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id); if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to remove bridge port %s from default 1Q bridge, rv:%d", @@ -3808,6 +3816,7 @@ bool PortsOrch::removeBridgePort(Port &port) return parseHandleSaiStatusFailure(handle_status); } } + portOidToName.erase(port.m_bridge_port_id); port.m_bridge_port_id = SAI_NULL_OBJECT_ID; SWSS_LOG_NOTICE("Remove bridge port %s from default 1Q bridge", port.m_alias.c_str()); @@ -3876,7 +3885,7 @@ bool PortsOrch::addVlan(string vlan_alias) } } - SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu", vlan_alias.c_str(), vlan_id); + SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu vlan_oid 0x%lx", vlan_alias.c_str(), vlan_id, vlan_oid); Port vlan(vlan_alias, Port::VLAN); vlan.m_vlan_info.vlan_oid = vlan_oid; @@ -3884,6 +3893,7 @@ bool PortsOrch::addVlan(string vlan_alias) vlan.m_members = set(); m_portList[vlan_alias] = vlan; m_port_ref_count[vlan_alias] = 0; + portOidToName[vlan_oid] = vlan_alias; return true; } @@ -3891,6 +3901,15 @@ bool PortsOrch::addVlan(string vlan_alias) bool PortsOrch::removeVlan(Port vlan) { SWSS_LOG_ENTER(); + + /* If there are still fdb entries associated with the VLAN, + return false for retry */ + if (vlan.m_fdb_count > 0) + { + SWSS_LOG_NOTICE("VLAN %s still has assiciated FDB entries", vlan.m_alias.c_str()); + return false; + } + if (m_port_ref_count[vlan.m_alias] > 0) { SWSS_LOG_ERROR("Failed to remove ref count %d VLAN %s", @@ -3931,6 +3950,7 @@ bool PortsOrch::removeVlan(Port vlan) SWSS_LOG_NOTICE("Remove VLAN %s vid:%hu", vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id); + portOidToName.erase(vlan.m_vlan_info.vlan_oid); m_portList.erase(vlan.m_alias); m_port_ref_count.erase(vlan.m_alias); @@ -4006,7 +4026,7 @@ bool PortsOrch::addVlanMember(Port &vlan, Port &port, string &tagging_mode) /* a physical port may join multiple vlans */ VlanMemberEntry vme = {vlan_member_id, sai_tagging_mode}; - port.m_vlan_members[vlan.m_vlan_info.vlan_id] = vme; + m_portVlanMember[port.m_alias][vlan.m_vlan_info.vlan_id] = vme; m_portList[port.m_alias] = port; vlan.m_members.insert(port.m_alias); m_portList[vlan.m_alias] = vlan; @@ -4017,16 +4037,22 @@ bool PortsOrch::addVlanMember(Port &vlan, Port &port, string &tagging_mode) return true; } +bool PortsOrch::getPortVlanMembers(Port &port, vlan_members_t &vlan_members) +{ + vlan_members = m_portVlanMember[port.m_alias]; + return true; +} + bool PortsOrch::removeVlanMember(Port &vlan, Port &port) { SWSS_LOG_ENTER(); sai_object_id_t vlan_member_id; sai_vlan_tagging_mode_t sai_tagging_mode; - auto vlan_member = port.m_vlan_members.find(vlan.m_vlan_info.vlan_id); + auto vlan_member = m_portVlanMember[port.m_alias].find(vlan.m_vlan_info.vlan_id); /* Assert the port belongs to this VLAN */ - assert (vlan_member != port.m_vlan_members.end()); + assert (vlan_member != m_portVlanMember[port.m_alias].end()); sai_tagging_mode = vlan_member->second.vlan_mode; vlan_member_id = vlan_member->second.vlan_member_id; @@ -4041,7 +4067,9 @@ bool PortsOrch::removeVlanMember(Port &vlan, Port &port) return parseHandleSaiStatusFailure(handle_status); } } - port.m_vlan_members.erase(vlan_member); + m_portVlanMember[port.m_alias].erase(vlan_member); + if (m_portVlanMember[port.m_alias].empty()) + m_portVlanMember.erase(port.m_alias); SWSS_LOG_NOTICE("Remove member %s from VLAN %s lid:%hx vmid:%" PRIx64, port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id, vlan_member_id); @@ -4076,6 +4104,16 @@ bool PortsOrch::addLag(string lag_alias, uint32_t spa_id, int32_t switch_id) { SWSS_LOG_ENTER(); + auto lagport = m_portList.find(lag_alias); + if (lagport != m_portList.end()) + { + if (m_portList[lag_alias].m_bridge_port_id != SAI_NULL_OBJECT_ID) + { + return false; + } + return true; + } + vector lag_attrs; string system_lag_alias = lag_alias; @@ -4127,6 +4165,7 @@ bool PortsOrch::addLag(string lag_alias, uint32_t spa_id, int32_t switch_id) lag.m_members = set(); m_portList[lag_alias] = lag; m_port_ref_count[lag_alias] = 0; + portOidToName[lag_id] = lag_alias; PortUpdate update = { lag, true }; notify(SUBJECT_TYPE_PORT_CHANGE, static_cast(&update)); @@ -4175,12 +4214,17 @@ bool PortsOrch::removeLag(Port lag) SWSS_LOG_ERROR("Failed to remove non-empty LAG %s", lag.m_alias.c_str()); return false; } - if (lag.m_vlan_members.size() > 0) + if (m_portVlanMember[lag.m_alias].size() > 0) { SWSS_LOG_ERROR("Failed to remove LAG %s, it is still in VLAN", lag.m_alias.c_str()); return false; } + if (lag.m_bridge_port_id != SAI_NULL_OBJECT_ID) + { + return false; + } + sai_status_t status = sai_lag_api->remove_lag(lag.m_lag_id); if (status != SAI_STATUS_SUCCESS) { @@ -4194,6 +4238,7 @@ bool PortsOrch::removeLag(Port lag) SWSS_LOG_NOTICE("Remove LAG %s lid:%" PRIx64, lag.m_alias.c_str(), lag.m_lag_id); + portOidToName.erase(lag.m_lag_id); m_portList.erase(lag.m_alias); m_port_ref_count.erase(lag.m_alias); diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index bcd5b5d38d..2208bf397b 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -143,6 +143,7 @@ class PortsOrch : public Orch, public Subject string m_inbandPortName = ""; bool isInbandPort(const string &alias); bool setVoqInbandIntf(string &alias, string &type); + bool getPortVlanMembers(Port &port, vlan_members_t &vlan_members); private: unique_ptr m_counterTable; @@ -207,6 +208,12 @@ class PortsOrch : public Orch, public Subject map, sai_object_id_t> m_portListLaneMap; map, tuple> m_lanesAliasSpeedMap; map m_portList; + map m_portVlanMember; + /* mapping from SAI object ID to Name for faster + * retrieval of Port/VLAN from object ID for events + * coming from SAI + */ + unordered_map portOidToName; unordered_map m_portOidToIndex; map m_port_ref_count; unordered_set m_pendingPortSet; From 34cec7b594059b74f8d846360775145fe5e33ec8 Mon Sep 17 00:00:00 2001 From: Dhanasekar Rathinavel Date: Tue, 20 Apr 2021 06:20:00 -0700 Subject: [PATCH 04/11] Fix armhf compilation issue --- orchagent/portsorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 34c70806cb..a35e5b28ef 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -3885,7 +3885,7 @@ bool PortsOrch::addVlan(string vlan_alias) } } - SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu vlan_oid 0x%lx", vlan_alias.c_str(), vlan_id, vlan_oid); + SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu vlan_oid 0x%lx", vlan_alias.c_str(), vlan_id, (long unsigned int)vlan_oid); Port vlan(vlan_alias, Port::VLAN); vlan.m_vlan_info.vlan_oid = vlan_oid; From eace766edeab3d599200147f90ed7cef17c2acb4 Mon Sep 17 00:00:00 2001 From: Dhanasekar Rathinavel Date: Thu, 22 Apr 2021 07:45:59 -0700 Subject: [PATCH 05/11] Fix Learn Mode config failure --- orchagent/portsorch.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index a35e5b28ef..4534afce5b 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -3107,10 +3107,13 @@ void PortsOrch::doLagTask(Consumer &consumer) switch_id = -1; } - if (!addLag(alias, lag_id, switch_id)) + if (m_portList.find(alias) == m_portList.end()) { - it++; - continue; + if (!addLag(alias, lag_id, switch_id)) + { + it++; + continue; + } } // Process attributes From 5f166a2bbac6adc53d89f6bef0060105ba328597 Mon Sep 17 00:00:00 2001 From: anilkpan <64167306+anilkpan@users.noreply.github.com> Date: Tue, 26 Oct 2021 23:44:23 -0700 Subject: [PATCH 06/11] updated as per review comments --- orchagent/port.h | 9 --------- orchagent/portsorch.cpp | 7 +++++-- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/orchagent/port.h b/orchagent/port.h index 8eb00abe10..3e0d01e26c 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -118,15 +118,6 @@ class Port sai_object_id_t m_tunnel_id = 0; sai_object_id_t m_ingress_acl_table_group_id = 0; sai_object_id_t m_egress_acl_table_group_id = 0; - /* there can be a large number of vlan members - * which will cause performance impact, as - * getPort copies the whole structure. - * Performance issue are seen during mac event - * processing with large number of vlans on a - * port. Large data should not be added to this - * structure. - */ - //vlan_members_t m_vlan_members; sai_object_id_t m_parent_port_id = 0; uint32_t m_dependency_bitmap = 0; sai_port_oper_status_t m_oper_status = SAI_PORT_OPER_STATUS_UNKNOWN; diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 4534afce5b..df1ce969a5 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -613,8 +613,11 @@ bool PortsOrch::getPort(sai_object_id_t id, Port &port) auto itr = portOidToName.find(id); if (itr == portOidToName.end()) + { return false; - else { + } + else + { getPort(itr->second, port); return true; } @@ -3888,7 +3891,7 @@ bool PortsOrch::addVlan(string vlan_alias) } } - SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu vlan_oid 0x%lx", vlan_alias.c_str(), vlan_id, (long unsigned int)vlan_oid); + SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu vlan_oid:%" PRIx64, vlan_alias.c_str(), vlan_id, (long unsigned int)vlan_oid); Port vlan(vlan_alias, Port::VLAN); vlan.m_vlan_info.vlan_oid = vlan_oid; From 5d0ee12d4623d25520fcdf4705feeed0113e1fd8 Mon Sep 17 00:00:00 2001 From: anilkpan <64167306+anilkpan@users.noreply.github.com> Date: Thu, 28 Oct 2021 17:15:40 -0700 Subject: [PATCH 07/11] Update fdborch.cpp --- orchagent/fdborch.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index e4a7138646..daab3ad52e 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -868,9 +868,6 @@ void FdbOrch::doTask(NotificationConsumer& consumer) Port port; Port vlanPort; - Port vlan; - Port port; - consumer.pop(op, data, values); if (&consumer == m_flushNotificationsConsumer) From 26fdd6c94f52231a808cdc8d5b6963b88dad8fb6 Mon Sep 17 00:00:00 2001 From: anilkpan <47642449+anilkpan@users.noreply.github.com> Date: Tue, 9 Nov 2021 11:45:23 -0800 Subject: [PATCH 08/11] Updated as per review comments --- orchagent/portsorch.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 63263e7281..797ebb7f5c 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -750,7 +750,7 @@ bool PortsOrch::getPort(sai_object_id_t id, Port &port) { getPort(itr->second, port); return true; - } + } return false; } @@ -791,8 +791,11 @@ bool PortsOrch::getPortByBridgePortId(sai_object_id_t bridge_port_id, Port &port auto itr = portOidToName.find(bridge_port_id); if (itr == portOidToName.end()) + { return false; - else { + } + else + { getPort(itr->second, port); return true; } From 3008db60dc2627f523db839c489c72de27635f64 Mon Sep 17 00:00:00 2001 From: anilkpan <47642449+anilkpan@users.noreply.github.com> Date: Wed, 10 Nov 2021 17:06:36 -0800 Subject: [PATCH 09/11] updated as per review comments --- orchagent/orch.cpp | 9 ++- orchagent/portsorch.cpp | 126 +++++++++++++++------------------------- orchagent/portsorch.h | 2 +- 3 files changed, 57 insertions(+), 80 deletions(-) diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index 226512f388..b978dbbc60 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -123,7 +123,14 @@ void Consumer::addToSync(const KeyOpFieldsValuesTuple &entry) } if (iter == ret.second) { - m_toSync.emplace(key, entry); + if(getTableName() == "VLAN_MEMBER_TABLE") + { + m_toSync.erase(key); + } + else + { + m_toSync.emplace(key, entry); + } } else { diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 797ebb7f5c..fdc6704ec1 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -741,8 +741,8 @@ bool PortsOrch::getPort(sai_object_id_t id, Port &port) { SWSS_LOG_ENTER(); - auto itr = portOidToName.find(id); - if (itr == portOidToName.end()) + auto itr = saiOidToAlias.find(id); + if (itr == saiOidToAlias.end()) { return false; } @@ -789,8 +789,8 @@ bool PortsOrch::getPortByBridgePortId(sai_object_id_t bridge_port_id, Port &port { SWSS_LOG_ENTER(); - auto itr = portOidToName.find(bridge_port_id); - if (itr == portOidToName.end()) + auto itr = saiOidToAlias.find(bridge_port_id); + if (itr == saiOidToAlias.end()) { return false; } @@ -2336,7 +2336,7 @@ bool PortsOrch::initPort(const string &alias, const string &role, const int inde /* Add port to port list */ m_portList[alias] = p; - portOidToName[id] = alias; + saiOidToAlias[id] = alias; m_port_ref_count[alias] = 0; m_portOidToIndex[id] = index; @@ -3526,23 +3526,24 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer) } else if (op == DEL_COMMAND) { - int ret = true; if (vlan.m_members.find(port_alias) != vlan.m_members.end()) { - ret = removeVlanMember(vlan, port); - } - if ((ret == true) && m_portVlanMember[port.m_alias].empty()) - { - ret = removeBridgePort(port); - } - if (ret == true) - { - it = consumer.m_toSync.erase(it); + if (removeVlanMember(vlan, port)) + { + if (m_portVlanMember[port.m_alias].empty()) + { + removeBridgePort(port); + } + it = consumer.m_toSync.erase(it); + } + else + { + it++; + } } else - { - it++; - } + /* Cannot locate the VLAN */ + it = consumer.m_toSync.erase(it); } else { @@ -4217,36 +4218,6 @@ bool PortsOrch::addBridgePort(Port &port) if (port.m_bridge_port_id != SAI_NULL_OBJECT_ID) { - /* If the port is being added to the first VLAN, - * set bridge port admin status to UP. - * This can happen if the port was just removed from - * last VLAN and fdb flush is still in progress. - */ - if (m_portVlanMember[port.m_alias].empty()) - { - if (port.m_fdb_count > 0) - { - return false; - } - sai_attribute_t attr; - attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE; - attr.value.booldata = true; - sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr); - if (status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to set bridge port %s admin status to UP, rv:%d", - port.m_alias.c_str(), status); - return false; - } - port.m_bridge_port_admin_state = true; - m_portList[port.m_alias] = port; - if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP)) - { - SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", - hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_KEEP], port.m_alias.c_str()); - return false; - } - } return true; } @@ -4324,8 +4295,6 @@ bool PortsOrch::addBridgePort(Port &port) } } - port.m_bridge_port_admin_state = true; - if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP)) { SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", @@ -4333,7 +4302,7 @@ bool PortsOrch::addBridgePort(Port &port) return false; } m_portList[port.m_alias] = port; - portOidToName[port.m_bridge_port_id] = port.m_alias; + saiOidToAlias[port.m_bridge_port_id] = port.m_alias; SWSS_LOG_NOTICE("Add bridge port %s to default 1Q bridge", port.m_alias.c_str()); PortUpdate update = { port, true }; @@ -4350,31 +4319,28 @@ bool PortsOrch::removeBridgePort(Port &port) { return true; } - if (port.m_bridge_port_admin_state) - { - /* Set bridge port admin status to DOWN */ - sai_attribute_t attr; - attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE; - attr.value.booldata = false; + /* Set bridge port admin status to DOWN */ + sai_attribute_t attr; + attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE; + attr.value.booldata = false; - sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr); - if (status != SAI_STATUS_SUCCESS) + sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to set bridge port %s admin status to DOWN, rv:%d", + port.m_alias.c_str(), status); + task_process_status handle_status = handleSaiSetStatus(SAI_API_BRIDGE, status); + if (handle_status != task_success) { - SWSS_LOG_ERROR("Failed to set bridge port %s admin status to DOWN, rv:%d", - port.m_alias.c_str(), status); - task_process_status handle_status = handleSaiSetStatus(SAI_API_BRIDGE, status); - if (handle_status != task_success) - { - return parseHandleSaiStatusFailure(handle_status); - } + return parseHandleSaiStatusFailure(handle_status); } + } - if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_STRIP)) - { - SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", - hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_STRIP], port.m_alias.c_str()); - return false; - } + if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_STRIP)) + { + SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", + hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_STRIP], port.m_alias.c_str()); + return false; } //Flush the FDB entires corresponding to the port @@ -4382,7 +4348,7 @@ bool PortsOrch::removeBridgePort(Port &port) SWSS_LOG_INFO("Flush FDB entries for port %s", port.m_alias.c_str()); /* Remove bridge port */ - sai_status_t status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id); + status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id); if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to remove bridge port %s from default 1Q bridge, rv:%d", @@ -4393,7 +4359,7 @@ bool PortsOrch::removeBridgePort(Port &port) return parseHandleSaiStatusFailure(handle_status); } } - portOidToName.erase(port.m_bridge_port_id); + saiOidToAlias.erase(port.m_bridge_port_id); port.m_bridge_port_id = SAI_NULL_OBJECT_ID; /* Remove bridge port */ @@ -4477,7 +4443,7 @@ bool PortsOrch::addVlan(string vlan_alias) vlan.m_members = set(); m_portList[vlan_alias] = vlan; m_port_ref_count[vlan_alias] = 0; - portOidToName[vlan_oid] = vlan_alias; + saiOidToAlias[vlan_oid] = vlan_alias; return true; } @@ -4540,7 +4506,7 @@ bool PortsOrch::removeVlan(Port vlan) SWSS_LOG_NOTICE("Remove VLAN %s vid:%hu", vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id); - portOidToName.erase(vlan.m_vlan_info.vlan_oid); + saiOidToAlias.erase(vlan.m_vlan_info.vlan_oid); m_portList.erase(vlan.m_alias); m_port_ref_count.erase(vlan.m_alias); @@ -4934,6 +4900,10 @@ bool PortsOrch::addLag(string lag_alias, uint32_t spa_id, int32_t switch_id) auto lagport = m_portList.find(lag_alias); if (lagport != m_portList.end()) { + /* The deletion of bridgeport attached to the lag may still be + * pending due to fdb entries still present on the lag. Wait + * until the cleanup is done. + */ if (m_portList[lag_alias].m_bridge_port_id != SAI_NULL_OBJECT_ID) { return false; @@ -4992,7 +4962,7 @@ bool PortsOrch::addLag(string lag_alias, uint32_t spa_id, int32_t switch_id) lag.m_members = set(); m_portList[lag_alias] = lag; m_port_ref_count[lag_alias] = 0; - portOidToName[lag_id] = lag_alias; + saiOidToAlias[lag_id] = lag_alias; PortUpdate update = { lag, true }; notify(SUBJECT_TYPE_PORT_CHANGE, static_cast(&update)); @@ -5065,7 +5035,7 @@ bool PortsOrch::removeLag(Port lag) SWSS_LOG_NOTICE("Remove LAG %s lid:%" PRIx64, lag.m_alias.c_str(), lag.m_lag_id); - portOidToName.erase(lag.m_lag_id); + saiOidToAlias.erase(lag.m_lag_id); m_portList.erase(lag.m_alias); m_port_ref_count.erase(lag.m_alias); diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index 0f4d189a55..748f2ea844 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -232,7 +232,7 @@ class PortsOrch : public Orch, public Subject * retrieval of Port/VLAN from object ID for events * coming from SAI */ - unordered_map portOidToName; + unordered_map saiOidToAlias; unordered_map m_portOidToIndex; map m_port_ref_count; unordered_set m_pendingPortSet; From 12be1fb65040e399c28c985b44d4e11f9c18ff42 Mon Sep 17 00:00:00 2001 From: anilkpan <47642449+anilkpan@users.noreply.github.com> Date: Wed, 10 Nov 2021 17:08:41 -0800 Subject: [PATCH 10/11] Update orch.cpp --- orchagent/orch.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index b978dbbc60..226512f388 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -123,14 +123,7 @@ void Consumer::addToSync(const KeyOpFieldsValuesTuple &entry) } if (iter == ret.second) { - if(getTableName() == "VLAN_MEMBER_TABLE") - { - m_toSync.erase(key); - } - else - { - m_toSync.emplace(key, entry); - } + m_toSync.emplace(key, entry); } else { From 902a62119dab3cd6c25c12cb7bae117ba3bb0c07 Mon Sep 17 00:00:00 2001 From: anilkpan <47642449+anilkpan@users.noreply.github.com> Date: Tue, 16 Nov 2021 14:45:19 -0800 Subject: [PATCH 11/11] fix compile issue --- orchagent/portsorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 56b4ea9b37..fde4b5ce57 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -4444,7 +4444,7 @@ bool PortsOrch::addVlan(string vlan_alias) } } - SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu vlan_oid:%" PRIx64, vlan_alias.c_str(), vlan_id, (long unsigned int)vlan_oid); + SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu vlan_oid:%" PRIx64, vlan_alias.c_str(), vlan_id, vlan_oid); Port vlan(vlan_alias, Port::VLAN); vlan.m_vlan_info.vlan_oid = vlan_oid;