connection: skip read activate call when reading from transport socket if the connection is read disabled #14043

antoniovicente · 2020-11-16T18:05:10Z

Commit Message:
connection: skip read activate call when reading from transport socket if the connection is read disabled

Also, rename setReadBufferReady to setTransportSocketIsReadable to make its intended use more clear.

Additional Description:
Risk Level: low
Testing: unit
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Also, rename setReadBufferReady to setTransportSocketIsReadable to make its intended use more clear. Signed-off-by: Antonio Vicente <avd@google.com>

davinci26

looks good

alyssawilk

Thanks for tackling yet another subtle corner case here!

alyssawilk · 2020-11-17T14:41:24Z

source/common/network/connection_impl.cc

+  transport_wants_read_ = true;
+  // Only schedule a read activation if read is enabled. If read was disabled, read resumption will
+  // happen once read is re-enabled since transport_wants_read_ is now set to true.
+  if (read_disable_count_ == 0) {


So I see a test change to ensure that when read_disabled we don't activate here.
there's one other change in this PR, which is that in filterChainWantsData we no longer set transport_wants_read_ true. Is that behavior regression tested as well?

It stroke me odd as well so I dug a bit deeper into that and I think the current and the previous code are equivalent:

In the previous code we set transport_wants_read_ via calling setReadBufferReady in ReadDisable if:

(*) transport_wants_read_ is true already or buffer.length() > 0. This comes from the if-statement above. if (filterChainWantsData() && (read_buffer_.length() > 0 || transport_wants_read_))

Looking at the two cases individually:

In the case that transport_wants_read_ is true already then it doesn't really matter that we do not set transport_wants_read_ to true.

In the case that buffer.length() > 0 then the next call to ReadDisable(false) will trigger the read activation regardless of the value of transport_wants_read_ because of (*).

This means that even in the case where that we have a cycle of ReadDisable(true)/ReadEnable(false) (this used to leave connections at zombie state because the read activation was canceled) we will re-activate reads correctly.

+1 on testing though, the more the merrier.

I think you're referring to the case where we check filterChainWantsData() in readDisable(false) below. Setting transport_wants_read_ when resuming from readDisable(false) does not matter because it is either already set when the method is called or onReadReady clears it before it is possible to drain the read buffer and call readDisable(false) from within that method.

Also, setting transport_wants_read_ in readDisable(false) was known to be redundant as seen in this comment thread: #13772 (comment)

Longer version:
After a read from the transport socket the connection read buffer can be in one of the following states:

over high-watermark, which implies transport_wants_read_ == true and high watermark triggered

at exactly the high-watermark, which implies transport_wants_read_ == true but high watermark not triggered

not empty but below high-watermark, implies short read and transport_wants_read_ == false

empty, implies transport_wants_read_ == false

transport_wants_read_ == true in the cases above since shouldDrainReadBuffer() == true triggers a call to setTransportSocketIsReadable() from the transport.

Resumption on readDisable(false) can happen if:
a. read_buffer_.highWatermarkTriggered() is true and read_disable_count_ == 1
b. read_buffer_ is not empty and read_disable_count_ == 0
c. read_buffer_ is empty and transport_wants_read_ == true

Additional considerations:
Bytes are only ever added or removed from the read buffer under the call stack of onReadReady

Resumption case (a) can only happen after transport socket read case (1), so transport_wants_read_ == true in this case. The next call to onReadReady will return early because read_disable_count_ >= 1, so transport_wants_read_ remains == true.

The next call to onReadReady after resumption case (b) will perform a read from the transport because read_disable_count_ == 0. onReadReady will set transport_wants_read_ to false before doing reads from the transport or processing the contents of the read buffer. Nothing reads the value of transport_wants_read_ between the time the read resumption is scheduled until onReadReady sets it to false.

transport_wants_read_ is already set to true in case (c)

While explaining this I noticed an edge case that I hadn't considered before: If the read buffer is exactly at the buffer_limit_, shouldDrainReadBuffer() will return true but highWatermarkTriggered() returns false. In fact, this edge case is an optimization for the common case where the read limit is configured to a multiple of 16KB, so transport reads end up exactly hitting the configured buffer limit and narrowly avoid the expensive calls to readDisable(true)/readDisable(false). I'ld love to get rid of this edge case, but in order to do that we need to reduce the cost of going through a readDisable(true)/readDisable(false) cycle.

alyssawilk · 2020-11-17T14:42:45Z

source/common/network/connection_impl.cc

@@ -369,7 +378,7 @@ void ConnectionImpl::readDisable(bool disable) {
      // buffer's high watermark has triggered.
      ASSERT(read_buffer_.length() > 0 || read_disable_count_ == 0);
      dispatch_buffered_data_ = true;
-      setReadBufferReady();
+      ioHandle().activateFileEvents(Event::FileReadyType::Read);


Think it's worth a comment on why this case is different?

Rearranged and expanded comments.

Signed-off-by: Antonio Vicente <avd@google.com>

alyssawilk

Awesome. Thanks for the detailed comments - I think they help explain a hairy and confusing area of code.

One typo nit and clang tidy, otherwise LGTM.

alyssawilk · 2020-11-18T22:04:40Z

source/common/network/connection_impl.cc

+  transport_wants_read_ = true;
+  // Only schedule a read activation if the connection is not read disabled to avoid spurious
+  // wakeups. When read disabled, the connection will not read from the transport, and limit
+  // dispatch to the current contents of the read buffer if it's high-watermark is triggered and


nit: it's -> its?

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente · 2020-11-21T02:21:13Z

/retest

repokitteh-read-only · 2020-11-21T02:21:17Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14043 (comment) was created by @antoniovicente.

see: more, trace.

alyssawilk · 2020-11-23T13:27:23Z

CI still cranky :-/

…ip_read_activate Signed-off-by: Antonio Vicente <avd@google.com>

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente · 2020-11-25T01:50:23Z

/retest

repokitteh-read-only · 2020-11-25T01:50:27Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #14043 (comment) was created by @antoniovicente.

see: more, trace.

antoniovicente · 2020-11-30T19:24:33Z

/retest

repokitteh-read-only · 2020-11-30T19:24:36Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #14043 (comment) was created by @antoniovicente.

see: more, trace.

antoniovicente · 2020-12-01T22:20:35Z

/retest

for coverage

repokitteh-read-only · 2020-12-01T22:20:38Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #14043 (comment) was created by @antoniovicente.

see: more, trace.

…ip_read_activate Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente · 2020-12-02T02:00:59Z

/retest

repokitteh-read-only · 2020-12-02T02:01:04Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14043 (comment) was created by @antoniovicente.

see: more, trace.

connection: skip read activate if the connection is read disabled.

3d9e14d

Also, rename setReadBufferReady to setTransportSocketIsReadable to make its intended use more clear. Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente requested review from asraa, ggreenway, htuch, lizan and PiotrSikora as code owners November 16, 2020 18:05

antoniovicente requested review from alyssawilk and davinci26 November 16, 2020 18:05

antoniovicente assigned alyssawilk and davinci26 Nov 16, 2020

davinci26 previously approved these changes Nov 17, 2020

View reviewed changes

alyssawilk reviewed Nov 17, 2020

View reviewed changes

expand comments

83ba7b1

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente dismissed davinci26’s stale review via 83ba7b1 November 18, 2020 07:11

alyssawilk reviewed Nov 19, 2020

View reviewed changes

antoniovicente mentioned this pull request Nov 20, 2020

buffer: improve read reservations to efficiently handle multiple slices #14054

Merged

antoniovicente added 2 commits November 19, 2020 19:10

fix typo

0584585

Signed-off-by: Antonio Vicente <avd@google.com>

fix clang-tidy

8fbec6c

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente added 2 commits November 23, 2020 14:13

Merge remote-tracking branch 'upstream/master' into high_watermark_sk…

fe52f31

…ip_read_activate Signed-off-by: Antonio Vicente <avd@google.com>

fix build

b1b3df0

Signed-off-by: Antonio Vicente <avd@google.com>

alyssawilk approved these changes Nov 30, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into high_watermark_sk…

69615f2

…ip_read_activate Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente merged commit 745ca24 into envoyproxy:master Dec 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connection: skip read activate call when reading from transport socket if the connection is read disabled #14043

connection: skip read activate call when reading from transport socket if the connection is read disabled #14043

antoniovicente commented Nov 16, 2020

davinci26 left a comment

alyssawilk left a comment

alyssawilk Nov 17, 2020

davinci26 Nov 17, 2020

antoniovicente Nov 18, 2020

alyssawilk Nov 17, 2020

antoniovicente Nov 18, 2020

alyssawilk left a comment

alyssawilk Nov 18, 2020

antoniovicente commented Nov 21, 2020

repokitteh-read-only bot commented Nov 21, 2020

alyssawilk commented Nov 23, 2020

antoniovicente commented Nov 25, 2020

repokitteh-read-only bot commented Nov 25, 2020

antoniovicente commented Nov 30, 2020

repokitteh-read-only bot commented Nov 30, 2020

antoniovicente commented Dec 1, 2020

repokitteh-read-only bot commented Dec 1, 2020

antoniovicente commented Dec 2, 2020

repokitteh-read-only bot commented Dec 2, 2020

connection: skip read activate call when reading from transport socket if the connection is read disabled #14043

connection: skip read activate call when reading from transport socket if the connection is read disabled #14043

Conversation

antoniovicente commented Nov 16, 2020

davinci26 left a comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk Nov 17, 2020

Choose a reason for hiding this comment

davinci26 Nov 17, 2020

Choose a reason for hiding this comment

antoniovicente Nov 18, 2020

Choose a reason for hiding this comment

alyssawilk Nov 17, 2020

Choose a reason for hiding this comment

antoniovicente Nov 18, 2020

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk Nov 18, 2020

Choose a reason for hiding this comment

antoniovicente commented Nov 21, 2020

repokitteh-read-only bot commented Nov 21, 2020

alyssawilk commented Nov 23, 2020

antoniovicente commented Nov 25, 2020

repokitteh-read-only bot commented Nov 25, 2020

antoniovicente commented Nov 30, 2020

repokitteh-read-only bot commented Nov 30, 2020

antoniovicente commented Dec 1, 2020

repokitteh-read-only bot commented Dec 1, 2020

antoniovicente commented Dec 2, 2020

repokitteh-read-only bot commented Dec 2, 2020