Being refactored: ECN additions #1495

larseggert · 2023-11-21T15:58:17Z

This PR is getting too large. I'm in the process of refactoring it into smaller chunks:

martinthomson

I take it that this is groundwork for ECN

Cargo.toml

neqo-helper/src/lib.rs

neqo-common/src/datagram.rs

neqo-helper/src/lib.rs

codecov-commenter · 2023-12-12T13:52:22Z

Codecov Report

Attention: 98 lines in your changes are missing coverage. Please review.

Comparison is base (9e6cd93) 86.52% compared to head (edbbfb7) 86.49%.

Files	Patch %	Lines
neqo-common/src/socket.rs	85.35%	29 Missing ⚠️
neqo-interop/src/main.rs	0.00%	25 Missing ⚠️
neqo-server/src/main.rs	0.00%	17 Missing ⚠️
neqo-client/src/main.rs	0.00%	12 Missing ⚠️
neqo-common/src/datagram.rs	72.41%	8 Missing ⚠️
neqo-transport/src/qlog.rs	0.00%	6 Missing ⚠️
neqo-transport/src/connection/mod.rs	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1495      +/-   ##
==========================================
- Coverage   86.52%   86.49%   -0.03%     
==========================================
  Files         117      118       +1     
  Lines       38229    38531     +302     
==========================================
+ Hits        33079    33329     +250     
- Misses       5150     5202      +52

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

martinthomson

I've a question for you and some suggestions, but I trust you to resolve those from here (and I don't want to block you while I'm on leave).

neqo-common/src/socket.rs

neqo-common/src/datagram.rs

neqo-common/src/socket.rs

martinthomson · 2023-12-19T03:36:42Z

neqo-server/src/main.rs

@@ -710,7 +711,9 @@ impl ServersRunner {
        match self.server.process(dgram, self.args.now()) {
            Output::Datagram(dgram) => {
                let socket = self.find_socket(dgram.source());
-                emit_packet(socket, dgram);
+                if let Err(e) = emit_datagram(socket, &dgram) {
+                    eprintln!("UDP write error: {}", e);


Do you want to open an issue to track EAGAIN handling in these?

neqo-common/src/socket.rs

larseggert · 2024-01-04T10:57:20Z

@martinthomson could you re-review? Made a bunch of changes that make things work on all platforms again.

larseggert · 2024-01-04T11:00:08Z

Blocked until nix-rust/nix#2203 has shipped.

martinthomson · 2024-01-11T23:52:21Z

neqo-common/Cargo.toml

+nix = { git = "https://github.com/larseggert/nix.git", branch = "feat-tos", features = [
+        "socket",
+        "net",
+        "uio",
+], optional = true }


OK, let's continue to block on this being integrated upstream. Worst case, we can take up your fork, but if this can be upstreamed, that's a lot easier.

It's in progress. Depends on another PR upstream.

neqo-common/src/datagram.rs

martinthomson · 2024-01-11T23:59:53Z

neqo-common/src/socket.rs

+        *tos = 0xff;
+        *ttl = 0xff;


0 or 0xff? Consistency?

martinthomson · 2024-01-12T00:02:47Z

neqo-common/src/socket.rs

+    }
+
+    #[test]
+    fn test_bind_v4() {


I have a real aversion to tests called test_whatever. Keep in mind that this will be called ...::test::test_bind_v4

martinthomson · 2024-01-17T23:13:20Z

neqo-transport/src/recovery.rs

@@ -412,7 +412,7 @@ impl LossRecoverySpace {
            .sent_packets
            .iter_mut()
            // BTreeMap iterates in order of ascending PN
-            .take_while(|(&k, _)| Some(k) < largest_acked)
+            .take_while(|(&k, _)| largest_acked.is_none() || Some(k) < largest_acked)


This sort of change really needs to be in a separate patch so that we can look at the recovery tests in more detail.

Yeah, I realized this is getting big. I will break it into individual chunks.

This part of the refactor of mozilla#1495. This doesn't include any ECN I/O or other logic, just adds the required fields to the datagram header.

@martinthomson

* Add TOS and TTL information to Datagrams This part of the refactor of #1495. This doesn't include any ECN I/O or other logic, just adds the required fields to the datagram header. * cargo fmt * Add test * Fix clippy * Add an IpTos type as suggested by @martinthomson. I might back this back out if it is not the direction we want to go in, pending further discussion. * Move IpTos things into their own source file * Make IpTos use a single byte

Most of the remaining bits from mozilla#1495 Depends on mozilla#1604

mxinden · 2024-02-21T14:25:34Z

Crossposting mxinden#1 (review) here to ensure review on fork does not get lost.

Most of the remaining bits from mozilla#1495 Depends on mozilla#1604

larseggert · 2024-03-01T07:30:53Z

Nothing left in this PR that would be of interest.

The remaining bits from mozilla#1495 The remaining todo item after this PR is to actually act on incoming CE marks, i.e., trigger a congestion control action. See mozilla#1689

* feat: Send and process ACK-ECN The remaining bits from #1495 The remaining todo item after this PR is to actually act on incoming CE marks, i.e., trigger a congestion control action. See #1689 * Modifier * Fix botched merge * Fix merge * Rework * Add more tests that hopefully cover all cases. * WIP * Tests passing * More tests * Minimize diff * ci(interop): run ecn test * Update neqo-transport/src/connection/tests/ecn.rs Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org> * Update neqo-transport/src/qlog.rs Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org> * Fix qns * Reduce number of CONNECTION_CLOSE frames If this makes ngtcp2 happy, refactor into separate PR. * Update neqo-transport/src/ecn.rs Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org> * Update neqo-transport/src/ecn.rs Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org> * Update neqo-transport/src/ecn.rs Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org> * Fixes * Update neqo-transport/src/connection/tests/ecn.rs Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org> * Update neqo-transport/src/connection/tests/ecn.rs Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org> * Update neqo-transport/src/connection/mod.rs Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org> * Update neqo-transport/src/frame.rs Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org> * Update neqo-transport/src/frame.rs Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org> * Update neqo-transport/src/ecn.rs Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org> * Some fixups from code review * More code review fixups * Check whether migration happened as expected. Minor other tweaks. * Consolidate all the `nodata` tests --------- Signed-off-by: Lars Eggert <lars@eggert.org> Co-authored-by: Max Inden <mail@max-inden.de> Co-authored-by: Martin Thomson <mt@lowentropy.net>

* feat: Send and process ACK-ECN The remaining bits from #1495 The remaining todo item after this PR is to actually act on incoming CE marks, i.e., trigger a congestion control action. See #1689 * Modifier * Fix botched merge * Fix merge * Rework * Add more tests that hopefully cover all cases. * WIP * Tests passing * More tests * Minimize diff * ci(interop): run ecn test * Update neqo-transport/src/connection/tests/ecn.rs Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org> * Update neqo-transport/src/qlog.rs Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org> * Fix qns * Reduce number of CONNECTION_CLOSE frames If this makes ngtcp2 happy, refactor into separate PR. * feat(transport): pass ECN CE marks to CC Pass ECN CE marks received through FRAME_TYPE_ACK_ECN frames to the congestion controler. * Update ACK delay on ECN CE mark --------- Signed-off-by: Lars Eggert <lars@eggert.org> Co-authored-by: Lars Eggert <lars@eggert.org>

larseggert force-pushed the feat-ecn branch from 397b3bb to 35e917a Compare November 21, 2023 16:26

martinthomson reviewed Nov 21, 2023

View reviewed changes

larseggert marked this pull request as ready for review December 12, 2023 13:44

larseggert requested a review from KershawChang as a code owner December 12, 2023 13:44

larseggert marked this pull request as draft December 12, 2023 13:44

larseggert closed this Dec 12, 2023

larseggert deleted the feat-ecn branch December 12, 2023 13:46

larseggert restored the feat-ecn branch December 12, 2023 13:47

larseggert reopened this Dec 12, 2023

larseggert force-pushed the feat-ecn branch 2 times, most recently from c0c6850 to fb5434d Compare December 18, 2023 13:32

martinthomson previously approved these changes Dec 19, 2023

View reviewed changes

larseggert force-pushed the feat-ecn branch from 09ed6fd to 9d47ccc Compare January 2, 2024 11:17

martinthomson reviewed Jan 2, 2024

View reviewed changes

neqo-common/src/socket.rs Outdated Show resolved Hide resolved

larseggert force-pushed the feat-ecn branch from 052d15a to 8a14bc3 Compare January 4, 2024 10:03

larseggert requested a review from martinthomson January 4, 2024 10:56

larseggert marked this pull request as ready for review January 4, 2024 10:57

larseggert added the blocked Blocked on something else label Jan 4, 2024

larseggert force-pushed the feat-ecn branch 3 times, most recently from 4ce889b to f766192 Compare January 11, 2024 16:29

larseggert force-pushed the feat-ecn branch 2 times, most recently from 50058c0 to cf3e00b Compare January 16, 2024 11:50

larseggert added 4 commits January 17, 2024 09:20

Rollup

f2c093c

Move functionality into neqo-common as non-default feature

8234696

Make clippy happy

7983f80

Use trait for socket as suggested by @martinthomson

57512f6

larseggert added 9 commits January 17, 2024 13:04

Minimize diff to origin/main

7219e7a

More minimization

afefa8f

Use new Datagram::new method everywhere. Make TOS and TTL path-specific.

93942b2

Small tweaks

62f963f

Reformat

0c7554d

Disable ECN on a path when ECN-marked packets are persistently lost

0422462

Remove println!

dbdaa80

Rebase cleanup

e06206b

Merge branch 'feat-ecn' of github.com:larseggert/neqo into feat-ecn

c0bf1a1

martinthomson reviewed Jan 17, 2024

View reviewed changes

larseggert marked this pull request as draft January 18, 2024 09:54

Address code review comments

052eb06

larseggert added a commit to larseggert/neqo that referenced this pull request Jan 18, 2024

Add TOS and TTL information to Datagrams

29e650b

This part of the refactor of mozilla#1495. This doesn't include any ECN I/O or other logic, just adds the required fields to the datagram header.

larseggert mentioned this pull request Jan 18, 2024

Add TOS and TTL information to Datagrams #1568

Merged

larseggert changed the title ~~Add TOS and TTL info to datagram objects~~ Being refactored: ECN additions Jan 18, 2024

larseggert added a commit to larseggert/neqo that referenced this pull request Jan 22, 2024

Add TOS and TTL information to Datagrams

c30154c

This part of the refactor of mozilla#1495. This doesn't include any ECN I/O or other logic, just adds the required fields to the datagram header.

larseggert dismissed martinthomson’s stale review via 052eb06 January 23, 2024 15:21

larseggert mentioned this pull request Jan 31, 2024

ECN #441

Closed

larseggert added a commit to larseggert/neqo that referenced this pull request Feb 14, 2024

feat: Send and process ACK-ECN

1ea7018

Most of the remaining bits from mozilla#1495 Depends on mozilla#1604

larseggert mentioned this pull request Feb 14, 2024

feat: Send and process ACK-ECN mxinden/neqo#1

Closed

mxinden pushed a commit to mxinden/neqo that referenced this pull request Feb 23, 2024

feat: Send and process ACK-ECN

8d8c989

Most of the remaining bits from mozilla#1495 Depends on mozilla#1604

larseggert mentioned this pull request Feb 26, 2024

feat: Send and process ACK-ECN #1678

Merged

larseggert closed this Mar 1, 2024

larseggert deleted the feat-ecn branch March 1, 2024 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Being refactored: ECN additions #1495

Being refactored: ECN additions #1495

larseggert commented Nov 21, 2023 •

edited

Loading

martinthomson left a comment

codecov-commenter commented Dec 12, 2023 •

edited

Loading

martinthomson left a comment

martinthomson Dec 19, 2023

larseggert commented Jan 4, 2024

larseggert commented Jan 4, 2024

martinthomson Jan 11, 2024

larseggert Jan 18, 2024

martinthomson Jan 11, 2024

martinthomson Jan 12, 2024

larseggert Jan 18, 2024

martinthomson Jan 17, 2024

larseggert Jan 18, 2024

mxinden commented Feb 21, 2024

larseggert commented Mar 1, 2024

Being refactored: ECN additions #1495

Being refactored: ECN additions #1495

Conversation

larseggert commented Nov 21, 2023 • edited Loading

martinthomson left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 12, 2023 • edited Loading

Codecov Report

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larseggert commented Jan 4, 2024

larseggert commented Jan 4, 2024

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

mxinden commented Feb 21, 2024

larseggert commented Mar 1, 2024

larseggert commented Nov 21, 2023 •

edited

Loading

codecov-commenter commented Dec 12, 2023 •

edited

Loading