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.

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

sys/net/gnrc/neterr: use gnrc_tx_sync #15695

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

sys/net/gnrc/neterr: use gnrc_tx_sync #15695

wants to merge 5 commits into from
+402 −121

Conversation

maribu
Copy link
Member

@maribu maribu commented Dec 23, 2020

Contribution description

This PR rewires gnrc_neterr to build upon #15694. Citing from #15694 for motivating this change:

  1. Simpler error reporting without footguns
    • The current approach of using core/msg for passing up error messages is difficult to use if other message come in. Currently, gnrc_sock is busy-waiting and fetching messages from the message queue until the number of expected status reports is received. It will enqueue all non-status-report messages again at the end of the queue. This has multiple issues:
      • Busy waiting is especially in lower power scenarios with time slotted MAC protocols harmful, as the CPU will remain active and consume power even though the it could sleep until the TX slot is reached
      • The status reports from the network stack are send to the user thread blocking. If the message queue of the user thread is full, the network stack would block until the user stack can fetch the messages. If another higher priority thread would start sending a message, it would busy wait for its status reports to completely come in. Hence, the first thread doesn't get CPU time to fetch messages and unblock the network stack. As a result, the system would lock up completely.
    • Just adding the error/status code to the gnrc_tx_sync_t would preallocate and reserve memory for the error reporting. That way gnrc_sock does not need to search through the message queue for status reports and the network stack does not need to block for the user thread fetching it.

Testing procedure

  1. gnrc_sock should still work with gnrc_neterr used
  2. tests/netdev_test should still work
  3. examples/gnrc_lorawan should still work

Issues/PRs references

Depends on and includes #15694

@maribu maribu added Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. State: waiting for other PR State: The PR requires another PR to be merged first Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Dec 23, 2020
@maribu maribu requested review from miri64 and jia200x December 23, 2020 16:45
@jia200x jia200x added the Process: blocked by feature freeze Integration Process: The impact of this PR is too high for merging it during soft feature freeze. label Jan 14, 2021
@miri64 miri64 removed the Process: blocked by feature freeze Integration Process: The impact of this PR is too high for merging it during soft feature freeze. label Jan 19, 2021
maribu added 5 commits January 27, 2021 09:35
@maribu
sys/net/gnrc/tx_sync: new module
c00705d 
The new `gnrc_tx_sync` module allows users of the GNRC network stack to
synchronize with the actual transmission of outgoing packets. This is directly
integrated into gnrc_sock. Hence, if `gnrc_tx_sync` is used, calls to e.g.
sock_udp_send() will block until the network stack has processed the message.

Use cases:
1. Prevent packet drop when sending at high rate
    - If the application is sending faster than the stack can handle, the
      message queues will overflow and outgoing packets are lost
2. Passing auxiliary data about the transmission back the stack
    - When e.g. the number of required retransmissions, the transmission time
      stamp, etc. should be made available to a user of an UDP sock, a
      synchronization mechanism is needed
3. Simpler error reporting without footguns
    - The current approach of using `core/msg` for passing up error messages is
      difficult to use if other message come in. Currently, gnrc_sock is
      busy-waiting and fetching messages from the message queue until the number
      of expected status reports is received. It will enqueue all
      non-status-report messages again at the end of the queue. This has
      multiple issues:
        - Busy waiting is especially in lower power scenarios with time slotted
          MAC protocols harmful, as the CPU will remain active and consume
          power even though the it could sleep until the TX slot is reached
        - The status reports from the network stack are send to the user thread
          blocking. If the message queue of the user thread is full, the network
          stack would block until the user stack can fetch the messages. If
          another higher priority thread would start sending a message, it
          would busy wait for its status reports to completely come in. Hence,
          the first thread doesn't get CPU time to fetch messages and unblock
          the network stack. As a result, the system would lock up completely.
    - Just adding the error/status code to the gnrc_tx_sync_t would preallocate
      and reserve memory for the error reporting. That way gnrc_sock does not
      need to search through the message queue for status reports and the
      network stack does not need to block for the user thread fetching it.
@maribu
tests/gnrc_tx_sync: add test for gnrc_tx_sync
9761ba6
@maribu
sys/net/gnrc/neterr: use gnrc_tx_sync
db3462c 
Re-wired gnrc_neterr to use gnrc_tx_sync to transfer error/status codes back up
the stack.
@maribu
examples/gnrc_lorawan: update to new gnrc_neterr API
61096b6
@maribu
tests/netdev_test: update to net gnrc_neterr API
c20a054
@miri64 miri64 mentioned this pull request Feb 5, 2021
Merged
2 tasks
@miri64
Copy link
Member

miri64 commented Feb 24, 2021

Needs rebase, now that #15694 is merged.

@maribu
Copy link
Member Author

maribu commented Feb 24, 2021

Needs rebase, now that #15694 is merged.

Yep. But it also depends on a gnrc_tx_sync implementation for gnrc_sixlowpand_frag_sfr.

I'll give this a look right now and if this isn't too much effort, I can PR this today. Otherwise this will have to wait a bit for, as I have some other projects to currently attend to.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 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
@stale stale bot closed this Apr 16, 2022
@maribu maribu reopened this Apr 16, 2022
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Apr 16, 2022
@github-actions github-actions bot added Area: examples Area: Example Applications Area: sys Area: System Area: tests Area: tests and testing framework labels Apr 16, 2022
@maribu
Copy link
Member Author

maribu commented Apr 16, 2022

I will revive this, hopefully soonish

@stale
Copy link

stale bot commented Nov 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 Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@jia200x jia200x Awaiting requested review from jia200x

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

@cgundogan cgundogan Awaiting requested review from cgundogan

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

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

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

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

@smlng smlng Awaiting requested review from smlng

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Process: needs >1 ACK Integration Process: This PR requires more than one ACK State: stale State: The issue / PR has no activity for >185 days State: waiting for other PR State: The PR requires another PR to be merged first State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maribu @miri64 @jia200x @MrKevinWeiss