Expose UDP_MAX_SEGMENTS via System #2891

rnro · 2024-09-19T13:16:09Z

Motivation:

Linux kernels recently increased the value of UDP_MAX_SEGMENTS from 64 to 128 causing hardcoded assumptions in our tests to become invalid.

Modifications:

Surface the value of UDP_MAX_SEGMENTS if it is defined on the platform in use via new public API System.udpMaxSegments() (otherwise return nil) and make use of it in DatagramChannelTests.

System.udpMaxSegments() is added as new public API in analogue to e.g. System.supportsUDPReceiveOffload.

Result:

DatagramChannelTests pass and new API.

FranzBusch · 2024-09-19T13:53:27Z

Sources/NIOCore/Utilities.swift

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


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

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.

I've changed this to a static var.

FranzBusch · 2024-09-19T13:53:34Z

Sources/NIOCore/Utilities.swift

@@ -258,3 +258,18 @@ extension System {
    public static let supportsUDPReceiveOffload: Bool = false
    #endif
 }
+
+extension System {


Wy is this in a new extension?

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

I've consolidated them now that I'm only adding one thing.

Motivation: Linux kernels recently increased the value of `UDP_MAX_SEGMENTS` from 64 to 128 causing hardcoded assumptions in our tests to become invalid. Modifications: Surface the value of `UDP_MAX_SEGMENTS` if it is defined on the platform in use via new public API `System.udpMaxSegments()` (otherwise return `nil`) and make use of it in `DatagramChannelTests`. `System.udpMaxSegments()` is added as new public API in analogue to e.g. `System.supportsUDPReceiveOffload`. Result: `DatagramChannelTests` pass and new API.

Lukasa · 2024-09-19T15:34:57Z

Sources/NIOCore/Utilities.swift

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


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.

Lukasa · 2024-09-19T15:35:17Z

Sources/CNIOLinux/shim.c

+#ifdef UDP_MAX_SEGMENTS
+const unsigned long CNIOLinux_UDP_MAX_SEGMENTS = UDP_MAX_SEGMENTS;
+#endif
+const unsigned long CNIOLinux_UDP_MAX_SEGMENTS = 0;


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

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.

Lukasa · 2024-09-19T16:29:22Z

Sources/NIOCore/Utilities.swift

+    /// Returns the UDP maximum segment count if the platform supports and defines it.
+    public static var udpMaxSegments: Int? {
+        #if os(Linux)
+        if CNIOLinux_UDP_MAX_SEGMENTS != -1 {


Mild nit: I suspect the optimizer will end up calling this function twice, as it doesn't know what it does. I recommend that you save the value to a temporary and branch from there.

I think I've addressed this but your comment isn't 100% clear to me.

Lukasa

Nice one, LGTM.

rnro added the 🆕 semver/minor Adds new public API. label Sep 19, 2024

rnro force-pushed the use_UDP_MAX_SEGMENTS branch 2 times, most recently from c9803ba to 9804b0e Compare September 19, 2024 13:44

FranzBusch reviewed Sep 19, 2024

View reviewed changes

rnro force-pushed the use_UDP_MAX_SEGMENTS branch from 9804b0e to 500c7e1 Compare September 19, 2024 13:53

Lukasa reviewed Sep 19, 2024

View reviewed changes

review comments

f1fc284

rnro force-pushed the use_UDP_MAX_SEGMENTS branch from 2cf2def to f1fc284 Compare September 20, 2024 08:30

Lukasa approved these changes Sep 20, 2024

View reviewed changes

Lukasa merged commit 9f6c011 into apple:main Sep 20, 2024
28 of 29 checks passed

rnro deleted the use_UDP_MAX_SEGMENTS branch September 20, 2024 10:25

rnro mentioned this pull request Sep 20, 2024

GitHub Actions CI fails on DatagramChannelTests.testWriteBufferAboveGSOSegmentCountLimit consistently #2889

Closed

Lukasa mentioned this pull request Sep 20, 2024

Build errors when building against a Static Linux SDK #2893

Closed

0xTim mentioned this pull request Jan 16, 2025

Add ChannelPipeline.SynchronousOperations.Position #3065

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose UDP_MAX_SEGMENTS via System #2891

Expose UDP_MAX_SEGMENTS via System #2891

rnro commented Sep 19, 2024

FranzBusch Sep 19, 2024

Lukasa Sep 19, 2024

rnro Sep 19, 2024

FranzBusch Sep 19, 2024

rnro Sep 19, 2024

rnro Sep 19, 2024

Lukasa Sep 19, 2024

Lukasa Sep 19, 2024

rnro Sep 19, 2024

Lukasa Sep 19, 2024

rnro Sep 20, 2024

Lukasa left a comment

Expose UDP_MAX_SEGMENTS via System #2891

Expose UDP_MAX_SEGMENTS via System #2891

Conversation

rnro commented Sep 19, 2024

Motivation:

Modifications:

Result:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment