quic: Use GRO for reading packets from the UDP socket in upstream QUIC#32628
quic: Use GRO for reading packets from the UDP socket in upstream QUIC#32628abeyad merged 30 commits intoenvoyproxy:mainfrom
Conversation
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>
|
@alyssawilk still haven't found a good way to write tests for this PR, relied on running existing tests so far |
|
I can also default it to _FALSE and only enable in Envoy Mobile for starters, though I don't think based on usage patterns that this is a high risk change |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
two thoughts for testing.
You can see if any tests which include a real EnvoyQuicClientConnection can have the TestThreadsafeSingletonInjector added. Even if the test fast-fails you can do an
EXPECT_CALL(os_sys_calls_, recvmsg like test/common/quic/quic_io_handle_wrapper_test.cc does, since I believe with the flag false we'd recvmmsg. Alternately there's an expect log message macro somewhere and you could see if any unit tests have an increased level of "starting gro recvmsg with..." logs
|
@alyssawilk @danzh2010 This is ready for review again, thanks! |
I added a test in envoy_quic_client_session_test that mocks the recvmsg, and makes sure recvmsg is called and recvmmsg is not called |
Signed-off-by: Ali Beyad <abeyad@google.com>
Signed-off-by: Ali Beyad <abeyad@google.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM modulo addressing Dan's flag concerns
/wait
Signed-off-by: Ali Beyad <abeyad@google.com>
Done, ready for another round of review |
Signed-off-by: Ali Beyad <abeyad@google.com>
danzh2010
left a comment
danzh2010
left a comment
There was a problem hiding this comment.
Thanks for making the change!
|
/retest |
Signed-off-by: Ali Beyad <abeyad@google.com>
changelogs/current.yaml
Outdated
| ``envoy.reloadable_features.prefer_quic_client_udp_gro`` to ``false``. | ||
| - area: http3 | ||
| change: | | ||
| Allow recvmmsg (multi-message) to be used for reading packets from a client QUIC UDP socket, if |
There was a problem hiding this comment.
wait I thought we wanted to disallow client recvmmsg?
There was a problem hiding this comment.
i changed the runtime guard to disallow_ so the default behavior is to disallow recvmmsg. PTAL
Signed-off-by: Ali Beyad <abeyad@google.com>
Signed-off-by: Ali Beyad <abeyad@google.com>
|
/retest |
2 similar comments
|
/retest |
|
/retest |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
oops, that's what I get for not reading far enough along.
Love this final version -all the readability and barely any refactor
|
/retest |
|
thanks for the reviews everyone! |
4 participants
This behavior is guarded by the runtime flag
envoy.restart_features.prefer_udp_gro.We previously turned off GRO in
EnvoyQuicClientConnectionbecause 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
recvmmsgshould be used or not, so we candisable 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