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

[macsec]: Set MTU for MACsec #2398

Merged
merged 5 commits into from
Aug 16, 2022
Merged

[macsec]: Set MTU for MACsec #2398

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
16cae21
Set mtu for MACsec
Pterosaur Jul 26, 2022
b8f618c
Support port mtu update w/o gearbox
jimmyzhai Jul 27, 2022
e55a065
Update fake_portorch.cpp
jimmyzhai Jul 27, 2022
e3ce7ca
Merge branch 'master' into fix_mtu_macsec
Pterosaur Jul 30, 2022
9ec471a
Fix issue if MTU wasn't set
Pterosaur Aug 3, 2022
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
4 changes: 4 additions & 0 deletions orchagent/macsecorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1275,6 +1275,8 @@ bool MACsecOrch::createMACsecPort(
phy);
});

m_port_orch->setMACsecEnabledState(port_id, true);

judyjoseph marked this conversation as resolved.
Show resolved Hide resolved
if (phy)
{
if (!setPFCForward(port_id, true))
Expand Down Expand Up @@ -1542,6 +1544,8 @@ bool MACsecOrch::deleteMACsecPort(
result &= false;
}

m_port_orch->setMACsecEnabledState(port_id, false);

if (phy)
{
if (!setPFCForward(port_id, false))
Expand Down
8 changes: 4 additions & 4 deletions orchagent/p4orch/tests/fake_portorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ bool PortsOrch::getPortAdminStatus(sai_object_id_t id, bool &up)
return true;
}

bool PortsOrch::setPortMtu(sai_object_id_t id, sai_uint32_t mtu)
bool PortsOrch::setPortMtu(const Port &port, sai_uint32_t mtu)
{
return true;
}
Expand Down Expand Up @@ -562,12 +562,12 @@ bool PortsOrch::getPortSpeed(sai_object_id_t port_id, sai_uint32_t &speed)
return true;
}

bool PortsOrch::setGearboxPortsAttr(Port &port, sai_port_attr_t id, void *value)
bool PortsOrch::setGearboxPortsAttr(const Port &port, sai_port_attr_t id, void *value)
{
return true;
}

bool PortsOrch::setGearboxPortAttr(Port &port, dest_port_type_t port_type, sai_port_attr_t id, void *value)
bool PortsOrch::setGearboxPortAttr(const Port &port, dest_port_type_t port_type, sai_port_attr_t id, void *value)
{
return true;
}
Expand Down Expand Up @@ -689,4 +689,4 @@ void PortsOrch::voqSyncDelLagMember(Port &lag, Port &port)
std::unordered_set<std::string> PortsOrch::generateCounterStats(const string &type, bool gearbox)
{
return {};
}
}
65 changes: 57 additions & 8 deletions orchagent/portsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1135,27 +1135,35 @@ bool PortsOrch::getPortAdminStatus(sai_object_id_t id, bool &up)
return true;
}

bool PortsOrch::setPortMtu(sai_object_id_t id, sai_uint32_t mtu)
bool PortsOrch::setPortMtu(const Port& port, sai_uint32_t mtu)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we changing this API? Please have it as the original one and have the caller pass the extra value.

Copy link
Contributor Author

@Pterosaur Pterosaur Aug 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Due to the Gearbox architecture support, the original function cannot semantically support the functionality of set MTU. We just follow other existing functions, like set Speed, set Fec and so on, that are needed by Gearbox platform.

sonic-swss/orchagent/portsorch.cpp

Lines 2191 to 2209 in 9ec471a

task_process_status PortsOrch::setPortSpeed(Port &port, sai_uint32_t speed)
{
sai_attribute_t attr;
sai_status_t status;
SWSS_LOG_ENTER();
attr.id = SAI_PORT_ATTR_SPEED;
attr.value.u32 = speed;
status = sai_port_api->set_port_attribute(port.m_port_id, &attr);
if (status != SAI_STATUS_SUCCESS)
{
return handleSaiSetStatus(SAI_API_PORT, status);
}
setGearboxPortsAttr(port, SAI_PORT_ATTR_SPEED, &speed);
return task_success;
}

sonic-swss/orchagent/portsorch.cpp

Lines 1253 to 1277 in 9ec471a

bool PortsOrch::setPortFec(Port &port, sai_port_fec_mode_t mode)
{
SWSS_LOG_ENTER();
sai_attribute_t attr;
attr.id = SAI_PORT_ATTR_FEC_MODE;
attr.value.s32 = mode;
sai_status_t status = sai_port_api->set_port_attribute(port.m_port_id, &attr);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to set fec mode %d to port pid:%" PRIx64, mode, port.m_port_id);
task_process_status handle_status = handleSaiSetStatus(SAI_API_PORT, status);
if (handle_status != task_success)
{
return parseHandleSaiStatusFailure(handle_status);
}
}
SWSS_LOG_INFO("Set fec mode %d to port pid:%" PRIx64, mode, port.m_port_id);
setGearboxPortsAttr(port, SAI_PORT_ATTR_FEC_MODE, &mode);
return true;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, from the changes I still think you can reuse the existing function. But I see your point as making it consistent across other set functions. Approving from my side.

{
SWSS_LOG_ENTER();

sai_attribute_t attr;
attr.id = SAI_PORT_ATTR_MTU;
/* mtu + 14 + 4 + 4 = 22 bytes */
attr.value.u32 = (uint32_t)(mtu + sizeof(struct ether_header) + FCS_LEN + VLAN_TAG_LEN);
mtu += (uint32_t)(sizeof(struct ether_header) + FCS_LEN + VLAN_TAG_LEN);
attr.value.u32 = mtu;

sai_status_t status = sai_port_api->set_port_attribute(id, &attr);
if (isMACsecPort(port.m_port_id))
{
attr.value.u32 += MAX_MACSEC_SECTAG_SIZE;
}

sai_status_t status = sai_port_api->set_port_attribute(port.m_port_id, &attr);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to set MTU %u to port pid:%" PRIx64 ", rv:%d",
attr.value.u32, id, status);
attr.value.u32, port.m_port_id, status);
task_process_status handle_status = handleSaiSetStatus(SAI_API_PORT, status);
if (handle_status != task_success)
{
return parseHandleSaiStatusFailure(handle_status);
}
}
SWSS_LOG_INFO("Set MTU %u to port pid:%" PRIx64, attr.value.u32, id);

judyjoseph marked this conversation as resolved.
Show resolved Hide resolved
setGearboxPortsAttr(port, SAI_PORT_ATTR_MTU, &mtu);
SWSS_LOG_INFO("Set MTU %u to port pid:%" PRIx64, attr.value.u32, port.m_port_id);
return true;
}

Expand Down Expand Up @@ -1988,7 +1996,7 @@ void PortsOrch::initPortSupportedSpeeds(const std::string& alias, sai_object_id_
/*
* If Gearbox is enabled and this is a Gearbox port then set the attributes accordingly.
*/
bool PortsOrch::setGearboxPortsAttr(Port &port, sai_port_attr_t id, void *value)
bool PortsOrch::setGearboxPortsAttr(const Port &port, sai_port_attr_t id, void *value)
{
bool status = false;

Expand All @@ -2006,7 +2014,7 @@ bool PortsOrch::setGearboxPortsAttr(Port &port, sai_port_attr_t id, void *value)
* If Gearbox is enabled and this is a Gearbox port then set the specific lane attribute.
* Note: the appl_db is also updated (Gearbox config_db tables are TBA).
*/
bool PortsOrch::setGearboxPortAttr(Port &port, dest_port_type_t port_type, sai_port_attr_t id, void *value)
bool PortsOrch::setGearboxPortAttr(const Port &port, dest_port_type_t port_type, sai_port_attr_t id, void *value)
{
sai_status_t status = SAI_STATUS_SUCCESS;
sai_object_id_t dest_port_id;
Expand Down Expand Up @@ -2060,6 +2068,15 @@ bool PortsOrch::setGearboxPortAttr(Port &port, dest_port_type_t port_type, sai_p
}
SWSS_LOG_NOTICE("BOX: Set %s lane %s %d", port.m_alias.c_str(), speed_attr.c_str(), speed);
break;
case SAI_PORT_ATTR_MTU:
attr.id = id;
attr.value.u32 = *static_cast<sai_uint32_t*>(value);
if (LINE_PORT_TYPE == port_type && isMACsecPort(dest_port_id))
{
attr.value.u32 += MAX_MACSEC_SECTAG_SIZE;
}
SWSS_LOG_NOTICE("BOX: Set %s MTU %d", port.m_alias.c_str(), attr.value.u32);
break;
default:
return false;
}
Expand Down Expand Up @@ -3269,7 +3286,7 @@ void PortsOrch::doPortTask(Consumer &consumer)

if (mtu != 0 && mtu != p.m_mtu)
{
if (setPortMtu(p.m_port_id, mtu))
if (setPortMtu(p, mtu))
{
p.m_mtu = mtu;
m_portList[alias] = p;
Expand Down Expand Up @@ -6653,6 +6670,8 @@ bool PortsOrch::initGearboxPort(Port &port)
SWSS_LOG_NOTICE("BOX: Connected Gearbox ports; system-side:0x%" PRIx64 " to line-side:0x%" PRIx64, systemPort, linePort);
m_gearboxPortListLaneMap[port.m_port_id] = make_tuple(systemPort, linePort);
port.m_line_side_id = linePort;
saiOidToAlias[systemPort] = port.m_alias;
saiOidToAlias[linePort] = port.m_alias;

/* Add gearbox system/line port name map to counter table */
FieldValueTuple tuple(port.m_alias + "_system", sai_serialize_object_id(systemPort));
Expand Down Expand Up @@ -7154,3 +7173,33 @@ bool PortsOrch::decrFdbCount(const std::string& alias, int count)
}
return true;
}

void PortsOrch::setMACsecEnabledState(sai_object_id_t port_id, bool enabled)
{
SWSS_LOG_ENTER();

Port p;
if (!getPort(port_id, p))
{
SWSS_LOG_ERROR("Failed to get port object for port id 0x%" PRIx64, port_id);
return;
}

if (enabled)
{
m_macsecEnabledPorts.insert(port_id);
}
else
{
m_macsecEnabledPorts.erase(port_id);
}

setPortMtu(p, p.m_mtu);
}

bool PortsOrch::isMACsecPort(sai_object_id_t port_id) const
{
SWSS_LOG_ENTER();

return m_macsecEnabledPorts.find(port_id) != m_macsecEnabledPorts.end();
}
12 changes: 8 additions & 4 deletions orchagent/portsorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#define FCS_LEN 4
#define VLAN_TAG_LEN 4
#define MAX_MACSEC_SECTAG_SIZE 32
#define PORT_STAT_COUNTER_FLEX_COUNTER_GROUP "PORT_STAT_COUNTER"
#define PORT_RATE_COUNTER_FLEX_COUNTER_GROUP "PORT_RATE_COUNTER"
#define PORT_BUFFER_DROP_STAT_FLEX_COUNTER_GROUP "PORT_BUFFER_DROP_STAT"
Expand Down Expand Up @@ -174,6 +175,9 @@ class PortsOrch : public Orch, public Subject

bool decrFdbCount(const string& alias, int count);

void setMACsecEnabledState(sai_object_id_t port_id, bool enabled);
bool isMACsecPort(sai_object_id_t port_id) const;

private:
unique_ptr<Table> m_counterTable;
unique_ptr<Table> m_counterLagTable;
Expand Down Expand Up @@ -301,7 +305,7 @@ class PortsOrch : public Orch, public Subject

bool setPortAdminStatus(Port &port, bool up);
bool getPortAdminStatus(sai_object_id_t id, bool& up);
bool setPortMtu(sai_object_id_t id, sai_uint32_t mtu);
bool setPortMtu(const Port& port, sai_uint32_t mtu);
bool setPortTpid(sai_object_id_t id, sai_uint16_t tpid);
bool setPortPvid (Port &port, sai_uint32_t pvid);
bool getPortPvid(Port &port, sai_uint32_t &pvid);
Expand All @@ -316,8 +320,8 @@ class PortsOrch : public Orch, public Subject
void initPortSupportedSpeeds(const std::string& alias, sai_object_id_t port_id);
task_process_status setPortSpeed(Port &port, sai_uint32_t speed);
bool getPortSpeed(sai_object_id_t id, sai_uint32_t &speed);
bool setGearboxPortsAttr(Port &port, sai_port_attr_t id, void *value);
bool setGearboxPortAttr(Port &port, dest_port_type_t port_type, sai_port_attr_t id, void *value);
bool setGearboxPortsAttr(const Port &port, sai_port_attr_t id, void *value);
bool setGearboxPortAttr(const Port &port, dest_port_type_t port_type, sai_port_attr_t id, void *value);

task_process_status setPortAdvSpeeds(sai_object_id_t port_id, std::vector<sai_uint32_t>& speed_list);

Expand Down Expand Up @@ -374,8 +378,8 @@ class PortsOrch : public Orch, public Subject
void voqSyncAddLagMember(Port &lag, Port &port);
void voqSyncDelLagMember(Port &lag, Port &port);
unique_ptr<LagIdAllocator> m_lagIdAllocator;
set<sai_object_id_t> m_macsecEnabledPorts;

std::unordered_set<std::string> generateCounterStats(const string& type, bool gearbox = false);

};
#endif /* SWSS_PORTSORCH_H */