Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

[DPB:orchagent:portsorch] Rework port dependency (#23) #1219

Merged
merged 3 commits into from
Mar 20, 2020
Merged

[DPB:orchagent:portsorch] Rework port dependency (#23) #1219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0315b5
Rework port dependency (#23)
vasant17 Mar 3, 2020
dc8477f
Addressed code-review comments
Mar 13, 2020
4d5ee7d
Code-review comments addressed
Mar 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 1 addition & 21 deletions orchagent/port.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,6 @@ class Port
UNKNOWN
} ;

enum Dependency {
ACL_DEP,
FDB_DEP,
INTF_DEP,
LAG_DEP,
VLAN_DEP
};

Port() {};
Port(std::string alias, Type type) :
m_alias(alias), m_type(type) {};
Expand Down Expand Up @@ -84,6 +76,7 @@ class Port
std::string m_learn_mode = "hardware";
bool m_autoneg = false;
bool m_admin_state_up = false;
bool m_init = false;
sai_object_id_t m_port_id = 0;
sai_port_fec_mode_t m_fec_mode = SAI_PORT_FEC_MODE_NONE;
VlanInfo m_vlan_info;
Expand Down Expand Up @@ -120,19 +113,6 @@ class Port

std::unordered_set<sai_object_id_t> m_ingress_acl_tables_uset;
std::unordered_set<sai_object_id_t> m_egress_acl_tables_uset;

inline void set_dependency(Dependency dep)
{
m_dependency_bitmap |= (1 << dep);
}
inline void clear_dependency(Dependency dep)
{
m_dependency_bitmap &= ~(1 << dep);
}
inline bool has_dependency()
{
return (m_dependency_bitmap != 0);
}
};

}
Expand Down
76 changes: 40 additions & 36 deletions orchagent/portsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,6 @@ bool PortsOrch::unbindRemoveAclTableGroup(sai_object_id_t port_oid,
return true;
}

port.clear_dependency(Port::ACL_DEP);
SWSS_LOG_NOTICE("Removing port OID %" PRIx64" ACL table group ID", port_oid);

// Unbind ACL group
Expand Down Expand Up @@ -1094,8 +1093,6 @@ bool PortsOrch::createBindAclTableGroup(sai_object_id_t port_oid,
return false;
}

port.set_dependency(Port::ACL_DEP);

SWSS_LOG_NOTICE("Create %s ACL table group and bind port %s to it",
ingress ? "ingress" : "egress", port.m_alias.c_str());
}
Expand Down Expand Up @@ -1566,30 +1563,20 @@ bool PortsOrch::addPort(const set<int> &lane_set, uint32_t speed, int an, string
return true;
}

bool PortsOrch::removePort(sai_object_id_t port_id)
sai_status_t PortsOrch::removePort(sai_object_id_t port_id)
{
SWSS_LOG_ENTER();

Port p;
if (getPort(port_id, p))
{
PortUpdate update = {p, false };
notify(SUBJECT_TYPE_PORT_CHANGE, static_cast<void *>(&update));
}

sai_status_t status = sai_port_api->remove_port(port_id);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to remove port %" PRIx64 ", rv:%d", port_id, status);
return false;
return status;
}

removeAclTableGroup(p);

m_portCount--;
SWSS_LOG_NOTICE("Remove port %" PRIx64, port_id);

return true;
return status;
}

string PortsOrch::getQueueWatermarkFlexCounterTableKey(string key)
Expand Down Expand Up @@ -1646,6 +1633,8 @@ bool PortsOrch::initPort(const string &alias, const set<int> &lane_set)
PortUpdate update = { p, true };
notify(SUBJECT_TYPE_PORT_CHANGE, static_cast<void *>(&update));

m_portList[alias].m_init = true;

SWSS_LOG_NOTICE("Initialized port %s", alias.c_str());
}
else
Expand Down Expand Up @@ -1678,6 +1667,7 @@ void PortsOrch::deInitPort(string alias, sai_object_id_t port_id)
RedisClient redisClient(m_counter_db.get());
redisClient.hdel(COUNTERS_PORT_NAME_MAP, alias);

m_portList[alias].m_init = false;
SWSS_LOG_NOTICE("De-Initialized port %s", alias.c_str());
}

Expand Down Expand Up @@ -1943,7 +1933,7 @@ void PortsOrch::doPortTask(Consumer &consumer)
{
if (m_lanesAliasSpeedMap.find(it->first) == m_lanesAliasSpeedMap.end())
{
if (!removePort(it->second))
if (SAI_STATUS_SUCCESS != removePort(it->second))
{
throw runtime_error("PortsOrch initialization failure.");
}
Expand Down Expand Up @@ -2264,26 +2254,47 @@ void PortsOrch::doPortTask(Consumer &consumer)
SWSS_LOG_NOTICE("Deleting Port %s", alias.c_str());
auto port_id = m_portList[alias].m_port_id;
auto hif_id = m_portList[alias].m_hif_id;
auto bridge_port_oid = m_portList[alias].m_bridge_port_id;

if (m_portList[alias].has_dependency())
Copy link
Contributor

@qiluo-msft qiluo-msft Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (m_portList[alias].has_dependency()) [](start = 12, length = 39)

We can still keep this if-block. No harm to check dependency in orchagent first. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. we had a lengthy discussion on this with Guhan and Renuka. Will rely on SAI REDIS return code and do NOT maintain any code in orchagent to track the dependency. In-fact I am removing the dependency tracking code (which went in as part of ACL/DPB code) in this PR which

if (bridge_port_oid != SAI_NULL_OBJECT_ID)
{
// Port has one or more dependencies, cannot remove
SWSS_LOG_WARN("Cannot to remove port because of dependency");
// Bridge port OID is set on a port as long as
// port is part of at-least one VLAN.
Copy link
Contributor

@qiluo-msft qiluo-msft Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VLAN [](start = 48, length = 4)

how about vxlan? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool PortsOrch::removePort(sai_object_id_t port_id)

The function behavior is weird:

  1. if removed, return true
  2. if in use, return false
  3. if other error, throw.

Suggest return an enum, or sai_status_t. No throw.

Refers to: orchagent/portsorch.cpp:1569 in ca4812c. [](commit_id = ca4812c, deletion_comment = False)

Half heartedly, I agree. I am NOT too happy because, we always handle the sai_status in the inner most function (where we call sai API's) and return only true of false to the calling function. And its pretty consistent throughout the code and I liked it. But here I am making an exception by returning sai_status to the caller. I did think about introducing our own enum, but that seemed like over kill. So, I will stick to this for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VLAN [](start = 48, length = 4)

how about vxlan?

For VxLAN I do NOT see any direct dependency on physical port. If your question is more about handling tunnel interface dependency on physical port, that is NOT in scope of this PR. Because, we expect every dependency to be tracked in SAI REDIS layer. Please let me know if I dint interpret your comment correctly

// Ideally this should be tracked by SAI redis.
// Until then, let this snippet be here.
SWSS_LOG_WARN("Cannot remove port as bridge port OID is present %lx", bridge_port_oid);
it++;
continue;
}

deInitPort(alias, port_id);
}

SWSS_LOG_NOTICE("Removing hostif %lx for Port %s", hif_id, alias.c_str());
sai_status_t status = sai_hostif_api->remove_hostif(hif_id);
if (status != SAI_STATUS_SUCCESS)
if (m_portList[alias].m_init)
{
throw runtime_error("Remove hostif for the port failed");
deInitPort(alias, port_id);
SWSS_LOG_NOTICE("Removing hostif %lx for Port %s", hif_id, alias.c_str());
sai_status_t status = sai_hostif_api->remove_hostif(hif_id);
if (status != SAI_STATUS_SUCCESS)
{
throw runtime_error("Remove hostif for the port failed");
}

Port p;
if (getPort(port_id, p))
{
PortUpdate update = {p, false };
notify(SUBJECT_TYPE_PORT_CHANGE, static_cast<void *>(&update));
}
}

if (!removePort(port_id))
sai_status_t status = removePort(port_id);
if (SAI_STATUS_SUCCESS != status)
{
throw runtime_error("Delete port failed");
if (SAI_STATUS_OBJECT_IN_USE != status)
{
throw runtime_error("Delete port failed");
}
SWSS_LOG_WARN("Failed to remove port %" PRIx64 ", as the object is in use", port_id);
it++;
continue;
}
removePortFromLanesMap(alias);
removePortFromPortListMap(port_id);
Expand Down Expand Up @@ -2616,13 +2627,6 @@ void PortsOrch::doLagTask(Consumer &consumer)
continue;
}

if (m_portList[alias].has_dependency())
{
// LAG has one or more dependencies, cannot remove
SWSS_LOG_WARN("Cannot to remove LAG because of dependency");
continue;
}

if (removeLag(lag))
it = consumer.m_toSync.erase(it);
else
Expand Down
2 changes: 1 addition & 1 deletion orchagent/portsorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ class PortsOrch : public Orch, public Subject
void getLagMember(Port &lag, vector<Port> &portv);

bool addPort(const set<int> &lane_set, uint32_t speed, int an=0, string fec="");
bool removePort(sai_object_id_t port_id);
sai_status_t removePort(sai_object_id_t port_id);
bool initPort(const string &alias, const set<int> &lane_set);
void deInitPort(string alias, sai_object_id_t port_id);

Expand Down