Enable dynamic TCP receive window resizing
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.
@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.
Ping me again on a workday on UK time and I'll see if I can do it. Ideally Mon/Tue.
@whitequark Thanks. It's workday today.
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.
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?
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.
@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.
I'm overwhelmed at work currently so I'll only be able to handle this 1-2 weeks later. Feel free to ping me.
@whitequark It has been two weeks now. Are you not so busy recently?
I'm on vacation currently, so I might be able to look at this PR in depth soon.
Codecov Report
Attention: Patch coverage is 86.20690% with 16 lines in your changes missing coverage. Please review.
Project coverage is 80.90%. Comparing base (
74bd1e5) to head (f29fa12).
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/socket/tcp.rs | 86.20% | 16 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #933 +/- ##
==========================================
+ Coverage 80.88% 80.90% +0.02%
==========================================
Files 81 81
Lines 28452 28567 +115
==========================================
+ Hits 23013 23112 +99
- Misses 5439 5455 +16
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@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.
I'll do my best to address this PR soon.