Skip to content
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.

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

Basic ECN congestion control support #3216

Merged
merged 16 commits into from
Nov 8, 2022
Merged

Basic ECN congestion control support #3216

merged 16 commits into from
Nov 8, 2022

Conversation

csujedihy
Copy link
Contributor

Description

Treat ECN signal as loss for CUBIC.

Testing

Tested it over duonic with RED support.

Duonic setup:

.\duonic.ps1 -Rdq -RttMs 20 -BottleneckMbps 100 -BottleneckBufferPackets 1000 -REDUpper 200 -REDLower 100 -REDMaxProb 50 -REDQWeightPercent 80

Before this PR:

.\secnetperf.exe -test:tput -target:192.168.1.12 -upload:100000000 -stats:1 -ecn:1
Started!

[conn][0000022CFD0143D0] STATS: RTT:101823 us SendTotalPackets=71443 SendSuspectedLostPackets=1639 SendSpuriousLostPackets=0 SendCongestionCount=3 RecvTotalPackets=1768 RecvReorderedPackets=0 RecvDroppedPackets=0 RecvDuplicatePackets=0 RecvDecryptionFailures=0
Result: 100000000 bytes @ 91893 kbps (8705.721 ms).
App Main returning status 0

With this PR:

.\secnetperf.exe -test:tput -target:192.168.1.12 -upload:100000000 -stats:1 -ecn:1
Started!

[conn][000001B13EE743E0] STATS: EcnCapable=1 RTT=26299 us SendTotalPackets=69784 SendSuspectedLostPackets=0 SendSpuriousLostPackets=0 SendCongestionCount=4 SendEcnCongestionCount=4 RecvTotalPackets=1749 RecvReorderedPackets=0 RecvDroppedPackets=0 RecvDuplicatePackets=0 RecvDecryptionFailures=0
Result: 100000000 bytes @ 91750 kbps (8719.256 ms).
App Main returning status 0

RTT has been reduced to ~26ms from ~100ms.

@csujedihy csujedihy requested a review from a team as a code owner November 4, 2022 02:19
@csujedihy csujedihy added the Area: Core Related to the shared, core protocol logic label Nov 4, 2022
src/inc/msquic.h Outdated Show resolved Hide resolved
src/manifest/MsQuicEtw.man Outdated Show resolved Hide resolved
@csujedihy
Copy link
Contributor Author

csujedihy commented Nov 5, 2022

I don't understand this compile error (macosx). I have already declared QuicCongestionControlOnEcn in inline.c.

Undefined symbols for architecture x86_64:
  "_QuicCongestionControlOnEcn", referenced from:
      _QuicLossDetectionProcessAckBlocks in libmsquic.a(loss_detection.c.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [/Users/runner/work/1/s/artifacts/bin/ios/x64_Debug_openssl/msquictest.app/msquictest] Error 1

src/core/inline.c Outdated Show resolved Hide resolved
@nibanks
Copy link
Member

nibanks commented Nov 5, 2022

I don't understand this compile error (macosx). I have already declared QuicCongestionControlOnEcn in inline.c.

Undefined symbols for architecture x86_64:
  "_QuicCongestionControlOnEcn", referenced from:
      _QuicLossDetectionProcessAckBlocks in libmsquic.a(loss_detection.c.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [/Users/runner/work/1/s/artifacts/bin/ios/x64_Debug_openssl/msquictest.app/msquictest] Error 1

Should be fixed with my commit. You shouldn't have had the inline in inline.c. 😄

src/core/cubic.c Outdated Show resolved Hide resolved
@nibanks nibanks merged commit a686e85 into main Nov 8, 2022
@nibanks nibanks deleted the huanyi/ecn-as-loss-cc branch November 8, 2022 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Core Related to the shared, core protocol logic Area: Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants