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.

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

[macsec]: Set MTU for MACsec #2398

Merged
merged 5 commits into from
Aug 16, 2022
Merged

Conversation

Pterosaur
Copy link
Contributor

@Pterosaur Pterosaur commented Jul 26, 2022

Signed-off-by: Ze Gan ganze718@gmail.com

What I did
Set extra MTU for MACsec enabled port.

Why I did it
MACsec frame will expend the packet with MACsec SecTAG, Otherwise if a packet length equals the MTU which will be dropped by SAI port.

How I verified it
Assume 9100 is the MTU, Check by the command ping -s 9100 10.0.0.57

Details if related

@Pterosaur Pterosaur force-pushed the fix_mtu_macsec branch 5 times, most recently from 8edfd13 to dd12a28 Compare July 26, 2022 08:42
Signed-off-by: Ze Gan <ganze718@gmail.com>
@Pterosaur Pterosaur marked this pull request as ready for review July 26, 2022 11:23
@Pterosaur Pterosaur requested a review from prsunny as a code owner July 26, 2022 11:23
@@ -1144,6 +1172,11 @@ bool PortsOrch::setPortMtu(sai_object_id_t id, sai_uint32_t mtu)
/* mtu + 14 + 4 + 4 = 22 bytes */
attr.value.u32 = (uint32_t)(mtu + sizeof(struct ether_header) + FCS_LEN + VLAN_TAG_LEN);

if (isMACsecPort(id))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely needed for non-gearbox based devices.
As discussed, do we need this additional 32 bytes to be added for the gbsyncd based devices as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we also needed it for line_side_port for gearbox. @jimmyzhai will add more on this PR for adapting gearbox mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have updated the fix to update port (w/o gearbox) MTU for MACsec.

Copy link
Contributor Author

@Pterosaur Pterosaur left a comment

Choose a reason for hiding this comment

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

lgtm

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 3, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@Pterosaur
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Ze Gan <ganze718@gmail.com>
@Pterosaur
Copy link
Contributor Author

/easycla

Copy link
Contributor

@judyjoseph judyjoseph left a comment

Choose a reason for hiding this comment

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

LGTM

@judyjoseph
Copy link
Contributor

@prsunny could you take a quick look too. thanks.

return true;
}

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

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.

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;
}

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.

@Pterosaur Pterosaur requested a review from prsunny August 13, 2022 02:01
return true;
}

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.

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.

@Pterosaur Pterosaur merged commit cfcf3d8 into sonic-net:master Aug 16, 2022
yxieca pushed a commit that referenced this pull request Aug 17, 2022
What I did
Set extra MTU for MACsec enabled port.

Why I did it
MACsec frame will expend the packet with MACsec SecTAG, Otherwise if a packet length equals the MTU which will be dropped by SAI port.

Signed-off-by: Ze Gan <ganze718@gmail.com>
Co-authored-by: Junhua Zhai <junhua.zhai@outlook.com>
yxieca pushed a commit that referenced this pull request Aug 18, 2022
What I did
Set extra MTU for MACsec enabled port.

Why I did it
MACsec frame will expend the packet with MACsec SecTAG, Otherwise if a packet length equals the MTU which will be dropped by SAI port.

Signed-off-by: Ze Gan <ganze718@gmail.com>
Co-authored-by: Junhua Zhai <junhua.zhai@outlook.com>
@yxieca
Copy link
Contributor

yxieca commented Aug 19, 2022

This change causes 202205 PR test to fail.

@Pterosaur
Copy link
Contributor Author

This change causes 202205 PR test to fail.

This PR: sonic-net/sonic-sairedis#1107 is for fixing the failure.

dprital added a commit to dprital/sonic-buildimage that referenced this pull request Aug 29, 2022
Update sonic-swss submodule pointer to include the following:
* Dynamic port configuration - add port buffer cfg to the port ref counter ([sonic-net#2194](sonic-net/sonic-swss#2194))
* tlm_teamd: Filter portchannel subinterface events from STATE_DB LAG_TABLE ([sonic-net#2408](sonic-net/sonic-swss#2408))
* [counters] Improve performance by polling only configured ports buffer queue/pg counters ([sonic-net#2360](sonic-net/sonic-swss#2360))
* added support for Xsight platform ([sonic-net#2426](sonic-net/sonic-swss#2426))
* [ci][asan] add DVS tests run with ASAN ([sonic-net#2363](sonic-net/sonic-swss#2363))
* Handle dual ToR neighbor miss scenario ([sonic-net#2151](sonic-net/sonic-swss#2151))
* Upstream new development on p4orch ([sonic-net#2237](sonic-net/sonic-swss#2237))
* [lgtm] Fix dependency ([sonic-net#2419](sonic-net/sonic-swss#2419))
* [muxorch] Returning true if nbr in skip_neighbor_ in isNeighborActive() ([sonic-net#2415](sonic-net/sonic-swss#2415))
* [macsec]: Set MTU for MACsec ([sonic-net#2398](sonic-net/sonic-swss#2398))
* Delete Invalid if condition in intfsorch.cpp ([sonic-net#2411](sonic-net/sonic-swss#2411))

Signed-off-by: dprital <drorp@nvidia.com>
Pterosaur added a commit to Pterosaur/sonic-swss that referenced this pull request Sep 23, 2022
What I did
Set extra MTU for MACsec enabled port.

Why I did it
MACsec frame will expend the packet with MACsec SecTAG, Otherwise if a packet length equals the MTU which will be dropped by SAI port.

Signed-off-by: Ze Gan <ganze718@gmail.com>
Co-authored-by: Junhua Zhai <junhua.zhai@outlook.com>
Pterosaur added a commit that referenced this pull request Sep 24, 2022
**What I did**
Set extra MTU for MACsec enabled port.

**Why I did it**
MACsec frame will expend the packet with MACsec SecTAG, Otherwise if a packet length equals the MTU which will be dropped by SAI port.

**How I verified it**
Assume 9100 is the MTU, Check by the command `ping -s 9100 10.0.0.57`

**Details if related**
Cherry-pick from #2398
Signed-off-by: Ze Gan <ganze718@gmail.com>
Co-authored-by: Junhua Zhai <junhua.zhai@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants