multitask quic socket operations by t-nelson · Pull Request #601 · anza-xyz/agave

t-nelson · 2024-04-05T09:06:10Z

Problem

quinn uses a single task to do socket operations. not great

Summary of Changes

use SO_REUSEPORT to fanout socket operation to multiple quinn::Endpoints

mergify · 2024-04-05T09:07:24Z

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

mergify · 2024-04-05T09:07:24Z

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

t-nelson

this is pretty much straight from @alessandrod's original work. i took the liberty of removing some debug statements, making the net-utils changes less invasive and a few other things noted below

t-nelson · 2024-04-05T09:07:33Z

    pub public_tpu_forwards_addr: Option<SocketAddr>,
 }

+const QUIC_ENDPOINTS: usize = 10;


didn't seem like this needed to be part of public api, so i moved it out of sdk

t-nelson · 2024-04-05T09:08:29Z

    parse_host_port(&string).map(|_| ())
 }

+#[derive(Clone, Debug)]


kinda regretting not deriving Copy after typing .clone() more times than anticipated...

t-nelson · 2024-04-05T09:08:54Z

+}
+
+impl Default for SocketConfig {
+    #[allow(clippy::derivable_impls)]


so we can be explicit

t-nelson · 2024-04-05T09:12:12Z

+    std::iter::once(Ok(socket))
+        .chain((1..num).map(|_| bind_to_with_config(ip, port, config.clone())))
+        .collect()


made this functional instead of imperative

t-nelson · 2024-04-05T09:12:56Z


 [dev-dependencies]
 assert_matches = { workspace = true }
+socket2 = { workspace = true }


moved from normal deps

t-nelson · 2024-04-05T09:16:01Z

    pub public_tpu_forwards_addr: Option<SocketAddr>,
 }

+const QUIC_ENDPOINTS: usize = 10;


we're going with 10?

I tried with 4 -- there was about 20% degradation in term of throughput. I think 10 is an okay choice for now. I have not seen much memory usage increase under the spamming tool.

lijunwangs · 2024-04-05T09:28:02Z

+            )
+            .unwrap();
+        let quic_config = SocketConfig {
+            reuseaddr: false,


Why do we need to redeclare when it was cloned at 2831?

lijunwangs · 2024-04-05T09:32:06Z

+            quic_config.clone(),
        );
+        let tpu_forwards_quic =
+            bind_more_with_config(tpu_forwards_quic, QUIC_ENDPOINTS, quic_config.clone()).unwrap();


no need for clone for the last one

lijunwangs · 2024-04-05T09:34:44Z

 }

+#[cfg(any(windows, target_os = "ios"))]
+fn udp_socket_with_config(config: SocketConfig) -> io::Result<Socket> {


config not used? -> _config

lijunwangs · 2024-04-05T10:13:59Z

        coalesce,
    ));
+
+    let mut accepts = incoming


Can you please put some comments here why we are doing this select! it is not immediately obvious.

ryleung-solana · 2024-04-05T12:19:04Z

        Arc<StreamStats>,
    ) {
-        let s = UdpSocket::bind("127.0.0.1:0").unwrap();
+        let sockets = (0..10)


Make this a constant

ryleung-solana · 2024-04-05T14:23:31Z

+        .collect::<FuturesUnordered<_>>();
    while !exit.load(Ordering::Relaxed) {
-        let timeout_connection = timeout(WAIT_FOR_CONNECTION_TIMEOUT, incoming.accept()).await;
+        let timeout_connection = select! {


I think what we're looking for is just to await the FuturesUnordered, or use JoinSet. This can also be made to work with a timeout, by simply adding a timeout future to the FuturesUnordered or JoinSet (and having the futures return an option/result).

ripatel-fd · 2024-04-05T17:33:25Z

SO_REUSEPORT does not load balance as you would expect on RHEL 8 Linux 4.18.0-513.9.1.el8_9.x86_64 when using localhost.
I would probably throw a Linux kernel version check in there or document that old Linux aren't supported.
But we shouldn't assume that users (especially large companies) are running a recent kernel version.

t-nelson · 2024-04-05T18:05:46Z

closing for #611

alessandrod · 2024-04-05T22:01:59Z

SO_REUSEPORT does not load balance as you would expect on RHEL 8 Linux 4.18.0-513.9.1.el8_9.x86_64 when using localhost. I would probably throw a Linux kernel version check in there or document that old Linux aren't supported. But we shouldn't assume that users (especially large companies) are running a recent kernel version.

@ripatel-fd do you have a [cit] for this? because we looked into git history and stickiness was there since day 1

ripatel-fd · 2024-04-05T23:52:21Z

SO_REUSEPORT does not load balance as you would expect on RHEL 8 Linux 4.18.0-513.9.1.el8_9.x86_64 when using localhost. I would probably throw a Linux kernel version check in there or document that old Linux aren't supported. But we shouldn't assume that users (especially large companies) are running a recent kernel version.

@ripatel-fd do you have a [cit] for this? because we looked into git history and stickiness was there since day 1

No, this is behavior observed when I tested on my box. I can write up a reprod C program and post a log of it running on my box. I'll send it to you in Discord once I have it.

(I do believe you with the stickiness. Glad to hear it's there... maybe I just messed up my test infra)

t-nelson and others added 2 commits April 5, 2024 03:05

net-utils: support SO_REUSEPORT

326ab03

tpu: use multiple quic endpoints

d25934a

t-nelson requested review from alessandrod, lijunwangs, ryleung-solana and sakridge April 5, 2024 09:06

t-nelson added v1.17 labels Apr 5, 2024

t-nelson commented Apr 5, 2024

View reviewed changes

lijunwangs reviewed Apr 5, 2024

View reviewed changes

ryleung-solana reviewed Apr 5, 2024

View reviewed changes

t-nelson closed this Apr 5, 2024

t-nelson mentioned this pull request Apr 6, 2024

Multi quic #634

Closed

Conversation

t-nelson commented Apr 5, 2024

Problem

Summary of Changes

Uh oh!

mergify Bot commented Apr 5, 2024

Uh oh!

mergify Bot commented Apr 5, 2024

Uh oh!

t-nelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ripatel-fd commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-nelson commented Apr 5, 2024

Uh oh!

alessandrod commented Apr 5, 2024

Uh oh!

ripatel-fd commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ripatel-fd commented Apr 5, 2024 •

edited

Loading

ripatel-fd commented Apr 5, 2024 •

edited

Loading