Skip to content

[MUX/PFCWD] Use in_ports for acls instead of seperate ACL table #1670

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

Merged
merged 2 commits into from
Mar 17, 2021
Merged

[MUX/PFCWD] Use in_ports for acls instead of seperate ACL table #1670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1ba4ed
Use in_ports for mux acls instead of seperate ACL table
prsunny Mar 11, 2021
d067e07
Address review comment, minor fix
prsunny Mar 17, 2021
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
40 changes: 35 additions & 5 deletions orchagent/aclorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2961,7 +2961,26 @@ bool AclOrch::removeAclRule(string table_id, string rule_id)
return m_AclTables[table_oid].remove(rule_id);
}

bool AclOrch::updateAclRule(shared_ptr<AclRule> rule, string table_id, string attr_name, void *data, bool oper)
AclRule* AclOrch::getAclRule(string table_id, string rule_id)
{
sai_object_id_t table_oid = getTableById(table_id);
if (table_oid == SAI_NULL_OBJECT_ID)
{
SWSS_LOG_INFO("Table %s does not exist", table_id.c_str());
return nullptr;
}

const auto& rule_it = m_AclTables[table_oid].rules.find(rule_id);
if (rule_it == m_AclTables[table_oid].rules.end())
{
SWSS_LOG_INFO("Rule %s doesn't exist", rule_id.c_str());
return nullptr;
}

return rule_it->second.get();
}

bool AclOrch::updateAclRule(string table_id, string rule_id, string attr_name, void *data, bool oper)
{
SWSS_LOG_ENTER();

Expand All @@ -2974,12 +2993,19 @@ bool AclOrch::updateAclRule(shared_ptr<AclRule> rule, string table_id, string at
return false;
}

auto rule_it = m_AclTables[table_oid].rules.find(rule_id);
if (rule_it == m_AclTables[table_oid].rules.end())
{
SWSS_LOG_ERROR("Failed to update ACL rule in ACL table %s. Rule doesn't exist", rule_id.c_str());
return false;
}

switch (aclMatchLookup[attr_name])
{
case SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS:
{
sai_object_id_t port_oid = *(sai_object_id_t *)data;
vector<sai_object_id_t> in_ports = rule->getInPorts();
vector<sai_object_id_t> in_ports = rule_it->second->getInPorts();

if (oper == RULE_OPER_ADD)
{
Expand All @@ -3004,10 +3030,14 @@ bool AclOrch::updateAclRule(shared_ptr<AclRule> rule, string table_id, string at
attr_value += p.m_alias;
attr_value += ',';
}
attr_value.pop_back();

rule->validateAddMatch(MATCH_IN_PORTS, attr_value);
m_AclTables[table_oid].rules[rule->getId()]->updateInPorts();
if (!attr_value.empty())
{
attr_value.pop_back();
}

rule_it->second->validateAddMatch(MATCH_IN_PORTS, attr_value);
rule_it->second->updateInPorts();
}
break;

Expand Down
3 changes: 2 additions & 1 deletion orchagent/aclorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,8 @@ class AclOrch : public Orch, public Observer
bool updateAclTable(AclTable &currentTable, AclTable &newTable);
bool addAclRule(shared_ptr<AclRule> aclRule, string table_id);
bool removeAclRule(string table_id, string rule_id);
bool updateAclRule(shared_ptr<AclRule> aclRule, string table_id, string attr_name, void *data, bool oper);
bool updateAclRule(string table_id, string rule_id, string attr_name, void *data, bool oper);
AclRule* getAclRule(string table_id, string rule_id);

bool isCombinedMirrorV6Table();
bool isAclActionSupported(acl_stage_type_t stage, sai_acl_action_type_t action) const;
Expand Down
72 changes: 59 additions & 13 deletions orchagent/muxorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ extern sai_router_interface_api_t* sai_router_intfs_api;

/* Constants */
#define MUX_TUNNEL "MuxTunnel0"
#define MUX_ACL_TABLE_NAME "mux_acl_table";
#define MUX_ACL_RULE_NAME "mux_acl_rule";
#define MUX_ACL_TABLE_NAME INGRESS_TABLE_DROP
#define MUX_ACL_RULE_NAME "mux_acl_rule"
#define MUX_HW_STATE_UNKNOWN "unknown"
#define MUX_HW_STATE_ERROR "error"

Expand Down Expand Up @@ -202,6 +202,10 @@ static sai_object_id_t create_tunnel(const IpAddress* p_dst_ip, const IpAddress*
attr.value.s32 = SAI_TUNNEL_TTL_MODE_PIPE_MODEL;
tunnel_attrs.push_back(attr);

attr.id = SAI_TUNNEL_ATTR_LOOPBACK_PACKET_ACTION;
attr.value.s32 = SAI_PACKET_ACTION_DROP;
tunnel_attrs.push_back(attr);

if (p_src_ip != nullptr)
{
attr.id = SAI_TUNNEL_ATTR_ENCAP_SRC_IP;
Expand Down Expand Up @@ -343,7 +347,7 @@ bool MuxCable::stateActive()
return false;
}

if (!aclHandler(port.m_port_id, false))
if (!aclHandler(port.m_port_id, mux_name_, false))
{
SWSS_LOG_INFO("Remove ACL drop rule failed for %s", mux_name_.c_str());
return false;
Expand Down Expand Up @@ -373,7 +377,7 @@ bool MuxCable::stateStandby()
return false;
}

if (!aclHandler(port.m_port_id))
if (!aclHandler(port.m_port_id, mux_name_))
{
SWSS_LOG_INFO("Add ACL drop rule failed for %s", mux_name_.c_str());
return false;
Expand Down Expand Up @@ -416,6 +420,7 @@ void MuxCable::setState(string new_state)
}

st_chg_in_progress_ = false;
st_chg_failed_ = false;
SWSS_LOG_INFO("Changed state to %s", new_state.c_str());

return;
Expand All @@ -429,11 +434,11 @@ string MuxCable::getState()
return (muxStateValToString.at(state_));
}

bool MuxCable::aclHandler(sai_object_id_t port, bool add)
bool MuxCable::aclHandler(sai_object_id_t port, string alias, bool add)
{
if (add)
{
acl_handler_ = make_shared<MuxAclHandler>(port);
acl_handler_ = make_shared<MuxAclHandler>(port, alias);
}
else
{
Expand Down Expand Up @@ -685,16 +690,18 @@ sai_object_id_t MuxNbrHandler::getNextHopId(const NextHopKey nhKey)

std::map<std::string, AclTable> MuxAclHandler::acl_table_;

MuxAclHandler::MuxAclHandler(sai_object_id_t port)
MuxAclHandler::MuxAclHandler(sai_object_id_t port, string alias)
{
SWSS_LOG_ENTER();

// There is one handler instance per MUX port
acl_table_type_t table_type = ACL_TABLE_MUX;
acl_table_type_t table_type = ACL_TABLE_DROP;
string table_name = MUX_ACL_TABLE_NAME;
string rule_name = MUX_ACL_RULE_NAME;

port_ = port;
alias_ = alias;

auto found = acl_table_.find(table_name);
if (found == acl_table_.end())
{
Expand All @@ -709,20 +716,44 @@ MuxAclHandler::MuxAclHandler(sai_object_id_t port)
else
{
SWSS_LOG_NOTICE("Binding port %" PRIx64 "", port);
// Otherwise just bind ACL table with the port
found->second.bind(port);

AclRule* rule = gAclOrch->getAclRule(table_name, rule_name);
if (rule == nullptr)
{
shared_ptr<AclRuleMux> newRule =
make_shared<AclRuleMux>(gAclOrch, rule_name, table_name, table_type);
createMuxAclRule(newRule, table_name);
}
else
{
gAclOrch->updateAclRule(table_name, rule_name, MATCH_IN_PORTS, &port, RULE_OPER_ADD);
}
}
}

MuxAclHandler::~MuxAclHandler(void)
{
SWSS_LOG_ENTER();
string table_name = MUX_ACL_TABLE_NAME;
string rule_name = MUX_ACL_RULE_NAME;

SWSS_LOG_NOTICE("Un-Binding port %" PRIx64 "", port_);

auto found = acl_table_.find(table_name);
found->second.unbind(port_);
AclRule* rule = gAclOrch->getAclRule(table_name, rule_name);
if (rule == nullptr)
{
SWSS_LOG_THROW("ACL Rule does not exist for port %s, rule %s", alias_.c_str(), rule_name.c_str());
}
Comment on lines +742 to +746
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we guaranteed there will always be a rule here? If there's no PFC storm and no switchover then won't we throw an unnecessary exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it is guaranteed; In the constructor, we always create an ACL rule and in this destructor, it would be an exception if we don't find the rule. Same is done for pfwdacl as well, @vmittal-msft,

Copy link
Contributor

Choose a reason for hiding this comment

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

AH this is the handler not the orch, I understand now. Thanks!


vector<sai_object_id_t> port_set = rule->getInPorts();
if ((port_set.size() == 1) && (port_set[0] == port_))
{
gAclOrch->removeAclRule(table_name, rule_name);
}
else
{
gAclOrch->updateAclRule(table_name, rule_name, MATCH_IN_PORTS, &port_, RULE_OPER_DELETE);
}
}

void MuxAclHandler::createMuxAclTable(sai_object_id_t port, string strTable)
Expand All @@ -736,7 +767,17 @@ void MuxAclHandler::createMuxAclTable(sai_object_id_t port, string strTable)
assert(inserted.second);

AclTable& acl_table = inserted.first->second;
acl_table.type = ACL_TABLE_MUX;

sai_object_id_t table_oid = gAclOrch->getTableById(strTable);
if (table_oid != SAI_NULL_OBJECT_ID)
{
// DROP ACL table is already created
SWSS_LOG_NOTICE("ACL table %s exists, reuse the same", strTable.c_str());
acl_table = *(gAclOrch->getTableByOid(table_oid));
return;
}

acl_table.type = ACL_TABLE_DROP;
acl_table.id = strTable;
acl_table.link(port);
acl_table.stage = ACL_STAGE_INGRESS;
Expand All @@ -753,6 +794,11 @@ void MuxAclHandler::createMuxAclRule(shared_ptr<AclRuleMux> rule, string strTabl
attr_value = "999";
rule->validateAddPriority(attr_name, attr_value);

// Add MATCH_IN_PORTS as match criteria for ingress table
attr_name = MATCH_IN_PORTS;
attr_value = alias_;
rule->validateAddMatch(attr_name, attr_value);

attr_name = ACTION_PACKET_ACTION;
attr_value = PACKET_ACTION_DROP;
rule->validateAddAction(attr_name, attr_value);
Expand Down
5 changes: 3 additions & 2 deletions orchagent/muxorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class MuxStateOrch;
class MuxAclHandler
{
public:
MuxAclHandler(sai_object_id_t port);
MuxAclHandler(sai_object_id_t port, string alias);
~MuxAclHandler(void);

private:
Expand All @@ -48,6 +48,7 @@ class MuxAclHandler
// class shared dict: ACL table name -> ACL table
static std::map<std::string, AclTable> acl_table_;
sai_object_id_t port_ = SAI_NULL_OBJECT_ID;
string alias_;
};

// IP to nexthop index mapping
Expand Down Expand Up @@ -101,7 +102,7 @@ class MuxCable
bool stateInitActive();
bool stateStandby();

bool aclHandler(sai_object_id_t, bool add = true);
bool aclHandler(sai_object_id_t port, string alias, bool add = true);
bool nbrHandler(bool enable, bool update_routes = true);

string mux_name_;
Expand Down
32 changes: 20 additions & 12 deletions orchagent/pfcactionhandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,6 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue,
SWSS_LOG_ENTER();

acl_table_type_t table_type;
sai_object_id_t table_oid;

string queuestr = to_string(queueId);
m_strRule = "Rule_PfcWdAclHandler_" + queuestr;
Expand All @@ -244,18 +243,15 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue,
}
else
{
table_oid = gAclOrch->getTableById(m_strIngressTable);
map<sai_object_id_t, AclTable> table_map = gAclOrch->getAclTables();
auto rule_iter = table_map[table_oid].rules.find(m_strRule);

if (rule_iter == table_map[table_oid].rules.end())
AclRule* rule = gAclOrch->getAclRule(m_strIngressTable, m_strRule);
if (rule == nullptr)
{
shared_ptr<AclRulePfcwd> newRule = make_shared<AclRulePfcwd>(gAclOrch, m_strRule, m_strIngressTable, table_type);
createPfcAclRule(newRule, queueId, m_strIngressTable, port);
}
else
{
gAclOrch->updateAclRule(rule_iter->second, m_strIngressTable, MATCH_IN_PORTS, &port, RULE_OPER_ADD);
gAclOrch->updateAclRule(m_strIngressTable, m_strRule, MATCH_IN_PORTS, &port, RULE_OPER_ADD);
}
}

Expand All @@ -281,11 +277,13 @@ PfcWdAclHandler::~PfcWdAclHandler(void)
{
SWSS_LOG_ENTER();

sai_object_id_t table_oid = gAclOrch->getTableById(m_strIngressTable);
map<sai_object_id_t, AclTable> table_map = gAclOrch->getAclTables();
auto rule_iter = table_map[table_oid].rules.find(m_strRule);
AclRule* rule = gAclOrch->getAclRule(m_strIngressTable, m_strRule);
if (rule == nullptr)
{
SWSS_LOG_THROW("ACL Rule does not exist for rule %s", m_strRule.c_str());
}

vector<sai_object_id_t> port_set = rule_iter->second->getInPorts();
vector<sai_object_id_t> port_set = rule->getInPorts();
sai_object_id_t port = getPort();

if ((port_set.size() == 1) && (port_set[0] == port))
Expand All @@ -294,7 +292,7 @@ PfcWdAclHandler::~PfcWdAclHandler(void)
}
else
{
gAclOrch->updateAclRule(rule_iter->second, m_strIngressTable, MATCH_IN_PORTS, &port, RULE_OPER_DELETE);
gAclOrch->updateAclRule(m_strIngressTable, m_strRule, MATCH_IN_PORTS, &port, RULE_OPER_DELETE);
}

auto found = m_aclTables.find(m_strEgressTable);
Expand Down Expand Up @@ -323,6 +321,16 @@ void PfcWdAclHandler::createPfcAclTable(sai_object_id_t port, string strTable, b
assert(inserted.second);

AclTable& aclTable = inserted.first->second;

sai_object_id_t table_oid = gAclOrch->getTableById(strTable);
if (ingress && table_oid != SAI_NULL_OBJECT_ID)
{
// DROP ACL table is already created
SWSS_LOG_NOTICE("ACL table %s exists, reuse the same", strTable.c_str());
aclTable = *(gAclOrch->getTableByOid(table_oid));
return;
}

aclTable.link(port);
aclTable.id = strTable;

Expand Down
Loading