-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Conversation
Signed-off-by: Dan Zhang <danzh@google.com>
/assign @RyanTheOptimist |
There was a problem hiding this 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?
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? |
* 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>
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>
#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>
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