Enable dynamic TCP receive window resizing #933

XOR-op · 2024-05-23T01:15:47Z

Closes #927.

This PR introduces the ability to resize TCP receive buffer after the connection is established, as well as helper getter functions. This feature will help implement "rwnd auto-tuning", for example in Linux kernel, which can reduce memory footprint when establishing a lot of connections without sacrificing the maximum achievable throughput.

Current requirements include:

The new buffer must be larger than the length of remaining data in the current buffer.
The new buffer must be multiple of (1 << self.remote_win_shift), which is the window scaling negotiated in the handshake.

src/socket/tcp.rs

XOR-op · 2024-07-12T17:26:25Z

@whitequark / @thvdveld Would you mind giving some forms of review? It has been quite some time, but I haven't receive any feedback from maintainers yet.

whitequark · 2024-07-13T04:58:36Z

Ping me again on a workday on UK time and I'll see if I can do it. Ideally Mon/Tue.

XOR-op · 2024-07-15T17:16:26Z

@whitequark Thanks. It's workday today.

whitequark · 2024-07-17T08:22:11Z

src/socket/tcp.rs

+/// Error returned by set_*
+#[derive(Debug, PartialEq, Eq, Clone, Copy)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+pub enum ArgumentError {


I don't think the creation of a new error type that is used only in this API follows our API design principles.

I don't expect a new error type for this API, too. However, I didn't find an appropriate existent error type to add enum variants. Do you have any suggestion for which of the existent {Send, Recv, Listen, Connect}Error should I add variants?

whitequark · 2024-07-17T08:24:00Z

src/socket/tcp.rs

+    /// Return the local receive window scaling factor defined in [RFC 1323].
+    ///
+    /// The value will become constant after the connection is established.
+    /// It may be reset to 0 during the handshake if remote side does not support window scaling.


I don't like that this API has a bidirectional data flow, where a value set by the consumer is read by the TCP/IP stack, but also then is updated by the TCP/IP stack. Is there any precedent (whether in smoltcp or in BSD TCP/IP) to have an option like this? I would there to be two API entry points, one to request an option, one to see if it was applied.

In google's gVisor userspace TCP/IP stack, they have similar options. I believe the issue is that, smoltcp has its default, and we want to override this default. That's why we have such a bidirectional data flow. Our TCP/IP stack reads the value (after connect/accept), and sets the value if no appropriate default value is set (before connect/accept).

As for the "two API entry points", I don't quite get that. Could you give some examples?

whitequark · 2024-07-17T08:24:31Z

src/socket/tcp.rs

+    /// otherwise, the old buffer is returned as an Ok value.
+    ///
+    /// See also the [local_recv_win_scale](struct.Socket.html#method.local_recv_win_scale) methods.
+    pub fn replace_recv_buffer<T: Into<SocketBuffer<'a>>>(


This looks like a generic method that has no direct correspondence to TCP receive window resizing and as such I don't see why it should be a part of this PR at all.

This method does enable the TCP receive window resizing. Smoltcp uses rx_buffer.capacity()-rx_buffer.len() to compute the current receive window. Without this method, it's impossible to adjust rx_buffer.capacity(). By replaced with a larger receive buffer, smoltcp knows it can advertise a larger receive window size safely without correctness issue.

whitequark

There is a number of design issues with this API and I think it needs to be discussed further before being implemented. As it is I don't think it is a good fit for smoltcp.

XOR-op · 2024-07-17T14:36:11Z

I believe some explanations are needed for this PR: the original purpose of this PR is to enable using a larger (or smaller) receive window after a TCP connection is established for better resource utilization. However, in the current implementation of smoltcp, the window scale is automatically computed, which will be the minimum value that can fully utilize the initial receive buffer. Assuming the initial buffer is 4MB, than the auto-computed window scale is log_2(4MB/64K)=6, which means we cannot replace with a buffer > 4MB. Thus we need to have ability to set window scale manually.

TL;DR:

We want to replace receive buffer dynamically (controlled by users).
To make sure we can use a larger buffer than the initial one, window scale should be able to be set manually.

whitequark · 2024-07-17T14:51:28Z

I feel like these two goals shouldn't be addressed in the same PR. For example, given that a buffer is replaced (which I'm still not sure is the right way to handle this, but I'm not saying it's not) why not recompute the window size for the buffer size automatically?

XOR-op · 2024-07-17T15:14:08Z

I feel like these two goals shouldn't be addressed in the same PR. For example, given that a buffer is replaced (which I'm still not sure is the right way to handle this, but I'm not saying it's not) why not recompute the window size for the buffer size automatically?

If the new buffer is always smaller than the initial buffer, than it has been automatically recomputed. But that will be less useful, since in most situations we want to increase the buffer size rather than reduce it. To be able to use a larger buffer after the establishment, we need to allow specifying window scale manually. (I don't add code for automatic re-computation, since that has been implemented even if without this PR.)

Also, to ensure the correctness of replacement, we need to access window scale anyway. That's why I see the two goals are related. And I'm afraid the smaller-only replacement will be accepted as a standalone PR due to lack of real usefulness. But I'll respect your opinion if you still think two PRs are needed.

XOR-op · 2024-08-08T18:32:42Z

@whitequark Can I know your current opinion on our previous discussion (API designs and whether to split PR)? If you insist, I can split this change into two separate PRs (ability to replace with a smaller buffer is the first, ability to replace with a larger buffer & change window scale before establishing connections is the second). Without your comments it's difficult for me to advance this PR. Thanks in advance for your time and effort.

whitequark · 2024-08-08T21:01:47Z

I'm overwhelmed at work currently so I'll only be able to handle this 1-2 weeks later. Feel free to ping me.

XOR-op · 2024-08-26T15:50:44Z

@whitequark It has been two weeks now. Are you not so busy recently?

whitequark · 2024-08-27T12:49:31Z

I'm on vacation currently, so I might be able to look at this PR in depth soon.

codecov · 2024-09-22T03:21:40Z

Codecov Report

Attention: Patch coverage is 86.20690% with 16 lines in your changes missing coverage. Please review.

Project coverage is 80.75%. Comparing base (336e7e5) to head (175a1ef).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/socket/tcp.rs	86.20%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #933      +/-   ##
==========================================
+ Coverage   80.73%   80.75%   +0.02%     
==========================================
  Files          81       81              
  Lines       28335    28450     +115     
==========================================
+ Hits        22877    22976      +99     
- Misses       5458     5474      +16

Flag	Coverage Δ
	`80.75% <86.20%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

XOR-op · 2024-10-07T21:21:56Z

@whitequark It has been another month since our last activity in this PR and hope you well. I think the two unresolved concerns are 1) whether we should add a new Error type; 2) how should we override the default window_scaling for API design. I hope to know thoughts from maintainers given the design challenges I faced, which are important for me to change my implementation.

whitequark · 2024-10-07T21:24:24Z

I'll do my best to address this PR soon.

tomDev5 reviewed Jun 14, 2024

View reviewed changes

src/socket/tcp.rs Outdated Show resolved Hide resolved

XOR-op force-pushed the resize-recv-buffer branch from 0953b05 to bb70b7b Compare July 2, 2024 21:47

XOR-op force-pushed the resize-recv-buffer branch from cdb64c2 to 73ec49c Compare July 11, 2024 03:43

XOR-op changed the title ~~Enable resizing receive buffer after TCP connection is established~~ Enable dymanic TCP receive window resizing Jul 12, 2024

XOR-op force-pushed the resize-recv-buffer branch from c92074f to 1555b5e Compare July 12, 2024 17:08

whitequark changed the title ~~Enable dymanic TCP receive window resizing~~ Enable dynamic TCP receive window resizing Jul 17, 2024

whitequark reviewed Jul 17, 2024

View reviewed changes

whitequark requested changes Jul 17, 2024

View reviewed changes

XOR-op added 10 commits September 21, 2024 23:19

impl: ability to resize tcp receive buffer

09d3f30

impl: new_with_window_scaling will panic

0fd6252

test: resize recv buffer

19f3911

docs: replace_recv_buffer

5ae098c

lints: fix clippy

b9aa203

change: use set_* api for window scale

79f2eb3

lints: fix clippy

c79b8ab

fix: lifetime constraint

f2a6021

fix: configuration overridden by private reset()

6b6c640

fix: docs

175a1ef

XOR-op force-pushed the resize-recv-buffer branch from 1555b5e to 175a1ef Compare September 22, 2024 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable dynamic TCP receive window resizing #933

Enable dynamic TCP receive window resizing #933

XOR-op commented May 23, 2024 •

edited

Loading

XOR-op commented Jul 12, 2024

whitequark commented Jul 13, 2024

XOR-op commented Jul 15, 2024

whitequark Jul 17, 2024

XOR-op Jul 17, 2024

whitequark Jul 17, 2024

XOR-op Jul 17, 2024

whitequark Jul 17, 2024

XOR-op Jul 17, 2024

whitequark left a comment

XOR-op commented Jul 17, 2024

whitequark commented Jul 17, 2024

XOR-op commented Jul 17, 2024

XOR-op commented Aug 8, 2024

whitequark commented Aug 8, 2024

XOR-op commented Aug 26, 2024

whitequark commented Aug 27, 2024

codecov bot commented Sep 22, 2024

XOR-op commented Oct 7, 2024

whitequark commented Oct 7, 2024

Enable dynamic TCP receive window resizing #933

Are you sure you want to change the base?

Enable dynamic TCP receive window resizing #933

Conversation

XOR-op commented May 23, 2024 • edited Loading

XOR-op commented Jul 12, 2024

whitequark commented Jul 13, 2024

XOR-op commented Jul 15, 2024

whitequark Jul 17, 2024

Choose a reason for hiding this comment

XOR-op Jul 17, 2024

Choose a reason for hiding this comment

whitequark Jul 17, 2024

Choose a reason for hiding this comment

XOR-op Jul 17, 2024

Choose a reason for hiding this comment

whitequark Jul 17, 2024

Choose a reason for hiding this comment

XOR-op Jul 17, 2024

Choose a reason for hiding this comment

whitequark left a comment

Choose a reason for hiding this comment

XOR-op commented Jul 17, 2024

whitequark commented Jul 17, 2024

XOR-op commented Jul 17, 2024

XOR-op commented Aug 8, 2024

whitequark commented Aug 8, 2024

XOR-op commented Aug 26, 2024

whitequark commented Aug 27, 2024

codecov bot commented Sep 22, 2024

Codecov Report

XOR-op commented Oct 7, 2024

whitequark commented Oct 7, 2024

XOR-op commented May 23, 2024 •

edited

Loading