smoltcp icon indicating copy to clipboard operation
smoltcp copied to clipboard

Enable dynamic TCP receive window resizing

Open XOR-op opened this issue 1 year ago • 13 comments

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:

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

XOR-op avatar May 23 '24 01:05 XOR-op

@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.

XOR-op avatar Jul 12 '24 17:07 XOR-op

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

whitequark avatar Jul 13 '24 04:07 whitequark

@whitequark Thanks. It's workday today.

XOR-op avatar Jul 15 '24 17:07 XOR-op

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:

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

XOR-op avatar Jul 17 '24 14:07 XOR-op

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?

whitequark avatar Jul 17 '24 14:07 whitequark

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 avatar Jul 17 '24 15:07 XOR-op

@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.

XOR-op avatar Aug 08 '24 18:08 XOR-op

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 avatar Aug 08 '24 21:08 whitequark

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

XOR-op avatar Aug 26 '24 15:08 XOR-op

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

whitequark avatar Aug 27 '24 12:08 whitequark

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.

codecov[bot] avatar Sep 22 '24 03:09 codecov[bot]

@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.

XOR-op avatar Oct 07 '24 21:10 XOR-op

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

whitequark avatar Oct 07 '24 21:10 whitequark