Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Expose UDP_MAX_SEGMENTS via System #2891

Merged
merged 2 commits into from
Sep 20, 2024
+29 −5
Merged

Expose UDP_MAX_SEGMENTS via System #2891

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
500c7e1
Expose UDP_MAX_SEGMENTS via System
rnro Sep 19, 2024
f1fc284
review comments
rnro Sep 19, 2024
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
5 changes: 4 additions & 1 deletion Sources/CNIOLinux/include/CNIOLinux.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
#include <errno.h>
#include <pthread.h>
#include <netinet/ip.h>
#include <netinet/udp.h>
#include <linux/udp.h>
#include <linux/vm_sockets.h>
#include <fcntl.h>
#include <fts.h>
Expand Down Expand Up @@ -143,5 +143,8 @@ extern const unsigned int CNIOLinux_RENAME_EXCHANGE;
extern const unsigned long CNIOLinux_UTIME_OMIT;
extern const unsigned long CNIOLinux_UTIME_NOW;

bool CNIOLinux_udp_max_segments_defined();
extern const unsigned long CNIOLinux_UDP_MAX_SEGMENTS;

#endif
#endif
13 changes: 13 additions & 0 deletions Sources/CNIOLinux/shim.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,4 +215,17 @@ const int CNIOLinux_AT_EMPTY_PATH = AT_EMPTY_PATH;
const unsigned long CNIOLinux_UTIME_OMIT = UTIME_OMIT;
const unsigned long CNIOLinux_UTIME_NOW = UTIME_NOW;


bool CNIOLinux_udp_max_segments_defined() {
#ifdef UDP_MAX_SEGMENTS
return true;
#else
return false;
#endif
}

#ifdef UDP_MAX_SEGMENTS
const unsigned long CNIOLinux_UDP_MAX_SEGMENTS = UDP_MAX_SEGMENTS;
#endif
const unsigned long CNIOLinux_UDP_MAX_SEGMENTS = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than have max_segments_defined, we should probably just return a sentinel value. -1 is good.

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 normally try to avoid sentinel values as a rule but given we know that this value should comfortably fit within an Int64 I can live with it here, particularly if it might help the optimizer see through this logic.

#endif
15 changes: 15 additions & 0 deletions Sources/NIOCore/Utilities.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,3 +258,18 @@ extension System {
public static let supportsUDPReceiveOffload: Bool = false
#endif
}

extension System {
Copy link
Member

Choose a reason for hiding this comment

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

Wy is this in a new extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for separation of groups of related code - but I'm happy to lump it together if you'd rather.

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've consolidated them now that I'm only adding one thing.

/// Returns the UDP maximum segment count if the platform supports and defines it.
public static func udpMaxSegments() -> Int? {
Copy link
Member

Choose a reason for hiding this comment

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

This should really be a static let that's computed on first access

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that's true: the cost of this function is pretty close to the cost of the dispatch_once, so we might prefer a static 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've changed this to a static var.

#if os(Linux)
if CNIOLinux_udp_max_segments_defined() {
return Int(CNIOLinux_UDP_MAX_SEGMENTS)
} else {
return nil
}
#else
return nil
#endif
}
}
14 changes: 10 additions & 4 deletions Tests/NIOPosixTests/DatagramChannelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1495,7 +1495,10 @@ class DatagramChannelTests: XCTestCase {
func testWriteBufferAtGSOSegmentCountLimit() throws {
try XCTSkipUnless(System.supportsUDPSegmentationOffload, "UDP_SEGMENT (GSO) is not supported on this platform")

var segments = 64
let udpMaxSegments = System.udpMaxSegments() ?? 64

var segments = udpMaxSegments

let segmentSize = 10
let didSet = self.firstChannel.setOption(.datagramSegmentSize, value: CInt(segmentSize))
XCTAssertNoThrow(try didSet.wait())
Expand All @@ -1514,7 +1517,7 @@ class DatagramChannelTests: XCTestCase {
self.firstChannel = try self.buildChannel(group: self.group)
let didSet = self.firstChannel.setOption(.datagramSegmentSize, value: CInt(segmentSize))
XCTAssertNoThrow(try didSet.wait())
segments = 61
segments = udpMaxSegments - 3
try send(byteCount: segments * segmentSize)
}

Expand All @@ -1525,13 +1528,16 @@ class DatagramChannelTests: XCTestCase {
func testWriteBufferAboveGSOSegmentCountLimitShouldError() throws {
try XCTSkipUnless(System.supportsUDPSegmentationOffload, "UDP_SEGMENT (GSO) is not supported on this platform")

// commonly 64 or 128 on systems which may or may not define UDP_MAX_SEGMENTS, pick the larger to ensure failure
let udpMaxSegments = System.udpMaxSegments() ?? 128

let segmentSize = 10
let didSet = self.firstChannel.setOption(.datagramSegmentSize, value: CInt(segmentSize))
XCTAssertNoThrow(try didSet.wait())

let buffer = self.firstChannel.allocator.buffer(repeating: 1, count: segmentSize * 65)
let buffer = self.firstChannel.allocator.buffer(repeating: 1, count: segmentSize * udpMaxSegments + 1)
let writeData = AddressedEnvelope(remoteAddress: self.secondChannel.localAddress!, data: buffer)
// The kernel limits messages to a maximum of 64 segments; any more should result in an error.
// The kernel limits messages to a maximum of UDP_MAX_SEGMENTS segments; any more should result in an error.
XCTAssertThrowsError(try self.firstChannel.writeAndFlush(NIOAny(writeData)).wait()) {
XCTAssert($0 is IOError)
}
Expand Down
Loading