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

gnrc_sixlowpan_frag_sfr: add support for queue-based ECN #16175

Merged
merged 2 commits into from
Oct 15, 2022

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Mar 9, 2021

Contribution description

This provides two queue metrics set the ECN bit in a 6LoWPAN SFR RFRAG header:

  • based on a netif's input queue, and
  • based on SFR's internal frame queue (that holds frames waiting for the inter-frame gap)
    Both metrics can be combined, so that if either metric hits the threshold (default 1/2 of the queue size for both), the ECN bit is set.

Testing procedure

No tests provided, as this is not as easy to be reproducible locally. If I find a way, I will provide a test. For now, there is only the compile test like this:

USEMODULE="gnrc_sixlowpan_frag_sfr_ecn_if_in gnrc_sixlowpan_frag_sfr_ecn_if_out gnrc_sixlowpan_frag_sfr_ecn_fqueue" make -C tests/gnrc_sixlowpan_frag_sfr

Issues/PRs references

Requires #16174 (merged)

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 9, 2021
@miri64 miri64 force-pushed the gnrc_sixlowpan_frag_sfr/enh/mark-ecn branch from 5de1c19 to b944fb3 Compare March 12, 2021 17:16
@miri64 miri64 force-pushed the gnrc_sixlowpan_frag_sfr/enh/mark-ecn branch 2 times, most recently from 9bb20c2 to f1722b4 Compare March 15, 2021 15:27
@miri64
Copy link
Member Author

miri64 commented Mar 15, 2021

Added #16193 as additional dependency.

@miri64 miri64 force-pushed the gnrc_sixlowpan_frag_sfr/enh/mark-ecn branch 2 times, most recently from 6de7e47 to 78480e0 Compare March 15, 2021 15:43
@miri64 miri64 force-pushed the gnrc_sixlowpan_frag_sfr/enh/mark-ecn branch from 73e1712 to dd30b37 Compare May 6, 2021 15:44
@miri64
Copy link
Member Author

miri64 commented May 6, 2021

Rebased and squashed to current master.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022
@miri64 miri64 removed the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022
@benpicco benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 11, 2022
@benpicco
Copy link
Contributor

What does it mean if the ECN bit is set?

This needs a rebase btw 😉

@miri64
Copy link
Member Author

miri64 commented Oct 11, 2022

What does it mean if the ECN bit is set?

It explicitly notifies the sender that a congestion happened (ECN = Explicit Congestion Notification) and thus amends loss-based congestion detection with hints, based on actual congestion. Here's some more info: https://en.wikipedia.org/wiki/Explicit_Congestion_Notification

@benpicco
Copy link
Contributor

Would be great to also have this expanded in the doc somewhere 😇

@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 miri64 enabled auto-merge October 13, 2022 07:40
@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
@dylad dylad 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
@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
@maribu
Copy link
Member

maribu commented Oct 14, 2022

Rebased on #18741 this would have passed Murdock

@miri64
Copy link
Member Author

miri64 commented Oct 14, 2022

Let's merge it after feature freeze then. :-)

@miri64 miri64 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 14, 2022
@miri64 miri64 merged commit 53ed211 into RIOT-OS:master Oct 15, 2022
@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
@miri64 miri64 deleted the gnrc_sixlowpan_frag_sfr/enh/mark-ecn branch January 20, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants