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

Add support for jumbo frames #277

Merged
merged 1 commit into from
Mar 7, 2021
Merged

Add support for jumbo frames #277

merged 1 commit into from
Mar 7, 2021

Conversation

lesliemonis
Copy link
Contributor

@lesliemonis lesliemonis commented Jan 24, 2021

Support for Jumbo Frames.

This PR adds support for the manager to receive jumbo frames with Ethernet payload upto 9600 bytes. It also ensures that there is enough memory allocated by the manager to process the same number of packets as before. This enhancement is made optional and can be enabled by using a flag (mentioned below) when executing the go.sh script.

Summary:

Usage:
New flag: -j Allow ports to send and receive jumbo frames

This PR includes
Resolves issues
Breaking API changes
Internal API changes
Usability improvements
Bug fixes
New functionality
New NF/onvm_mgr args Yes
Changes to starting NFs
Dependency updates
Web stats updates

Merging notes:

  • Dependencies: None

TODO before merging :

  • PR is ready for review

Test Plan:

The PR was tested using the following setup:

Topology

Node1--------Node2--------Node3

OpenNetVM was run on Node2 along with the l2fwd example NF.

Commands:

The manager was run using the following command:

./go.sh -k 3 -n 0xF8 -m 0,1,2 -s stdout -j

The l2fwd NF was run using the following command:

./go.sh 1 -n

The MTU on the interfaces of Node1 and Node2 was set to 9000 bytes using the following command:

# ip link set <interface_name> mtu 9000

The support for jumbo frames was tested by running a ping test from Node1 to Node3 using the following command on Node1:

$ ping <ip_address> -M do -s 8972

Correct working was verified by obtaining a successful ping and observing the packets being processed in the manager and NF.

pktgen-dpdk was also employed to test these changes. pktgen-dpdk was run on Node1 and Node3, and the size of the frames in the traffic was set to 9618 bytes.

Command used to run pktgen-dpdk:

# ./pktgen -l 0-2 -n 2 -- -T -P -j -m "[1:2].0"

pktgen cli command to set frame size:

Pktgen:/> set 0 size 9618

--- Update ---

Updated description
Added pktgen-dpdk test plan

Review:

Subscribers: @twood02 @kkrama

@onvm
Copy link

onvm commented Jan 24, 2021

In response to PR creation

CI Message

Aborting, need an authorized user to run CI

Copy link
Member

@twood02 twood02 left a comment

Choose a reason for hiding this comment

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

Please change the variable name, but otherwise it looks good.

Can you provide us with the commands we can use to test this? - nevermind I see this is in the writeup

@lesliemonis
Copy link
Contributor Author

Please change the variable name

Done

@lesliemonis lesliemonis requested a review from twood02 January 24, 2021 20:27
@@ -392,6 +399,11 @@ init_port(uint8_t port_num) {

rte_eth_promiscuous_enable(port_num);

if (ONVM_USE_JUMBO_FRAMES)
rte_eth_dev_set_mtu(port_num, 9000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this configuration enough for enabling jumbo packet support? I see some rx port configuration DEV_RX_OFFLOAD_JUMBO_FRAME in this Feature list [1]? Also, you can check the usage of this configuration in [2] example.

[1] https://dpdk.readthedocs.io/en/v17.11/nics/features.html
[2] https://github.com/DPDK/dpdk/blob/main/examples/ipsec-secgw/ipsec-secgw.c#L2175

Copy link
Member

@dennisafa dennisafa Mar 6, 2021

Choose a reason for hiding this comment

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

The DEV_RX_OFFLOAD_JUMBO_FRAME flag offloads some handling of the jumbo frame to the physical NIC. I'm guessing what it does is forces the hardware to assemble the jumbo frame instead of the PMD (correct me if I'm wrong @twood02 ). This could be used to increase performance. I don't think it's necessary to enable this flag in order to support receiving jumbo frames. We should check if the NIC PMD we are using actually supports the MTU that is passed in. DPDK will tell you if the PMD doesn't have a set_mtu function or if it doesn't support the MTU: https://github.com/DPDK/dpdk/blob/main/lib/librte_ethdev/rte_ethdev.c#L3482
Please add a check to the return value of rte_eth_dev_set_mtu here, and exit the program if the jumbo frame flag is set but is not supported by the NIC.

Copy link
Contributor Author

@lesliemonis lesliemonis Mar 6, 2021

Choose a reason for hiding this comment

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

Is this configuration enough for enabling jumbo packet support? I see some rx port configuration DEV_RX_OFFLOAD_JUMBO_FRAME in this Feature list [1]? Also, you can check the usage of this configuration in [2] example.

[1] https://dpdk.readthedocs.io/en/v17.11/nics/features.html
[2] https://github.com/DPDK/dpdk/blob/main/examples/ipsec-secgw/ipsec-secgw.c#L2175

I tried the solution in [2] and it seems like the DEV_RX_OFFLOAD_JUMBO_FRAME offload flag does the job without the need to use rte_eth_dev_set_mtu() provided the value of rxmode.max_rx_pkt_len is also set accordingly in the port configuration. The code in [3] shows that the MTU is implicitly set when the offload is in use.

The DEV_RX_OFFLOAD_JUMBO_FRAME flag offloads some handling of the jumbo frame to the physical NIC. I'm guessing what it does is forces the hardware to assemble the jumbo frame instead of the PMD (correct me if I'm wrong @twood02 ). This could be used to increase performance. I don't think it's necessary to enable this flag in order to support receiving jumbo frames. We should check if the NIC PMD we are using actually supports the MTU that is passed in. DPDK will tell you if the PMD doesn't have a set_mtu function or if it doesn't support the MTU: https://github.com/DPDK/dpdk/blob/main/lib/librte_ethdev/rte_ethdev.c#L3482
Please add a check to the return value of rte_eth_dev_set_mtu here, and exit the program if the jumbo frame flag is set but is not supported by the NIC.

Yes, using the offload flag isn't necessary in order to support receiving jumbo frames. However, using it seems like the better way to go. The code in [3] checks whether the device would support the desired jumbo frame length (9618 in this case).

The support for jumbo frames in pktgen-dpdk is quite similar to what is done in [2]. I've updated the PR to match the method and values used in this patch for pktgen-dpdk [4].

[3] https://github.com/DPDK/dpdk/blob/2a2ebeab9e3309ab332baefdbcc1caa7e62ecc82/lib/librte_ethdev/rte_ethdev.c#L1408
[4] http://git.dpdk.org/apps/pktgen-dpdk/commit/app?id=ffc2494bdfcaadb8885b8afcc40be33cf8971372

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with the offload flag - there's a chance that some NIC's won't support this, so we should be wary of that. Btw, how did you test that jumbo frames are properly received and processed? Could you post replication steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with the offload flag - there's a chance that some NIC's won't support this, so we should be wary of that. Btw, how did you test that jumbo frames are properly received and processed? Could you post replication steps?

Yes, I'm not sure how an unsupported NIC would react to this flag. It looks like some of the sample applications and pktgen-dpdk follow this approach when enabling jumbo frames.

I tested the recent changes using pktgen-dpdk for traffic generation and the l2fwd example NF for packet processing. I have updated the PR description (Test Plan) to include the commands that I used.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

@@ -392,6 +399,11 @@ init_port(uint8_t port_num) {

rte_eth_promiscuous_enable(port_num);

if (ONVM_USE_JUMBO_FRAMES)
rte_eth_dev_set_mtu(port_num, 9000);
Copy link
Member

@dennisafa dennisafa Mar 6, 2021

Choose a reason for hiding this comment

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

The DEV_RX_OFFLOAD_JUMBO_FRAME flag offloads some handling of the jumbo frame to the physical NIC. I'm guessing what it does is forces the hardware to assemble the jumbo frame instead of the PMD (correct me if I'm wrong @twood02 ). This could be used to increase performance. I don't think it's necessary to enable this flag in order to support receiving jumbo frames. We should check if the NIC PMD we are using actually supports the MTU that is passed in. DPDK will tell you if the PMD doesn't have a set_mtu function or if it doesn't support the MTU: https://github.com/DPDK/dpdk/blob/main/lib/librte_ethdev/rte_ethdev.c#L3482
Please add a check to the return value of rte_eth_dev_set_mtu here, and exit the program if the jumbo frame flag is set but is not supported by the NIC.

Add support for ports to send and receive packets with
length upto 9600 bytes. Use the -j option when running the
manager to enable this support.
@lesliemonis lesliemonis requested a review from dennisafa March 6, 2021 23:07
Copy link
Member

@dennisafa dennisafa left a comment

Choose a reason for hiding this comment

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

Looks good to me - we check the ret val of init_port so we'll be able to catch an unsupported NIC config.

@dennisafa dennisafa merged commit ea11806 into sdnfv:develop Mar 7, 2021
@lesliemonis lesliemonis deleted the jumbo-frames branch March 7, 2021 19:34
@twood02 twood02 added this to the ONVM 21 Summer Release milestone Aug 23, 2021
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