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

Jump to bottom

[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG #1747

Merged
merged 10 commits into from
Jun 1, 2021
Merged

[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG #1747

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7c2768c
[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG
gechiang May 17, 2021
3522e81
Changed per review comment to move TPID capability query under switch…
gechiang May 19, 2021
37285d1
Resolved merge conflict
gechiang May 26, 2021
9bc5391
Removed unnecessary application DB check before the TPID test begins
gechiang May 26, 2021
88619b0
Fix test case issue
gechiang May 29, 2021
a4855a7
Fix test case issue
gechiang May 29, 2021
88eea45
remove unnecessary check
gechiang May 31, 2021
ef088bc
Merge remote-tracking branch 'upstream/master'
gechiang Jun 1, 2021
090a2b7
Re-enable Portchannel TPID testing after merged latest from master in…
gechiang Jun 1, 2021
67b0f87
Fix missing Producer DB that accidentally deleted on previous merge
gechiang Jun 1, 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
27 changes: 26 additions & 1 deletion cfgmgr/portmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,21 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu)
return true;
}

bool PortMgr::setPortTpid(const string &alias, const string &tpid)
{
stringstream cmd;
string res;

// Set the port TPID in application database to update port TPID
vector<FieldValueTuple> fvs;
FieldValueTuple fv("tpid", tpid);
fvs.push_back(fv);
m_appPortTable.set(alias, fvs);

return true;
}


bool PortMgr::setPortAdminStatus(const string &alias, const bool up)
{
stringstream cmd;
Expand Down Expand Up @@ -102,7 +117,7 @@ void PortMgr::doTask(Consumer &consumer)
continue;
}

string admin_status, mtu, learn_mode;
string admin_status, mtu, learn_mode, tpid;

bool configured = (m_portList.find(alias) != m_portList.end());

Expand Down Expand Up @@ -131,6 +146,10 @@ void PortMgr::doTask(Consumer &consumer)
{
learn_mode = fvValue(i);
}
else if (fvField(i) == "tpid")
{
tpid = fvValue(i);
}
}

if (!mtu.empty())
Expand All @@ -150,6 +169,12 @@ void PortMgr::doTask(Consumer &consumer)
setPortLearnMode(alias, learn_mode);
SWSS_LOG_NOTICE("Configure %s MAC learn mode to %s", alias.c_str(), learn_mode.c_str());
}

if (!tpid.empty())
{
setPortTpid(alias, tpid);
SWSS_LOG_NOTICE("Configure %s TPID to %s", alias.c_str(), tpid.c_str());
}
}
else if (op == DEL_COMMAND)
{
Expand Down
1 change: 1 addition & 0 deletions cfgmgr/portmgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class PortMgr : public Orch

void doTask(Consumer &consumer);
bool setPortMtu(const std::string &alias, const std::string &mtu);
bool setPortTpid(const std::string &alias, const std::string &tpid);
bool setPortAdminStatus(const std::string &alias, const bool up);
bool setPortLearnMode(const std::string &alias, const std::string &learn_mode);
bool isPortStateOk(const std::string &alias);
Expand Down
27 changes: 27 additions & 0 deletions cfgmgr/teammgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ void TeamMgr::doLagTask(Consumer &consumer)
string admin_status = DEFAULT_ADMIN_STATUS_STR;
string mtu = DEFAULT_MTU_STR;
string learn_mode;
string tpid;

for (auto i : kfvFieldsValues(t))
{
Expand Down Expand Up @@ -178,6 +179,11 @@ void TeamMgr::doLagTask(Consumer &consumer)
SWSS_LOG_INFO("Get learn_mode %s",
learn_mode.c_str());
}
else if (fvField(i) == "tpid")
{
tpid = fvValue(i);
SWSS_LOG_INFO("Get TPID %s", tpid.c_str());
}
}

if (m_lagList.find(alias) == m_lagList.end())
Expand All @@ -198,6 +204,11 @@ void TeamMgr::doLagTask(Consumer &consumer)
setLagLearnMode(alias, learn_mode);
SWSS_LOG_NOTICE("Configure %s MAC learn mode to %s", alias.c_str(), learn_mode.c_str());
}
if (!tpid.empty())
{
setLagTpid(alias, tpid);
SWSS_LOG_NOTICE("Configure %s TPID to %s", alias.c_str(), tpid.c_str());
}
}
else if (op == DEL_COMMAND)
{
Expand Down Expand Up @@ -395,6 +406,22 @@ bool TeamMgr::setLagMtu(const string &alias, const string &mtu)
return true;
}

bool TeamMgr::setLagTpid(const string &alias, const string &tpid)
{
SWSS_LOG_ENTER();

vector<FieldValueTuple> fvs;
FieldValueTuple fv("tpid", tpid);
fvs.push_back(fv);
m_appLagTable.set(alias, fvs);

SWSS_LOG_NOTICE("Set port channel %s TPID to %s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please have it in same line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change the logging to same line.

alias.c_str(), tpid.c_str());

return true;
}


bool TeamMgr::setLagLearnMode(const string &alias, const string &learn_mode)
{
// Set the port MAC learn mode in application database
Expand Down
1 change: 1 addition & 0 deletions cfgmgr/teammgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class TeamMgr : public Orch
bool setLagAdminStatus(const std::string &alias, const std::string &admin_status);
bool setLagMtu(const std::string &alias, const std::string &mtu);
bool setLagLearnMode(const std::string &alias, const std::string &learn_mode);
bool setLagTpid(const std::string &alias, const std::string &tpid);


bool isPortEnslaved(const std::string &);
Expand Down
55 changes: 55 additions & 0 deletions orchagent/orchdaemon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@
#include "sairedis.h"
#include "chassisorch.h"

extern "C" {
#include "sai.h"
}
#include "sai_serialize.h"

using namespace std;
using namespace swss;

Expand Down Expand Up @@ -275,6 +280,56 @@ bool OrchDaemon::init()

gMacsecOrch = new MACsecOrch(m_applDb, m_stateDb, macsec_app_tables, gPortsOrch);

Table m_switchTable(stateDbSwitchTable.first, stateDbSwitchTable.second);
// Check if SAI is capable of handling TPID config and store result in StateDB switch capability table
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move this section to switch init and make this part of switchorch class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed per suggestion in my new commit.

vector<FieldValueTuple> fvVector;
sai_status_t status = SAI_STATUS_SUCCESS;
sai_attr_capability_t capability;

// Check if SAI is capable of handling TPID for Port
status = sai_query_attribute_capability(gSwitchId, SAI_OBJECT_TYPE_PORT, SAI_PORT_ATTR_TPID, &capability);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_WARN("Could not query port TPID capability %d", status);
// Since pre-req of TPID support requires querry capability failed, it means TPID not supported
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_PORT_TPID_CAPABLE, "false");
}
else
{
if (capability.set_implemented)
{
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_PORT_TPID_CAPABLE, "true");
}
else
{
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_PORT_TPID_CAPABLE, "false");
}
SWSS_LOG_NOTICE("port TPID capability %d", capability.set_implemented);
}
// Check if SAI is capable of handling TPID for LAG
status = sai_query_attribute_capability(gSwitchId, SAI_OBJECT_TYPE_LAG, SAI_LAG_ATTR_TPID, &capability);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_WARN("Could not query LAG TPID capability %d", status);
// Since pre-req of TPID support requires querry capability failed, it means TPID not supported
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_LAG_TPID_CAPABLE, "false");
}
else
{
if (capability.set_implemented)
{
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_LAG_TPID_CAPABLE, "true");
}
else
{
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_LAG_TPID_CAPABLE, "false");
}
SWSS_LOG_NOTICE("LAG TPID capability %d", capability.set_implemented);
}
m_switchTable.set("switch", fvVector);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use the API SwitchOrch::set_switch_capability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Will change this per your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed per suggestion in my new commit.

}

/*
* The order of the orch list is important for state restore of warm start and
* the queued processing in m_toSync map after gPortsOrch->allPortsReady() is set.
Expand Down
3 changes: 3 additions & 0 deletions orchagent/orchdaemon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@

using namespace swss;

#define SWITCH_CAPABILITY_TABLE_PORT_TPID_CAPABLE "PORT_TPID_CAPABLE"
#define SWITCH_CAPABILITY_TABLE_LAG_TPID_CAPABLE "LAG_TPID_CAPABLE"

class OrchDaemon
{
public:
Expand Down
7 changes: 7 additions & 0 deletions orchagent/port.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ extern "C" {
*/
#define DEFAULT_MTU 1492

/*
* Default TPID is 8100
* User can configure other values such as 9100, 9200, or 88A8
*/
#define DEFAULT_TPID 0x8100

#define VNID_NONE 0xFFFFFFFF

namespace swss {
Expand Down Expand Up @@ -128,6 +134,7 @@ class Port
std::vector<sai_object_id_t> m_priority_group_ids;
sai_port_priority_flow_control_mode_t m_pfc_asym = SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_COMBINED;
uint8_t m_pfc_bitmask = 0;
uint16_t m_tpid = DEFAULT_TPID;
uint32_t m_nat_zone_id = 0;
uint32_t m_vnid = VNID_NONE;
uint32_t m_fdb_count = 0;
Expand Down
113 changes: 113 additions & 0 deletions orchagent/portsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,6 +917,36 @@ bool PortsOrch::setPortMtu(sai_object_id_t id, sai_uint32_t mtu)
return true;
}


bool PortsOrch::setPortTpid(sai_object_id_t id, sai_uint16_t tpid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we combine this with Lag to have single function and pass is_lag as boolean since only two lines are different and rest are the same? Just a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a chance that we may end up having to implement the LAG handling by having Orchagent apply the LAG TPID config to all its LAG members. This is because currently only one SAI vendor supports TPID setting on Port only and not yet for LAG. For our fanout switch usage, this is already good enough. But if we ever want to apply it to LAG while SAI vendor not supporting it, it will be much easier to have the port/LAG handling having its own function to do the handling. So for now, let's keep the way it is just in case the enhancement is needed in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok

{
SWSS_LOG_ENTER();
sai_status_t status = SAI_STATUS_SUCCESS;
sai_attribute_t attr;

attr.id = SAI_PORT_ATTR_TPID;

attr.value.u16 = (uint16_t)tpid;

status = sai_port_api->set_port_attribute(id, &attr);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to set TPID 0x%x to port pid:%" PRIx64 ", rv:%d",
attr.value.u16, id, status);
task_process_status handle_status = handleSaiSetStatus(SAI_API_PORT, status);
if (handle_status != task_success)
{
return parseHandleSaiStatusFailure(handle_status);
}
}
else
{
SWSS_LOG_NOTICE("Set TPID 0x%x to port pid:%" PRIx64, attr.value.u16, id);
}
return true;
}


bool PortsOrch::setPortFec(Port &port, sai_port_fec_mode_t mode)
{
SWSS_LOG_ENTER();
Expand Down Expand Up @@ -2353,6 +2383,8 @@ void PortsOrch::doPortTask(Consumer &consumer)
string learn_mode;
int an = -1;
int index = -1;
string tpid_string;
uint16_t tpid = 0;

for (auto i : kfvFieldsValues(t))
{
Expand Down Expand Up @@ -2384,6 +2416,15 @@ void PortsOrch::doPortTask(Consumer &consumer)
{
mtu = (uint32_t)stoul(fvValue(i));
}
/* Set port TPID */
if (fvField(i) == "tpid")
{
tpid_string = fvValue(i);
// Need to get rid of the leading 0x
tpid_string.erase(0,2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it better to use str.find and then str.erase(0, pos)? Do we always expect a 0x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CLI enforces that the TPID values must be one of the following: "0x8100", "0x9100", "0x9200", or "0x88A8".
So yes it will always have the "0x" which we can get rid of.

tpid = (uint16_t)stoi(tpid_string, 0, 16);
SWSS_LOG_DEBUG("Handling TPID to 0x%x, string value:%s", tpid, tpid_string.c_str());
}
/* Set port speed */
else if (fvField(i) == "speed")
{
Expand Down Expand Up @@ -2699,6 +2740,22 @@ void PortsOrch::doPortTask(Consumer &consumer)
}
}

if (tpid != 0 && tpid != p.m_tpid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we reset? I think the check should be just tpid != p.m_tpid, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case "tpid !=0" check serves the purpose of identifying whether it is handling non-TPID operations in which case variable tpid is initialized to "0" and there is no need to perform TPID operation. This is inline with how MTU and speed setting is being handled. Hope this explains why this check is required.

{
SWSS_LOG_DEBUG("Set port %s TPID to 0x%x", alias.c_str(), tpid);
if (setPortTpid(p.m_port_id, tpid))
{
p.m_tpid = tpid;
m_portList[alias] = p;
}
else
{
SWSS_LOG_ERROR("Failed to set port %s TPID to 0x%x", alias.c_str(), tpid);
it++;
continue;
}
}

if (!fec_mode.empty())
{
if (fec_mode_map.find(fec_mode) != fec_mode_map.end())
Expand Down Expand Up @@ -3151,6 +3208,8 @@ void PortsOrch::doLagTask(Consumer &consumer)
string operation_status;
uint32_t lag_id = 0;
int32_t switch_id = -1;
string tpid_string;
uint16_t tpid = 0;

for (auto i : kfvFieldsValues(t))
{
Expand Down Expand Up @@ -3180,6 +3239,14 @@ void PortsOrch::doLagTask(Consumer &consumer)
{
switch_id = stoi(fvValue(i));
}
else if (fvField(i) == "tpid")
{
tpid_string = fvValue(i);
// Need to get rid of the leading 0x
tpid_string.erase(0,2);
tpid = (uint16_t)stoi(tpid_string, 0, 16);
SWSS_LOG_DEBUG("reading TPID string:%s to uint16: 0x%x", tpid_string.c_str(), tpid);
}
}

if (table_name == CHASSIS_APP_LAG_TABLE_NAME)
Expand Down Expand Up @@ -3236,6 +3303,23 @@ void PortsOrch::doLagTask(Consumer &consumer)
updateChildPortsMtu(l, mtu);
}

if (tpid != 0)
{
if (tpid != l.m_tpid)
{
if(!setLagTpid(l.m_lag_id, tpid))
{
SWSS_LOG_ERROR("Failed to set LAG %s TPID 0x%x", alias.c_str(), tpid);
}
else
{
SWSS_LOG_DEBUG("Set LAG %s TPID to 0x%x", alias.c_str(), tpid);
l.m_tpid = tpid;
m_portList[alias] = l;
}
}
}

if (!learn_mode.empty() && (l.m_learn_mode != learn_mode))
{
if (l.m_bridge_port_id != SAI_NULL_OBJECT_ID)
Expand Down Expand Up @@ -4441,6 +4525,35 @@ bool PortsOrch::removeLagMember(Port &lag, Port &port)
return true;
}

bool PortsOrch::setLagTpid(sai_object_id_t id, sai_uint16_t tpid)
{
SWSS_LOG_ENTER();
sai_status_t status = SAI_STATUS_SUCCESS;
sai_attribute_t attr;

attr.id = SAI_LAG_ATTR_TPID;

attr.value.u16 = (uint16_t)tpid;

status = sai_lag_api->set_lag_attribute(id, &attr);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to set TPID 0x%x to LAG pid:%" PRIx64 ", rv:%d",
attr.value.u16, id, status);
task_process_status handle_status = handleSaiSetStatus(SAI_API_LAG, status);
if (handle_status != task_success)
{
return parseHandleSaiStatusFailure(handle_status);
}
}
else
{
SWSS_LOG_NOTICE("Set TPID 0x%x to LAG pid:%" PRIx64 , attr.value.u16, id);
}
return true;
}


bool PortsOrch::setCollectionOnLagMember(Port &lagMember, bool enableCollection)
{
/* Port must be LAG member */
Expand Down
Loading