feat: Send and process ACK-ECN #1678

larseggert · 2024-02-26T07:59:24Z

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.

codecov · 2024-02-26T08:08:00Z

Codecov Report

Attention: Patch coverage is 97.91667% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 93.16%. Comparing base (f88b862) to head (8cefb15).
Report is 2 commits behind head on main.

Files	Patch %	Lines
neqo-transport/src/ecn.rs	93.68%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1678      +/-   ##
==========================================
+ Coverage   93.13%   93.16%   +0.03%     
==========================================
  Files         117      119       +2     
  Lines       36342    36635     +293     
==========================================
+ Hits        33846    34130     +284     
- Misses       2496     2505       +9

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

larseggert · 2024-02-26T14:39:01Z

Making this a draft PR again until I can add some tests.

mxinden

For what my review is worth, this looks good to me.

larseggert · 2024-02-28T09:21:44Z

@KershawChang @martinthomson when you have a chance, please review?

KershawChang

Overall, this looks good to me.

neqo-transport/src/path.rs

martinthomson

Getting there for sure. No CC response, but sending and receiving is useful.

neqo-common/src/datagram.rs

neqo-transport/src/connection/mod.rs

neqo-transport/src/connection/tests/ecn.rs

neqo-transport/src/path.rs

neqo-transport/src/tracking.rs

mxinden · 2024-02-29T08:35:13Z

No CC response

Happening with #1689 @martinthomson.

larseggert · 2024-02-29T16:44:40Z

@martinthomson thanks for the detailed review. I think I fixed most things, see the review responses on a few things where I was unclear.

We probably need to implement something like https://datatracker.ietf.org/doc/html/rfc9000#section-a.4; the current disablement based on a fixed number of lost packets on a path is obviously bad, because eventually we will exceed that number.

martinthomson

LGTM

neqo-common/src/tos.rs

neqo-transport/src/tracking.rs

neqo-transport/src/ecn.rs

neqo-transport/src/tracking.rs

If this makes ngtcp2 happy, refactor into separate PR.

Signed-off-by: Lars Eggert <lars@eggert.org>

larseggert · 2024-04-09T12:26:57Z

@mxinden 1999874 made ngtcp2 happy - thanks for the tip!

larseggert · 2024-04-09T13:47:32Z

@KershawChang @martinthomson this is - yet again - ready for review.

neqo-transport/src/ecn.rs

Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>

neqo-transport/src/ecn.rs

Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>

Signed-off-by: Lars Eggert <lars@eggert.org>

neqo-transport/src/connection/tests/ecn.rs

neqo-transport/src/connection/mod.rs

neqo-transport/src/frame.rs

neqo-transport/src/ecn.rs

Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org>

Minor other tweaks.

larseggert requested review from KershawChang and martinthomson as code owners February 26, 2024 07:59

This was referenced Feb 26, 2024

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

Closed

Being refactored: ECN additions #1495

Closed

larseggert enabled auto-merge February 26, 2024 09:29

larseggert marked this pull request as draft February 26, 2024 14:38

auto-merge was automatically disabled February 26, 2024 14:38
Pull request was converted to draft

larseggert marked this pull request as ready for review February 26, 2024 17:15

larseggert enabled auto-merge February 26, 2024 17:15

mxinden approved these changes Feb 27, 2024

View reviewed changes

mxinden mentioned this pull request Feb 27, 2024

feat(transport): pass ECN CE marks to CC #1689

Merged

KershawChang reviewed Feb 28, 2024

View reviewed changes

neqo-transport/src/path.rs Outdated Show resolved Hide resolved

neqo-transport/src/path.rs Outdated Show resolved Hide resolved

larseggert marked this pull request as draft February 28, 2024 15:28

auto-merge was automatically disabled February 28, 2024 15:28
Pull request was converted to draft

martinthomson reviewed Feb 29, 2024

View reviewed changes

martinthomson previously approved these changes Mar 1, 2024

View reviewed changes

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

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

neqo-transport/src/tracking.rs Outdated Show resolved Hide resolved

neqo-transport/src/tracking.rs Outdated Show resolved Hide resolved

larseggert marked this pull request as ready for review March 5, 2024 12:00

larseggert requested review from martinthomson, mxinden and KershawChang March 5, 2024 14:41

larseggert enabled auto-merge March 5, 2024 14:42

mxinden reviewed Mar 5, 2024

View reviewed changes

neqo-transport/src/ecn.rs Outdated Show resolved Hide resolved

mxinden reviewed Mar 5, 2024

View reviewed changes

neqo-transport/src/ecn.rs Show resolved Hide resolved

martinthomson reviewed Mar 6, 2024

View reviewed changes

larseggert disabled auto-merge March 6, 2024 06:57

larseggert added 4 commits April 9, 2024 09:24

Merge branch 'main' into feat-more-ecn

79306bc

Reduce number of CONNECTION_CLOSE frames

1999874

If this makes ngtcp2 happy, refactor into separate PR.

Merge branch 'main' into feat-more-ecn

0c8c709

Merge branch 'main' into feat-more-ecn

95af14b

Signed-off-by: Lars Eggert <lars@eggert.org>

mxinden reviewed Apr 11, 2024

View reviewed changes

neqo-transport/src/ecn.rs Outdated Show resolved Hide resolved

Update neqo-transport/src/ecn.rs

faa3c69

Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>

mxinden reviewed Apr 11, 2024

View reviewed changes

neqo-transport/src/ecn.rs Outdated Show resolved Hide resolved

neqo-transport/src/ecn.rs Outdated Show resolved Hide resolved

larseggert and others added 4 commits April 11, 2024 14:22

Update neqo-transport/src/ecn.rs

b95d1a1

Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>

Update neqo-transport/src/ecn.rs

0729827

Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>

Fixes

84b95b5

Merge branch 'main' into feat-more-ecn

1aa3180

Signed-off-by: Lars Eggert <lars@eggert.org>

martinthomson approved these changes Apr 15, 2024

View reviewed changes

larseggert and others added 11 commits April 16, 2024 10:07

Update neqo-transport/src/connection/tests/ecn.rs

a63f744

Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org>

Update neqo-transport/src/connection/tests/ecn.rs

65d8b61

Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org>

Update neqo-transport/src/connection/mod.rs

b2441b9

Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org>

Update neqo-transport/src/frame.rs

c19f74c

Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org>

Update neqo-transport/src/frame.rs

3178f23

Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org>

Update neqo-transport/src/ecn.rs

34f6bff

Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org>

Some fixups from code review

aad2e58

More code review fixups

3f811f7

Merge branch 'main' into feat-more-ecn

46d2f31

Check whether migration happened as expected.

c582ff0

Minor other tweaks.

Consolidate all the nodata tests

8cefb15

larseggert added this pull request to the merge queue Apr 16, 2024

Merged via the queue into mozilla:main with commit 670237d Apr 16, 2024
18 checks passed

larseggert deleted the feat-more-ecn branch April 16, 2024 10:37

mxinden mentioned this pull request Aug 21, 2024

expose ECN counts in connection Stats #2068

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Send and process ACK-ECN #1678

feat: Send and process ACK-ECN #1678

larseggert commented Feb 26, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading

larseggert commented Feb 26, 2024

mxinden left a comment

larseggert commented Feb 28, 2024

KershawChang left a comment

martinthomson left a comment

mxinden commented Feb 29, 2024

larseggert commented Feb 29, 2024

martinthomson left a comment

larseggert commented Apr 9, 2024

larseggert commented Apr 9, 2024

feat: Send and process ACK-ECN #1678

feat: Send and process ACK-ECN #1678

Conversation

larseggert commented Feb 26, 2024 • edited Loading

codecov bot commented Feb 26, 2024 • edited Loading

Codecov Report

larseggert commented Feb 26, 2024

mxinden left a comment

Choose a reason for hiding this comment

larseggert commented Feb 28, 2024

KershawChang left a comment

Choose a reason for hiding this comment

martinthomson left a comment

Choose a reason for hiding this comment

mxinden commented Feb 29, 2024

larseggert commented Feb 29, 2024

martinthomson left a comment

Choose a reason for hiding this comment

larseggert commented Apr 9, 2024

larseggert commented Apr 9, 2024

larseggert commented Feb 26, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading