Skip to content

feat: Send and process ACK-ECN #1678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 43 commits into from
Apr 16, 2024
Merged

feat: Send and process ACK-ECN #1678

merged 43 commits into from
Apr 16, 2024

Conversation

larseggert
Copy link
Collaborator

@larseggert larseggert commented Feb 26, 2024 •
edited
Loading

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.

This was referenced Feb 26, 2024
Closed
Closed
@codecov Codecov
Copy link

codecov bot commented Feb 26, 2024 •
edited
Loading

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 larseggert enabled auto-merge February 26, 2024 09:29
@larseggert 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
Copy link
Collaborator Author

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

@larseggert larseggert marked this pull request as ready for review February 26, 2024 17:15
@larseggert larseggert enabled auto-merge February 26, 2024 17:15
mxinden
mxinden approved these changes Feb 27, 2024
View reviewed changes
Copy link
Collaborator

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mxinden mxinden mentioned this pull request Feb 27, 2024
Merged
@larseggert
Copy link
Collaborator Author

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

KershawChang
KershawChang reviewed Feb 28, 2024
View reviewed changes
Copy link
Collaborator

@KershawChang KershawChang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks good to me.

@larseggert 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
martinthomson reviewed Feb 29, 2024
View reviewed changes
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mxinden
Copy link
Collaborator

mxinden commented Feb 29, 2024

No CC response

Happening with #1689 @martinthomson.

@larseggert
Copy link
Collaborator Author

@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
martinthomson previously approved these changes Mar 1, 2024
View reviewed changes
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@larseggert larseggert marked this pull request as ready for review March 5, 2024 12:00
@larseggert larseggert enabled auto-merge March 5, 2024 14:42
mxinden
mxinden reviewed Mar 5, 2024
View reviewed changes
martinthomson
martinthomson reviewed Mar 6, 2024
View reviewed changes
@larseggert larseggert disabled auto-merge March 6, 2024 06:57
@larseggert
Copy link
Collaborator Author

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

@larseggert
Copy link
Collaborator Author

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

mxinden
mxinden reviewed Apr 11, 2024
View reviewed changes
@larseggert @mxinden
Update neqo-transport/src/ecn.rs
faa3c69 
Co-authored-by: Max Inden <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
mxinden
mxinden reviewed Apr 11, 2024
View reviewed changes
larseggert and others added 4 commits April 11, 2024 14:22
@larseggert @mxinden
Update neqo-transport/src/ecn.rs
b95d1a1 
Co-authored-by: Max Inden <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
@larseggert @mxinden
Update neqo-transport/src/ecn.rs
0729827 
Co-authored-by: Max Inden <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
@larseggert
Fixes
84b95b5
@larseggert
Merge branch 'main' into feat-more-ecn
1aa3180 
Signed-off-by: Lars Eggert <[email protected]>
martinthomson
martinthomson approved these changes Apr 15, 2024
View reviewed changes
larseggert and others added 11 commits April 16, 2024 10:07
@larseggert @martinthomson
Update neqo-transport/src/connection/tests/ecn.rs
a63f744 
Co-authored-by: Martin Thomson <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
@larseggert @martinthomson
Update neqo-transport/src/connection/tests/ecn.rs
65d8b61 
Co-authored-by: Martin Thomson <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
@larseggert @martinthomson
Update neqo-transport/src/connection/mod.rs
b2441b9 
Co-authored-by: Martin Thomson <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
@larseggert @martinthomson
Update neqo-transport/src/frame.rs
c19f74c 
Co-authored-by: Martin Thomson <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
@larseggert @martinthomson
Update neqo-transport/src/frame.rs
3178f23 
Co-authored-by: Martin Thomson <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
@larseggert @martinthomson
Update neqo-transport/src/ecn.rs
34f6bff 
Co-authored-by: Martin Thomson <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
@larseggert
Some fixups from code review
aad2e58
@larseggert
More code review fixups
3f811f7
@larseggert
Merge branch 'main' into feat-more-ecn
46d2f31
@larseggert
Check whether migration happened as expected.
c582ff0 
Minor other tweaks.
@larseggert
Consolidate all the nodata tests
8cefb15
@larseggert 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 larseggert deleted the feat-more-ecn branch April 16, 2024 10:37
@mxinden mxinden mentioned this pull request Aug 21, 2024
Closed
@mxinden mxinden mentioned this pull request Nov 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden approved these changes

@martinthomson martinthomson martinthomson approved these changes

@KershawChang KershawChang Awaiting requested review from KershawChang KershawChang is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ECN
5 participants
@larseggert @mxinden @tatsuhiro-t @martinthomson @KershawChang