quic: make server connection defer sending packets during processing incoming packets #22796

danzh2010 · 2022-08-22T15:28:32Z

Commit Message: enable defer sending in QuicConnection::ProcessUdpPacket() on server side. This bundles packet writes for each connection at the end of the event loop instead of each packet read. It is for improving CPU cost of sendmsg() for QUIC listener, especially with UDP GSO batch writing.

Also change how closing connections are deferred during L4 filters creation. Without this change, the connection close is deferred till handling all types of delayed close in QuicSession::OnCanWrite() which ProcessUdpPacket() would call into upon handling the 1st incoming packet. With this change, connection close caused by L4 filter creation is handled right before calling ProcessUdpPacket() because OnCanWrite() is deferred.

Risk Level: medium, behavior change of quic write pipeline
Testing: existing tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Runtime guard: envoy_reloadable_features_quic_defer_send_in_response_to_packet

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

RyanTheOptimist · 2022-08-24T17:20:39Z

Looks like //test/integration:quic_protocol_integration_test is failing. Is that expected?

RyanTheOptimist

I might be missing it, but do we have test which verifies the intended behavior for this change? Specifically that this "bundles packet writes for each connection at the end of the event loop instead of each packet read. " I was expecting to see a test were multiple packets were read and the flag controlled when packets were sent?

source/common/quic/envoy_quic_server_connection.cc

source/common/quic/envoy_quic_server_session.cc

RyanTheOptimist · 2022-08-24T17:29:28Z

source/common/quic/quic_filter_manager_connection_impl.cc

-    if (close_type_during_initialize_.has_value()) {
-      close(close_type_during_initialize_.value());
-      close_type_during_initialize_ = absl::nullopt;
-    }


Do you think there's any need for a feature flag to control this part of the change? Presumably you're confident that it's a safe change? Or should we guard it with the same flag?

It's guarded by close_type_during_initialize_. If the current feature flag is on, close_type_during_initialize_ wont' have value here, as OnCanWrite() happens outside of ProcessUdpPacket().

I don't think I understand. maybeApplyDelayClosePolicy() is called from a couple of different places. Before this PR, it's possible for maybeApplyDelayClosePolicy() to actually call close() if not in delayed close (on line 150). But with this PR, if we're not in delayed close then maybeApplyDelayClosePolicy() will not call close().

But I guess you're saying that before this PR, close_type_during_initialize_ will never have a value here. Do I have that right?

maybeApplyDelayClosePolicy() is only called in OnCanWrite() which was called at the end of ProcessUdpPacket() and various other places before this PR. But in this PR, we check close_type_during_initialize_ at the beginning of ProcessUdpPacket() and close connection there. So OnCanWrite() won't be called at all in ProcessUdpPacket(). This check only takes effect at processing the 1st packet, close_type_during_initialize_ will always be nullptr afterwards.

Ok, I see. Thanks for the explanation.

danzh2010 · 2022-08-24T18:03:25Z

I might be missing it, but do we have test which verifies the intended behavior for this change? Specifically that this "bundles packet writes for each connection at the end of the event loop instead of each packet read. " I was expecting to see a test were multiple packets were read and the flag controlled when packets were sent?

I modified test/common/quic/active_quic_listener_test.cc to check that send alarm is armed after receiving CHLO.

danzh2010 · 2022-08-24T18:12:02Z

Looks like //test/integration:quic_protocol_integration_test is failing. Is that expected?

Nope, I'm looking into it. They are all upstream disconnect tests, but it's weird that they fail only in windows build.

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

RyanTheOptimist · 2022-08-24T20:44:46Z

I might be missing it, but do we have test which verifies the intended behavior for this change? Specifically that this "bundles packet writes for each connection at the end of the event loop instead of each packet read. " I was expecting to see a test were multiple packets were read and the flag controlled when packets were sent?

I modified test/common/quic/active_quic_listener_test.cc to check that send alarm is armed after receiving CHLO.

nod And presumably the send alarm being set leads to the behavior mentioned in the PR description. But it's not obvious to me that this is the case. (Though I certainly believe you that it is!) Would it be hard to have a test which actually waited to send something until multiple packets are received? For example, the existing test in active_quic_listener_test.cc doesn't seem to send any packets after receiving the CHLO. Is there a way to make it do that?

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

RyanTheOptimist · 2022-08-25T17:20:30Z

source/common/quic/quic_filter_manager_connection_impl.cc

-    if (close_type_during_initialize_.has_value()) {
-      close(close_type_during_initialize_.value());
-      close_type_during_initialize_ = absl::nullopt;
-    }


Ok, I see. Thanks for the explanation.

RyanTheOptimist · 2022-08-25T17:20:51Z

source/common/quic/quic_filter_manager_connection_impl.cc

@@ -97,6 +99,7 @@ void QuicFilterManagerConnectionImpl::close(Network::ConnectionCloseType type) {
    } else {
      delayed_close_state_ = DelayedCloseState::CloseAfterFlush;
    }
+    std::cerr << "========= danzh: delay close\n";


Presumably you're still debugging the CI failures?

Yes, this is the only way I could think of to debug windows.

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

RyanTheOptimist · 2022-08-30T17:36:59Z

/wait

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

danzh2010 · 2022-08-30T22:26:20Z

nod And presumably the send alarm being set leads to the behavior mentioned in the PR description. But it's not obvious to me that this is the case. (Though I certainly believe you that it is!) Would it be hard to have a test which actually waited to send something until multiple packets are received? For example, the existing test in active_quic_listener_test.cc doesn't seem to send any packets after receiving the CHLO. Is there a way to make it do that?

It was harder than I expected. CHLO doesn't always elicit sending packet due to packet coalescing, so there might be no behavioral change in this test with and without defer sending. But we are not testing QUICHE code, so to make the test more obvious, I used the getter of defer_send_in_response_to_packets_ to check its value in the test.

RyanTheOptimist

LGTM modulo a TODO

RyanTheOptimist · 2022-09-01T17:23:33Z

source/common/quic/envoy_quic_server_connection.cc

+  if (defer_send_) {
+    // Defer sending while processing UDP packets till the end of the current event loop to optimize
+    // UDP GSO sendmsg efficiency. But this optimization causes some test failures under Windows,
+    // and Windows doesn't support GSO, do not apply this optimization on Windows.


nit: Mind filing a github issue to track this and adding a TODO to get to the bottom of this?

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

danzh1989 added 21 commits August 16, 2021 13:07

switch to github

bc25efa

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

rename namespace

47683cb

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

Merge remote-tracking branch 'origin/master'

aec3b1c

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

Merge remote-tracking branch 'upstream/main'

9d72076

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

Merge remote-tracking branch 'upstream/main'

a25e5ac

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

Merge remote-tracking branch 'upstream/main'

cdb1893

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

Merge remote-tracking branch 'upstream/main'

b8b746c

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

Merge remote-tracking branch 'upstream/main'

443788e

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

Merge remote-tracking branch 'upstream/main'

07c3ca7

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

Merge remote-tracking branch 'upstream/main'

ae57162

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

Merge remote-tracking branch 'upstream/main'

dc58b5e

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

Merge remote-tracking branch 'upstream/main'

0906585

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

Merge remote-tracking branch 'upstream/main'

d3102d6

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

Merge remote-tracking branch 'upstream/main'

e157fd1

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

Merge remote-tracking branch 'upstream/main'

c24b729

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

Merge remote-tracking branch 'upstream/main'

683ae53

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

defer sending

71b645b

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

comments

0a145e8

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

Merge remote-tracking branch 'upstream/main'

67cf4fa

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

Merge branch 'master' into defersending

8fd9fb6

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

add runtime guard

729cc0a

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

danzh2010 changed the title ~~Defersending~~ quic: make server connection defer sending packets during processing incoming packets Aug 22, 2022

danzh1989 added 5 commits August 22, 2022 18:26

Merge remote-tracking branch 'upstream/main'

85a5298

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

add release note

ed45549

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

Merge branch 'master' into defersending

b86f48c

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

Merge remote-tracking branch 'upstream/main'

c3ff0d2

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

fix filter tests

ea24357

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

danzh2010 requested review from RyanTheOptimist and alyssawilk as code owners August 24, 2022 14:34

Merge remote-tracking branch 'upstream/main'

16b461a

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

RyanTheOptimist self-assigned this Aug 24, 2022

RyanTheOptimist reviewed Aug 24, 2022

View reviewed changes

add debug log

a17ad13

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

add more logging

1c71d30

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

RyanTheOptimist reviewed Aug 25, 2022

View reviewed changes

danzh1989 added 5 commits August 26, 2022 21:37

enable quiche verbosity log

e4c7004

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

Merge branch 'master' into defersending

3d2755b

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

exclude windows

d5b4897

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

fix build

9df80fe

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

Merge remote-tracking branch 'upstream/main'

6a6bb9b

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

repokitteh-read-only bot added the waiting label Aug 30, 2022

danzh1989 added 3 commits August 30, 2022 20:25

Merge remote-tracking branch 'upstream/main'

f35c293

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

Merge branch 'master' into defersending

8390937

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

tests

fa513d6

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

repokitteh-read-only bot removed the waiting label Aug 30, 2022

danzh1989 added 2 commits August 30, 2022 22:23

Merge remote-tracking branch 'upstream/main'

35cf0e1

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

Merge branch 'master' into defersending

04ff1b0

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

RyanTheOptimist previously approved these changes Sep 1, 2022

View reviewed changes

danzh2010 mentioned this pull request Sep 2, 2022

Defer sending in QuicConnection can't be enabled on Windows #22976

Open

TODO

8b39fea

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

danzh2010 dismissed RyanTheOptimist’s stale review via 8b39fea September 2, 2022 15:19

RyanTheOptimist approved these changes Sep 2, 2022

View reviewed changes

RyanTheOptimist merged commit 255af42 into envoyproxy:main Sep 2, 2022

alyssawilk mentioned this pull request Apr 24, 2023

envoy_reloadable_features_quic_defer_send_in_response_to_packet deprecation #26906

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: make server connection defer sending packets during processing incoming packets #22796

quic: make server connection defer sending packets during processing incoming packets #22796

danzh2010 commented Aug 22, 2022

RyanTheOptimist commented Aug 24, 2022

RyanTheOptimist left a comment

RyanTheOptimist Aug 24, 2022

danzh2010 Aug 24, 2022

RyanTheOptimist Aug 24, 2022

danzh2010 Aug 25, 2022

RyanTheOptimist Aug 25, 2022

danzh2010 commented Aug 24, 2022

danzh2010 commented Aug 24, 2022

RyanTheOptimist commented Aug 24, 2022

RyanTheOptimist Aug 25, 2022

RyanTheOptimist Aug 25, 2022

danzh2010 Aug 25, 2022

RyanTheOptimist commented Aug 30, 2022

danzh2010 commented Aug 30, 2022

RyanTheOptimist left a comment

RyanTheOptimist Sep 1, 2022

danzh2010 Sep 2, 2022

quic: make server connection defer sending packets during processing incoming packets #22796

quic: make server connection defer sending packets during processing incoming packets #22796

Conversation

danzh2010 commented Aug 22, 2022

RyanTheOptimist commented Aug 24, 2022

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danzh2010 commented Aug 24, 2022

danzh2010 commented Aug 24, 2022

RyanTheOptimist commented Aug 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist commented Aug 30, 2022

danzh2010 commented Aug 30, 2022

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment