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

drivers/{dose, slipdev, sam0_eth}: generate RX event for queued packets #18201

Merged
merged 3 commits into from
Aug 26, 2022
Merged

drivers/{dose, slipdev, sam0_eth}: generate RX event for queued packets #18201

merged 3 commits into from
Aug 26, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jun 13, 2022 •
edited
Loading

Contribution description

I was debugging the issue that packets where stuck in the RX path until the next packet was received, turns out they were stuck in the driver's RX queue:

2022-06-08 19:15:52,658 - INFO # > ping -i 2000 fd11:a000::e858:4ed2:564e:215b
2022-06-08 19:15:52,658 - INFO # 
2022-06-08 19:15:54,667 - INFO # 12 bytes from fd11:a000::e858:4ed2:564e:215b: icmp_seq=0 ttl=64 time=2001.958 ms
2022-06-08 19:15:56,667 - INFO # 12 bytes from fd11:a000::e858:4ed2:564e:215b: icmp_seq=1 ttl=64 time=2001.959 ms
2022-06-08 19:15:59,658 - INFO # 
2022-06-08 19:15:59,662 - INFO # --- fd11:a000::e858:4ed2:564e:215b PING statistics ---
2022-06-08 19:15:59,667 - INFO # 3 packets transmitted, 2 packets received, 33% packet loss
2022-06-08 19:15:59,672 - INFO # round-trip min/avg/max = 2001.958/2001.958/2001.959 ms

To fix this, generate another NETDEV_EVENT_RX_COMPLETE complete event if there is still a packet in the rx buffer when we finished processing the current one.

Testing procedure

IMG_0176

The issue can be triggered by heavy network traffic, e.g.

sudo ping -f fd11:9c00::e858:4ed2:564e:215a
sudo ping -A fd11:9800::e858:4ed2:564e:215b

With this patch, we don't have packets stuck in the RX buffer after that anymore

2022-06-13 15:42:47,841 - INFO # > ping fd11::1
2022-06-13 15:42:47,841 - INFO # 
2022-06-13 15:42:47,855 - INFO # 12 bytes from fd11::1: icmp_seq=0 ttl=61 time=8.472 ms
2022-06-13 15:42:48,855 - INFO # 12 bytes from fd11::1: icmp_seq=1 ttl=61 time=8.367 ms
2022-06-13 15:42:49,856 - INFO # 12 bytes from fd11::1: icmp_seq=2 ttl=61 time=8.557 ms
2022-06-13 15:42:49,856 - INFO # 
2022-06-13 15:42:49,859 - INFO # --- fd11::1 PING statistics ---
2022-06-13 15:42:49,864 - INFO # 3 packets transmitted, 3 packets received, 0% packet loss
2022-06-13 15:42:49,869 - INFO # round-trip min/avg/max = 8.367/8.465/8.557 ms

Issues/PRs references

@benpicco benpicco requested review from miri64 and jue89 as code owners June 13, 2022 13:01
@benpicco benpicco requested a review from jia200x June 13, 2022 13:01
@github-actions github-actions bot added the Area: drivers Area: Device drivers label Jun 13, 2022
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jun 13, 2022
@benpicco benpicco force-pushed the slip_dose_rxqueue branch from 9616555 to 34a3523 Compare June 13, 2022 14:26
@benpicco benpicco requested review from dylad and keestux as code owners June 13, 2022 14:26
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Jun 13, 2022
@benpicco benpicco mentioned this pull request Jun 13, 2022
Closed
@benpicco benpicco requested a review from chrysn June 14, 2022 08:52
@benpicco benpicco changed the title drivers/{dose, slipdev}: generate RX event for queued packets drivers/{dose, slipdev, sam0_eth}: generate RX event for queued packets Jun 14, 2022
maribu
maribu approved these changes Jun 15, 2022
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. Code change makes sense and I trust your testing

jia200x
jia200x requested changes Jun 15, 2022
View reviewed changes
drivers/slipdev/slipdev.c Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the slip_dose_rxqueue branch from 34a3523 to 2583239 Compare June 17, 2022 08:51
@benpicco benpicco mentioned this pull request Jun 17, 2022
Closed
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 11, 2022
@benpicco benpicco mentioned this pull request Jul 22, 2022
Closed
@jia200x
Copy link
Member

jia200x commented Aug 23, 2022

code looks reasonable. Could you test these device drivers on more time before merging?

@benpicco
Copy link
Contributor Author

Yes this fixes the issue.
(Although for SLIP I would prefer #18066)

@jia200x
Copy link
Member

jia200x commented Aug 26, 2022

Yes this fixes the issue.
(Although for SLIP I would prefer #18066)

Should we then merge this one without the SLIP fix and get #18066 in?

@benpicco
Copy link
Contributor Author

I can just rebase #18066 if this gets merged

jia200x
jia200x approved these changes Aug 26, 2022
View reviewed changes
Copy link
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

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

ACK. Code makes sense and I trust your testing

@benpicco benpicco merged commit 28cedd5 into RIOT-OS:master Aug 26, 2022
@benpicco benpicco deleted the slip_dose_rxqueue branch August 26, 2022 11:25
@benpicco
Copy link
Contributor Author

Thank you for the review!
Glad to have this bug fix.

@benpicco benpicco mentioned this pull request Aug 26, 2022
Closed
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@jia200x jia200x jia200x approved these changes

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

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

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

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

@chrysn chrysn Awaiting requested review from chrysn

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2022.10
Development

Successfully merging this pull request may close these issues.
3 participants
@benpicco @jia200x @maribu