Skip to content

consomme: make per-connection TCP buffer sizes configurable#3595

Closed
benhillis wants to merge 2 commits into
microsoft:mainfrom
benhillis:user/benhillis/consomme-tcp-buffer-size
Closed

consomme: make per-connection TCP buffer sizes configurable#3595
benhillis wants to merge 2 commits into
microsoft:mainfrom
benhillis:user/benhillis/consomme-tcp-buffer-size

Conversation

@benhillis

Copy link
Copy Markdown
Member

The hardcoded 256 KiB per-connection rx/tx ring buffers in consomme cap TCP throughput at roughly 2 Gbps on paths whose bandwidth-delay product exceeds the buffer size.

Expose tcp_rx_buffer_size and tcp_tx_buffer_size on ConsommeParams so embedders can pick a value appropriate for their workload. The default remains 256 KiB, preserving existing behavior.

Testing

  • cargo build --release
  • cargo clippy --release -p consomme -p net_consomme --all-targets
  • cargo nextest run -p consomme (55/55 pass)
  • cargo doc --no-deps -p consomme -p net_consomme
  • cargo xtask fmt --fix

Empirically, raising the per-connection buffers to 1 MiB took iperf3 throughput across consomme from ~514 Mbps to roughly 3-4 Gbps in the host-to-guest direction. A follow-up will add autotuning so the default can grow without a fixed per-connection memory cost.

The hardcoded 256 KiB per-connection rx/tx ring buffers cap throughput at

~2 Gbps on paths whose bandwidth-delay product exceeds the buffer size.

Expose `tcp_rx_buffer_size` and `tcp_tx_buffer_size` on `ConsommeParams`

so embedders can pick a value appropriate for their workload. The default

remains 256 KiB, preserving existing behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 29, 2026 20:21
@benhillis benhillis requested a review from a team as a code owner May 29, 2026 20:21
@github-actions github-actions Bot added the unsafe Related to unsafe code label May 29, 2026
@github-actions

Copy link
Copy Markdown

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Exposes per-connection TCP rx/tx ring buffer sizes through ConsommeParams so embedders can raise the previously-hardcoded 256 KiB cap that was bottlenecking TCP throughput on high-BDP paths. Defaults are unchanged.

Changes:

  • Adds public tcp_rx_buffer_size / tcp_tx_buffer_size fields to ConsommeParams, defaulting to a new DEFAULT_TCP_BUFFER_SIZE of 256 KiB.
  • Threads these values through Consomme::new into tcp::Tcp::new, replacing the inline 256 * 1024 literals in ConnectionParams.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
vm/devices/net/net_consomme/consomme/src/lib.rs Adds default constant, public params fields, and propagates them to Tcp::new.
vm/devices/net/net_consomme/consomme/src/tcp.rs Tcp::new now takes rx/tx buffer sizes instead of using hardcoded 256 KiB.

Comment thread vm/devices/net/net_consomme/consomme/src/lib.rs
@damanm24

Copy link
Copy Markdown
Contributor

I noticed the same findings (that our TCP buffer sizes in Consommé limit our throughput) in #3413. I think this is the right change to make.

@github-actions

Copy link
Copy Markdown

…tion

The doc previously said these take effect for new connections, implying
params_mut() updates would apply. In fact Consomme::new snapshots the
sizes at construction and never resyncs, so runtime updates are ignored.
Clarify the docs to match the actual behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

@benhillis

Copy link
Copy Markdown
Member Author

Closing — superseded by #3597, which includes these configurable buffer commits and builds the autotune mechanism on top (now the default). No need to land this separately.

@benhillis benhillis closed this Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

unsafe Related to unsafe code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants