io_handle: cache envoy address instances for recvmsg and recvmmsg #34649

danzh2010 · 2024-06-10T18:42:04Z

Commit Message: Maintain a LRU cache of Address::Instance objects in IoSocketHandleImpl to keep track of source and destination addresses received in recvmsg and recvmmsg. This cache is added mainly used for UDP read. This is an alternative to #33917 to address the issue that each recvmsg/recvmmsg will create a new instance of the source/destination addresses (which does stringification upon instantiation) even though they have been received before.

The cache can be initialized during IoSocketHandleImpl construction based on a new param max_addresses_cache_size_ in SocketCreationOptions. This change only enables EnvoyQuicClientConnection to use a SocketCreationOptions with size 4 to create the underlying IoSocketHandleImpl. There is no change to the server side recvmsg and recvmmsg calls as the significant CPU cost on server side hasn't been observed.

There is no cache support in non-IoSocketHandleImpl-based implementations.

Additional Description: Added QUICHE dependency to IoSocketHandleImpl to use data structures like QuicLRUCache and QuicSocketAddress. Refactored source/common/http:utility_lib to break the circular dependency as needed.

Risk Level: low, only affect quic upstream
Testing: added new unit and integration tests
Docs Changes: N/A
Release Notes: yes
Platform Specific Features: N/A
Runtime guard: envoy.reloadable_features.quic_upstream_socket_use_address_cache_for_read

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

repokitteh-read-only · 2024-06-10T18:42:11Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #34649 was opened by danzh2010.

see: more, trace.

alyssawilk · 2024-06-11T12:25:40Z

Throwing over to @RyanTheOptimist but please get !googler review once you're done

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

RyanTheOptimist

Thanks for doing this. For some reason I thought we could do this at the QUIC layer, but after looking at your PR, obviously we have to do it at the socket layer. I think this looks pretty good. Do we have any sort of benchmark we can run to ensure that this does what we want?

RyanTheOptimist · 2024-06-11T17:41:44Z

envoy/network/socket_interface.h

@@ -15,8 +17,14 @@ struct SocketCreationOptions {
  // and only valid on Linux.
  bool mptcp_enabled_{false};

+  // Specifies whether there should be a cache for all the address instances associated to this


Let's rephrase this slightly:

// Specifies the maximum size of the cache of the address instances associated with
// packets received by this socket. If this is 0, no addresses will be cached. Is only
// valid for datagram sockets.

RyanTheOptimist · 2024-06-11T17:51:15Z

source/common/quic/envoy_quic_utils.cc

+          false, /*max_addresses_cache_size_ = */ (
+              Runtime::runtimeFeatureEnabled(
+                  "envoy.reloadable_features.quic_upstream_socket_use_address_cache_for_read")
+                  ? 4u


Out of curiosity, why 4? Is this same method use for client and server? (The runtime feature says "upstream" so maybe this is client-only?)

This is only used to create client socket. I would think 4 is sufficient for a quic client connection, with a default path and an alternative path.

RyanTheOptimist · 2024-06-11T17:53:23Z

source/common/quic/envoy_quic_utils.cc

+              Runtime::runtimeFeatureEnabled(
+                  "envoy.reloadable_features.quic_upstream_socket_use_address_cache_for_read")
+                  ? 4u
+                  : 0u)});


nit: Up to you, but consider factoring this out into a small helper method:

int getSocketAddressCacheSize() { if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.quic_upstream_socket_use_address_cache_for_read") { return 4; } return 0; }

This would avoid the need for the /*max_addresses_cache_size_ = */ ( which is a bit hard to parse mentally. Instead we would simply say:
Network::SocketCreationOptions{false, getSocketAddressCacheSize()}

added a tmp variable for this.

RyanTheOptimist · 2024-06-11T17:57:11Z

source/common/network/io_socket_handle_impl.h

+  Address::InstanceConstSharedPtr
+  maybeGetDstAddressFromHeader(const cmsghdr& cmsg, uint32_t self_port, os_fd_t fd, bool v6only);
+
+  std::unique_ptr<AddressInstanceLRUCache> recent_received_addresses_;


Brief comment, in particular explaining when it can be null.

RyanTheOptimist · 2024-06-11T17:58:15Z

source/common/network/io_socket_handle_impl.h

+
+private:
+  Address::InstanceConstSharedPtr
+  maybeGetDstAddressFromHeader(const cmsghdr& cmsg, uint32_t self_port, os_fd_t fd, bool v6only);


Brief comment please. In particular, explain if the return value can be null and if so why, and what the "maybe" implies.

RyanTheOptimist · 2024-06-11T18:02:39Z

source/common/network/io_socket_handle_impl.cc

+          quic_address, std::make_unique<Address::InstanceConstSharedPtr>(new_address));
+      return new_address;
+    }
+    return *it->second;


nit: since this is the smaller codepath, let's inver the conditional and reduce the nesting a tad:

if (it != recent_received_addresses_->end()) { return *it->second; } Address::InstanceConstSharedPtr new_address = Address::addressFromSockAddrOrDie(ss, sizeof(sockaddr_in6), fd, v6only); recent_received_addresses_->Insert( quic_address, std::make_unique<Address::InstanceConstSharedPtr>(new_address)); return new_address;

RyanTheOptimist · 2024-06-11T18:03:25Z

source/common/network/io_socket_handle_impl.cc

+          quic_address, std::make_unique<Address::InstanceConstSharedPtr>(new_address));
+      return new_address;
+    }
+    return *it->second;


RyanTheOptimist · 2024-06-11T18:05:11Z

source/common/network/io_socket_handle_impl.cc

+    } else {
+      output.msg_[i].peer_address_ = Address::addressFromSockAddrOrDie(
+          raw_addresses[i], hdr.msg_namelen, fd_, socket_v6only_ || !udp_read_normalize_addresses_);
+    }


Hm. It seems like we have 4 places where we're doing basically the same thing of looking up an address in the cache and possible creating a new one. Can we share this code somehow, perhaps with a helper method?

Moved them into a member function.

RyanTheOptimist · 2024-06-11T18:05:46Z

source/common/network/io_socket_handle_impl.cc

@@ -477,7 +533,7 @@ IoHandlePtr IoSocketHandleImpl::accept(struct sockaddr* addr, socklen_t* addrlen
    return nullptr;
  }
  return SocketInterfaceImpl::makePlatformSpecificSocket(result.return_value_, socket_v6only_,
-                                                         domain_);
+                                                         domain_, {});


Why does this do something different from the similar call below? Perhaps we should have a comment?

accept() is only used by TCP socket which doesn't use the address cache. Whereas dup() can be used by both TCP and UDP, so copy over the same creation option.

RyanTheOptimist · 2024-06-11T18:11:13Z

source/common/quic/envoy_quic_client_connection.cc

@@ -172,10 +172,10 @@ void EnvoyQuicClientConnection::probeWithNewPort(const quic::QuicSocketAddress&
  Network::Address::InstanceConstSharedPtr new_local_address;
  if (current_local_address->ip()->version() == Network::Address::IpVersion::v4) {
    new_local_address = std::make_shared<Network::Address::Ipv4Instance>(
-        current_local_address->ip()->addressAsString());
+        current_local_address->ip()->addressAsString(), &current_local_address->socketInterface());


I'm not sure that I understand this change. Can you say more about it? I can't quite plumb it all through in my mind.

This is not quite relevant to the rest of the PR. The Ipv4Instance has a handle to a socket interface extension which will be used to create a specific IoHandle instance. If not passed in, the global default extension will be use.

Here we are creating an alternative socket for this connection. The alternative socket should use the same socket interface extension to create IoHandle. Since the address cache is specific to IoSocketHandleImpl, we want to make sure the same extension is used across the connection life time.

danzh2010 · 2024-06-14T18:51:17Z

Do we have any sort of benchmark we can run to ensure that this does what we want?

It's hard to benchmark recvmsg and recvmmsg calls. I added unit test and integration test to ensure we don't create different address objects in each read.

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

RyanTheOptimist · 2024-06-18T15:41:55Z

/wait
for passing CI

danzh2010 · 2024-06-18T16:19:36Z

/retest

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

danzh2010 · 2024-06-19T18:05:48Z

/retest

danzh2010 · 2024-06-20T15:20:35Z

The Envoy/Publish and verify CI failure seems unrelated.

abeyad · 2024-06-20T20:45:40Z

/assign @ggreenway for a cross-company reviewer

repokitteh-read-only · 2024-06-20T20:45:45Z

neither of for, a, cross-company, reviewer can be assigned to this issue.

🐱

Caused by: a #34649 (comment) was created by @abeyad.

see: more, trace.

ggreenway

/wait

bazel/external/quiche.BUILD

changelogs/current.yaml

source/common/network/io_socket_handle_impl.cc

source/common/quic/envoy_quic_utils.cc

Signed-off-by: Greg Greenway <ggreenway@apple.com>

RyanTheOptimist · 2024-06-25T13:21:54Z

/wait
on CI

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

…voyproxy#34649) Maintain a LRU cache of Address::Instance objects in IoSocketHandleImpl to keep track of source and destination addresses received in recvmsg and recvmmsg. This cache is added mainly used for UDP read. This is an alternative to envoyproxy#33917 to address the issue that each recvmsg/recvmmsg will create a new instance of the source/destination addresses (which does stringification upon instantiation) even though they have been received before. The cache can be initialized during IoSocketHandleImpl construction based on a new param max_addresses_cache_size_ in SocketCreationOptions. This change only enables EnvoyQuicClientConnection to use a SocketCreationOptions with size 4 to create the underlying IoSocketHandleImpl. There is no change to the server side recvmsg and recvmmsg calls as the significant CPU cost on server side hasn't been observed. There is no cache support in non-IoSocketHandleImpl-based implementations. Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>

danzh1989 added 3 commits June 10, 2024 15:09

cache envoy addresses

a9c2a5a

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

add new files

01338f6

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

flag protect

6d3747f

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

alyssawilk assigned RyanTheOptimist Jun 11, 2024

danzh1989 added 3 commits June 11, 2024 14:58

format

d1e6674

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

reorder flags

d5201e8

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

Merge branch 'main' into udpreadpackets

c661769

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

RyanTheOptimist reviewed Jun 11, 2024

View reviewed changes

address comments

10820b0

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

RyanTheOptimist previously approved these changes Jun 17, 2024

View reviewed changes

danzh1989 added 2 commits June 17, 2024 21:36

fix build

637a2cd

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

Merge branch 'main' into udpreadpackets

b9d264b

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

danzh2010 dismissed RyanTheOptimist’s stale review via b9d264b June 17, 2024 21:57

fix build again

fcc168c

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

danzh2010 requested review from ggreenway, KBaichoo, alyssawilk and mattklein123 as code owners June 18, 2024 01:20

more build fix

21d47cc

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

danzh2010 requested a review from lizan as a code owner June 18, 2024 14:31

repokitteh-read-only bot added the waiting label Jun 18, 2024

compile option build

b8a7f7a

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

repokitteh-read-only bot removed the waiting label Jun 18, 2024

ggreenway self-assigned this Jun 20, 2024

RyanTheOptimist previously approved these changes Jun 20, 2024

View reviewed changes

ggreenway requested changes Jun 24, 2024

View reviewed changes

bazel/external/quiche.BUILD Show resolved Hide resolved

changelogs/current.yaml Outdated Show resolved Hide resolved

source/common/network/io_socket_handle_impl.cc Show resolved Hide resolved

source/common/quic/envoy_quic_utils.cc Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jun 24, 2024

Update changelogs/current.yaml

55a61f2

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway dismissed RyanTheOptimist’s stale review via 55a61f2 June 24, 2024 22:58

repokitteh-read-only bot removed the waiting label Jun 24, 2024

ggreenway approved these changes Jun 24, 2024

View reviewed changes

ggreenway enabled auto-merge (squash) June 24, 2024 22:59

ggreenway disabled auto-merge June 24, 2024 22:59

ggreenway enabled auto-merge (squash) June 24, 2024 22:59

repokitteh-read-only bot added the waiting label Jun 25, 2024

danzh1989 added 2 commits June 25, 2024 14:11

Merge branch 'main' into udpreadpackets

53c94b5

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

Merge remote-tracking branch 'origin/udpreadpackets' into udpreadpackets

a239098

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

danzh2010 requested a review from wbpcode as a code owner June 25, 2024 14:25

repokitteh-read-only bot removed the waiting label Jun 25, 2024

ggreenway merged commit b32f876 into envoyproxy:main Jun 25, 2024
53 of 56 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

io_handle: cache envoy address instances for recvmsg and recvmmsg #34649

io_handle: cache envoy address instances for recvmsg and recvmmsg #34649

danzh2010 commented Jun 10, 2024

repokitteh-read-only bot commented Jun 10, 2024

alyssawilk commented Jun 11, 2024

RyanTheOptimist left a comment

RyanTheOptimist Jun 11, 2024

danzh2010 Jun 14, 2024

RyanTheOptimist Jun 11, 2024

danzh2010 Jun 14, 2024

RyanTheOptimist Jun 11, 2024

danzh2010 Jun 14, 2024

RyanTheOptimist Jun 11, 2024

danzh2010 Jun 14, 2024

RyanTheOptimist Jun 11, 2024

danzh2010 Jun 14, 2024

RyanTheOptimist Jun 11, 2024

danzh2010 Jun 14, 2024

RyanTheOptimist Jun 11, 2024

danzh2010 Jun 14, 2024

RyanTheOptimist Jun 11, 2024

danzh2010 Jun 14, 2024

RyanTheOptimist Jun 11, 2024

danzh2010 Jun 14, 2024

RyanTheOptimist Jun 11, 2024

danzh2010 Jun 14, 2024

danzh2010 commented Jun 14, 2024

RyanTheOptimist commented Jun 18, 2024

danzh2010 commented Jun 18, 2024

danzh2010 commented Jun 19, 2024

danzh2010 commented Jun 20, 2024

abeyad commented Jun 20, 2024

repokitteh-read-only bot commented Jun 20, 2024

ggreenway left a comment

RyanTheOptimist commented Jun 25, 2024

io_handle: cache envoy address instances for recvmsg and recvmmsg #34649

io_handle: cache envoy address instances for recvmsg and recvmmsg #34649

Conversation

danzh2010 commented Jun 10, 2024

repokitteh-read-only bot commented Jun 10, 2024

alyssawilk commented Jun 11, 2024

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danzh2010 commented Jun 14, 2024

RyanTheOptimist commented Jun 18, 2024

danzh2010 commented Jun 18, 2024

danzh2010 commented Jun 19, 2024

danzh2010 commented Jun 20, 2024

abeyad commented Jun 20, 2024

repokitteh-read-only bot commented Jun 20, 2024

ggreenway left a comment

Choose a reason for hiding this comment

RyanTheOptimist commented Jun 25, 2024