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 UDP_GRO #2385

Merged
merged 2 commits into from
Mar 6, 2023
Merged

Add support for UDP_GRO #2385

merged 2 commits into from
Mar 6, 2023

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Mar 6, 2023

Motivation:

Support was added for UDP_SEGMENT in #2372 which allows for large UDP datagrams to be written to a socket by letting the kernel or NIC segment the data across multiple datagrams. This reduces traversals across the network stack which can lead to performance improvements. UDP_GRO is the receive-side counterpart allowing the kernel/NIC to aggregate datagrams and reduce network stack traversals.

Modifications:

  • Add a function in CNIOLinux to check whether UDP_GRO is supported
  • Add the relevant socket and channel options
  • Add tests

Result:

  • UDP_GRO can be enabled where supported and applications may receive large buffers.

Motivation:

Support was added for UDP_SEGMENT in apple#2372 which allows for large UDP
datagrams to be written to a socket by letting the kernel or NIC segment
the data across multiple datagrams. This reduces traversals across the
network stack which can lead to performance improvements. UDP_GRO is the
receive-side counterpart allowing the kernel/NIC to aggregate datagrams
and reduce network stack traversals.

Modifications:

- Add a function in CNIOLinux to check whether UDP_GRO is supported
- Add the relevant socket and channel options
- Add tests

Result:

- UDP_GRO can be enabled where supported and applications may receive
  large buffers.
@glbrntt glbrntt added the semver/minor Adds new public API. label Mar 6, 2023
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Mostly looks great, a few tiny notes!

Sources/CNIOLinux/include/CNIOLinux.h Outdated Show resolved Hide resolved

static func setUDPReceiveOffload(_ enabled: Bool, socket: NIOBSDSocket.Handle) throws {
#if os(Linux)
var isEnabled: CInt = enabled ? 1 : 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be var?

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 think so: option_value in setsockopt takes an UnsafeRawPointer

@Lukasa Lukasa enabled auto-merge (squash) March 6, 2023 15:11
@Lukasa Lukasa merged commit d1fa3e2 into apple:main Mar 6, 2023
@glbrntt glbrntt deleted the gb-gro branch March 6, 2023 15:13
@finagolfin
Copy link
Contributor

These tests are failing for me on Ubuntu 20.04 x86_64 and the CI is not running them, which may be why that was missed:

DatagramChannelTests.testChannelCanReceiveLargeBufferWithGROUsingScalarReads : Test skipped: required false value but got true - UDP_SEGMENT (GSO) is not supported on this platform

This is the error I see when they are run locally:

/home/butta/swift-nio/Tests/NIOPosixTests/DatagramChannelTests.swift:1378: error: DatagramChannelTests.testChannelCanReceiveLargeBufferWithGROUsingScalarReads : XCTAssertEqual failed: ("1000") is not equal to ("10000") -Test Case 'DatagramChannelTests.testChannelCanReceiveLargeBufferWithGROUsingScalarReads' failed (0.007 seconds)

@Lukasa
Copy link
Contributor

Lukasa commented Mar 9, 2023

Thanks for this. We’re tracking this on our side as well, as our internal CI platform has also flagged this regression.

@Lukasa
Copy link
Contributor

Lukasa commented Mar 9, 2023

Also as a breadcrumb, can you provide the output of uname -a?

@finagolfin
Copy link
Contributor

Linux Butta-server 5.4.0-132-generic #148-Ubuntu SMP Mon Oct 17 16:02:06 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

@Lukasa
Copy link
Contributor

Lukasa commented Mar 9, 2023

10/10 for the hostname.

@glbrntt
Copy link
Contributor Author

glbrntt commented Mar 9, 2023

We also see failures on 5.4.

FWIW these tests do pass with a newer kernel (5.15).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants