Skip to content

congure_quic: initial import of QUIC congestion control#15952

Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom
miri64:congure/feat/congure_quic
Oct 17, 2022
Merged

congure_quic: initial import of QUIC congestion control#15952
miri64 merged 2 commits intoRIOT-OS:masterfrom
miri64:congure/feat/congure_quic

Conversation

@miri64
Copy link
Member

@miri64 miri64 commented Feb 8, 2021

Contribution description

This provides an implementation for CongURE of the congestion control mechanism of the QUIC protocol as specified in draft-ietf-quic-recovery (version 34 at the opening of this PR). It has the advantage of having congestion control via loss and ECN already build in and also provides means for packet pacing, so perfect for SFR!

Testing procedure

Tests are provided for which riotctrl (at least v0.2.2) is required for them to run. If installed,

make -j -C tests/congure_quic flash test

should succeed.

Issues/PRs references

Requires #15951 (merged), #16111, and #16119. (merged) When merged, I will adapt the test framework to use the turo framework introduced in #15950, which was the original idea to begin with (which is why it is already printing JSON strings).
PR dependency graph

@miri64 miri64 added Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first Area: sys Area: System labels Feb 8, 2021
@miri64
Copy link
Member Author

miri64 commented Feb 9, 2021

Maybe @larseggert also wants to take a look.

@miri64 miri64 force-pushed the congure/feat/congure_quic branch 2 times, most recently from 7a11318 to 3b5f6ca Compare February 11, 2021 20:08
@miri64 miri64 force-pushed the congure/feat/congure_quic branch from 3b5f6ca to 485b887 Compare February 26, 2021 12:10
@miri64
Copy link
Member Author

miri64 commented Feb 26, 2021

Rebased to current master, needs some adaptions to the changes done in #15951, so I am marking this WIP temporarily.

@miri64 miri64 added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Feb 26, 2021
@miri64
Copy link
Member Author

miri64 commented Feb 26, 2021

Adapted tests for behavior changes of congure_test in #15951.

@miri64
Copy link
Member Author

miri64 commented Feb 26, 2021

And added the new Kconfig stuff (which does not work properly yet, since dependency resolution of ztimer is a bit messy... @leandrolanzieri said he will have a look).

@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 1, 2021
@miri64
Copy link
Member Author

miri64 commented Mar 1, 2021

The test framework needed some rework (see #16111) to handle longer parameters, so I will base this PR now on that.

@miri64 miri64 force-pushed the congure/feat/congure_quic branch from 95e5479 to c6e9a42 Compare March 2, 2021 09:54
@miri64
Copy link
Member Author

miri64 commented Mar 2, 2021

Rebased to current master and dependencies and added #16119 as new dependency.

benpicco
benpicco previously approved these changes Apr 9, 2022
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

If memory serves me right the only thing missing here were the failing tests - but that seems to be resolved. So ACK.

@benpicco benpicco dismissed their stale review April 9, 2022 10:48

test is still failing

@benpicco
Copy link
Contributor

benpicco commented Apr 9, 2022

Arg the new test is still failing

@benpicco
Copy link
Contributor

We should be able to merge this if we can get it past CI

@miri64
Copy link
Member Author

miri64 commented Oct 13, 2022

Arg the new test is still failing

Mh, but the failures look like actual test failures, not just failure of the test infrastructure. I will investigate after rebase.

@miri64
Copy link
Member Author

miri64 commented Oct 13, 2022

But first: Rebase

@miri64 miri64 force-pushed the congure/feat/congure_quic branch from 7a9831f to 1c36643 Compare October 13, 2022 08:08
@riot-ci
Copy link

riot-ci commented Oct 13, 2022

Murdock results

✔️ PASSED

1bf241e tests: Initial import of congure_quic tests

Success Failures Total Runtime
1989 0 1989 06m:47s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@miri64 miri64 force-pushed the congure/feat/congure_quic branch from 1c36643 to 594a998 Compare October 13, 2022 08:55
@miri64
Copy link
Member Author

miri64 commented Oct 13, 2022

Fixed.

@miri64
Copy link
Member Author

miri64 commented Oct 13, 2022

(there was a bugfix introduced in 1a1792a, however, the old code was not removed; with my fix I took the same bug fix I used for congure_reno)

@miri64 miri64 force-pushed the congure/feat/congure_quic branch from 594a998 to 13ab6f8 Compare October 13, 2022 09:06
@miri64
Copy link
Member Author

miri64 commented Oct 13, 2022

@leandrolanzieri @MrKevinWeiss there seems to be a Kconfig issue which I don't really understand in Murdock (ci-prod)... locally I can not reproduce this, neither with

TEST_KCONFIG=1 make -C tests/congure_test/ -j

nor

TEST_KCONFIG=0 make -C tests/congure_test/ -j

@miri64 miri64 force-pushed the congure/feat/congure_quic branch from 13ab6f8 to 769424a Compare October 13, 2022 09:25
@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 13, 2022
@miri64
Copy link
Member Author

miri64 commented Oct 14, 2022

I'm fine with this one going in only after feature freeze and clearing up the Murdock queue for now.

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looking good, Makefile.ci needs two more entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.