Skip to content

Conversation

SRIKKANTH
Copy link
Collaborator

There is a need for testing network perf with different MTU sizes.
This change enables it.

@LiliDeng LiliDeng requested a review from Copilot June 12, 2025 14:13
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables users to pass custom MTU values for Ntttcp network performance tests.

  • Updated test functions in networkperf.py to accept an additional variables dictionary parameter.
  • Modified perf_ntttcp in common.py to extract and apply the MTU from the provided variables.
  • Updated Ntttcp performance message creation and dataclasses in ntttcp.py and messages.py to include client and server MTU fields.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
microsoft/testsuites/performance/networkperf.py Added variables parameter to test case functions for Ntttcp performance testing.
microsoft/testsuites/performance/common.py Introduced MTU extraction from variables and applied MTU settings for default and sriov NICs.
lisa/tools/ntttcp.py Updated performance message functions to include client and server MTU parameters.
lisa/messages.py Added MTU fields to the network performance message dataclasses.

@SRIKKANTH
Copy link
Collaborator Author

@LiliDeng please let me know if anything more needs to be done on this PR.

@squirrelsc
Copy link
Member

Please hold off on this change for now. We're currently adding a unified message for ntttcp. Once that's merged, please add the new metrics to the unified message.

#3863

@SRIKKANTH SRIKKANTH force-pushed the smyakam/custom_mtu_netperf/06_12_2025 branch from 0918e8d to 7783b9f Compare June 20, 2025 07:21
@squirrelsc
Copy link
Member

Please hold off on this change for now. We're currently adding a unified message for ntttcp. Once that's merged, please add the new metrics to the unified message.

#3863

#3863 is merged, please update the PR to enable mtu for unified ntttcp messages too.

@SRIKKANTH SRIKKANTH force-pushed the smyakam/custom_mtu_netperf/06_12_2025 branch from 7783b9f to c9262bb Compare September 11, 2025 07:56
@SRIKKANTH SRIKKANTH marked this pull request as draft September 11, 2025 07:57
@SRIKKANTH SRIKKANTH force-pushed the smyakam/custom_mtu_netperf/06_12_2025 branch 2 times, most recently from 96bf6c5 to 84f05d6 Compare September 15, 2025 11:09
@SRIKKANTH SRIKKANTH marked this pull request as ready for review September 15, 2025 11:20
Allow user passed MTUs for ntttcp network perf tests.

There is a need for testing network perf with different MTU sizes.
This change enables it.
@SRIKKANTH SRIKKANTH force-pushed the smyakam/custom_mtu_netperf/06_12_2025 branch from 8c96840 to f580493 Compare September 15, 2025 11:23
@SRIKKANTH SRIKKANTH marked this pull request as draft September 18, 2025 05:17
@SRIKKANTH SRIKKANTH marked this pull request as ready for review September 18, 2025 08:54
@SRIKKANTH SRIKKANTH force-pushed the smyakam/custom_mtu_netperf/06_12_2025 branch from b4872d4 to ad4c1bf Compare September 18, 2025 09:00
# typical MTU values are between 576 and 9000
# 576 is the minimum possible MTU for IPv4 and 1280 for IPv6
# 9000 is a common value for jumbo frames on Ethernet networks
return 576 <= mtu <= 9000
Copy link
Member

Choose a reason for hiding this comment

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

If someone sets the value to 570 but it runs with a different value, it should raise an exception and fail the case instead of silently using the default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

set_mtu has this logic already.

Copy link
Member

Choose a reason for hiding this comment

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

Based on the logic, if the MTU is out of range, the function returns false and sets MTU to None, which is unexpected. If you want set_mtu to fail via command, remove the check here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants