Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

quic: turn off GRO in client connection #19088

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

danzh2010
Copy link
Contributor

Signed-off-by: Dan Zhang danzh@google.com

Commit Message: hard code prefer_gro to false. The performance of GRO hasn't been evaluated yet, so it shouldn't be default on.

Risk Level: low
Testing: existing tests pass

Signed-off-by: Dan Zhang <danzh@google.com>
@RyanTheOptimist
Copy link
Contributor

/assign @RyanTheOptimist

Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

This change looks good. Should we consider making this configurable?

@danzh2010
Copy link
Contributor Author

This change looks good. Should we consider making this configurable?

Eventually yes, we probably want to add a client side config knob somewhere. But right now the GRO implementation has some issues already and its performance hasn't been measured. So I think not exposing it is the best way.

@RyanTheOptimist
Copy link
Contributor

This change looks good. Should we consider making this configurable?

Eventually yes, we probably want to add a client side config knob somewhere. But right now the GRO implementation has some issues already and its performance hasn't been measured. So I think not exposing it is the best way.

Sounds good. Would you mind filing an issue about this so we have it recorded?

@RyanTheOptimist RyanTheOptimist merged commit a7a00d9 into envoyproxy:main Nov 24, 2021
mpuncel added a commit to mpuncel/envoy that referenced this pull request Nov 30, 2021
* main: (77 commits)
  Fix verify_and_print_latest_release logic (envoyproxy#19111)
  http2: drain only once when reached max_requests_per_connection (envoyproxy#19078)
  Overload: Reset H2 server stream only use codec level reset mechanism (envoyproxy#18895)
  Update QUICHE from c2ddf95dc to 7f2d442e3 (envoyproxy#19095)
  tools: Fix dependency checker release dates bug (envoyproxy#19109)
  cve_scan: Use `envoy.dependency.cve_scan` (envoyproxy#19047)
  tcp: fix overenthusiastic bounds on the new pool (envoyproxy#19036)
  dep: update Proxy-Wasm C++ host (2021-11-18). (envoyproxy#19074)
  build(deps): bump frozendict from 2.0.7 to 2.1.0 in /tools/base (envoyproxy#19080)
  kafka: dependency upgrades (envoyproxy#18995)
  build(deps): bump charset-normalizer in /tools/dependency (envoyproxy#19105)
  build(deps): bump slack-sdk in /.github/actions/pr_notifier (envoyproxy#19093)
  dep: Remove dependency - six (envoyproxy#19085)
  Remove requested_server_name_ field from StreamInfo (envoyproxy#19102)
  broken link path fix for items http_filters/grpc_json_transcoder_filter (envoyproxy#19101)
  quic: turn off GRO (envoyproxy#19088)
  Listener: Add global conn limit opt out. (envoyproxy#18876)
  Specify type for matching Subject Alternative Name. (envoyproxy#18628)
  Fix a broken example in Lua filter docs (envoyproxy#19086)
  Fix a small typo (envoyproxy#19058)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
abeyad added a commit to abeyad/envoy that referenced this pull request Feb 28, 2024
This behavior is guarded by the runtime flag
`envoy.reloadable_features.prefer_udp_gro`.

We previously turned off GRO in `EnvoyQuicClientConnection` because we
weren't sure about the performance (see
envoyproxy#19088). However, kernel experts
have recommended using GRO over recvmmsg as a much more CPU efficient
solution for reading multiple UDP packets into a single buffer.

Signed-off-by: Ali Beyad <abeyad@google.com>
abeyad added a commit that referenced this pull request Mar 12, 2024
#32628)

This behavior is guarded by the runtime flag `envoy.restart_features.prefer_udp_gro`.

We previously turned off GRO in `EnvoyQuicClientConnection` because we weren't sure about the performance (see
#19088). However, kernel experts have recommended using GRO over recvmmsg as a much more CPU efficient solution for reading multiple UDP packets into a single buffer.

This change also enables setting the option of whether `recvmmsg` should be used or not, so we can
disable it for client connections, while keeping it for listener sockets.

Risk Level: low (can be turned off with the runtime guard)
Testing: unit tests
Release Notes: included
Runtime guard: envoy.reloadable_features.prefer_udp_gro

Signed-off-by: Ali Beyad <abeyad@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants