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

drivers/slipdev: make use of chunked ringbuffer #18066

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented May 5, 2022

Contribution description

Just a test to see if this can increase performance with high data ingress.

But turns out this also solves two nasty SLIP bugs:

Testing procedure

SLIP should work as before, ideally a bit better.

Issues/PRs references

alternative to #18826, #18229

@github-actions github-actions bot added Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration labels May 5, 2022
@miri64
Copy link
Member

miri64 commented May 9, 2022

Just a test to see if this can increase performance with high data ingress.

Any results yet?

@benpicco benpicco force-pushed the slipdev-chunked_rb branch from 7dc1283 to b1c19c4 Compare May 9, 2022 11:22
@benpicco
Copy link
Contributor Author

benpicco commented May 9, 2022

Any results yet?

So far no improvements, but also no worse than master. (I tested examples/benchmark_udp and for i in {10..20}; do echo $i | nc -u -6 -w0 2001:dbc::e4ea:aff8:af5d:f8e5 1234; done and counted the RX frames)
But I didn't yet tests conditions with higher CPU load yet.

@benpicco
Copy link
Contributor Author

Uh so this turns out to fix a gnarly pktbuf corruption issue with SLIP

@benpicco benpicco force-pushed the slipdev-chunked_rb branch from 4ed7888 to 88db0e9 Compare June 23, 2022 13:31
@benpicco benpicco requested a review from kfessel June 23, 2022 14:24
drivers/slipdev/stdio.c Outdated Show resolved Hide resolved
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

From my POV (non-user of this driver):

  • the change improves it:
    reading in chunks enables the networkstack to check the size of the next chunck prior to requesting it making loss of packages less likely
  • and the readability (there is only one place the unescaping is done)

@kfessel
Copy link
Contributor

kfessel commented Jul 27, 2022

@benpicco: you may squash and make this ready for review

@benpicco benpicco force-pushed the slipdev-chunked_rb branch from 180a449 to a379450 Compare July 27, 2022 09:44
@benpicco benpicco marked this pull request as ready for review July 27, 2022 09:44
@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 27, 2022
@benpicco
Copy link
Contributor Author

benpicco commented Oct 4, 2022

All squashed now.

@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Oct 13, 2022
miri64
miri64 previously requested changes Oct 25, 2022
drivers/slipdev/stdio.c Show resolved Hide resolved
drivers/slipdev/slipdev.c Show resolved Hide resolved
@miri64 miri64 dismissed their stale review October 25, 2022 10:47

Seems like my worries were misplaced.

@benpicco benpicco 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 25, 2022
@riot-ci
Copy link

riot-ci commented Nov 1, 2022

Murdock results

✔️ PASSED

1a888fe drivers/slipdev: make use of chunked ringbuffer

Success Failures Total Runtime
8100 0 8100 10m:06s

Artifacts

@leandrolanzieri leandrolanzieri 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 Nov 1, 2022
@benpicco benpicco requested a review from fabian18 November 27, 2023 16:48
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

Even if it may not be a performance increase,I can understand the new code better than the old code.

drivers/slipdev/slipdev.c Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the slipdev-chunked_rb branch from f54d381 to 1a888fe Compare January 2, 2024 13:41
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

BOARD=same54-xpro UPLINK=slip make flash term (gnrc_border_router)

> ping -c 10 -s 1000 fe80::1%6 
ping -c 10 -s 1000 fe80::1%6
1008 bytes from fe80::1%6: icmp_seq=0 ttl=64 time=185.192 ms
ps
1008 bytes from fe80::1%6: icmp_seq=1 ttl=64 time=185.191 ms
1008 bytes from fe80::1%6: icmp_seq=2 ttl=64 time=185.160 ms
1008 bytes from fe80::1%6: icmp_seq=3 ttl=64 time=185.192 ms
1008 bytes from fe80::1%6: icmp_seq=4 ttl=64 time=185.189 ms
1008 bytes from fe80::1%6: icmp_seq=5 ttl=64 time=185.169 ms
1008 bytes from fe80::1%6: icmp_seq=6 ttl=64 time=185.157 ms
1008 bytes from fe80::1%6: icmp_seq=7 ttl=64 time=185.172 ms
1008 bytes from fe80::1%6: icmp_seq=8 ttl=64 time=185.264 ms
1008 bytes from fe80::1%6: icmp_seq=9 ttl=64 time=185.176 ms

--- fe80::1%6 PING statistics ---
10 packets transmitted, 10 packets received, 0% packet loss
round-trip min/avg/max = 185.157/185.186/185.264 ms
> ps
	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
	  - | isr_stack            | -        - |   - |    512 (  256) (  256) | 0x20000000 | 0x200001c8
	  1 | main                 | running  Q |   7 |   1536 (  936) (  600) | 0x20000320 | 0x20000624 
	  2 | 6lo                  | bl rx    _ |   3 |   1024 (  256) (  768) | 0x20004664 | 0x20004964 
	  3 | ipv6                 | bl rx    _ |   4 |   1024 (  496) (  528) | 0x200009fc | 0x20000c6c 
	  4 | udp                  | bl rx    _ |   5 |    512 (  256) (  256) | 0x20004a64 | 0x20004b64 
	  5 | sam0_eth             | bl anyfl _ |   2 |    896 (  296) (  600) | 0x20001988 | 0x20001c4c 
	  6 | slipdev              | bl anyfl _ |   2 |    896 (  280) (  616) | 0x20001e90 | 0x20002154 
	    | SUM                  |            |     |   6400 ( 2776) ( 3624)

  • works fine IMO
  • code got cleaner
  • PR has been open for a long time now and we also have been using it internally for that time

so let's have it merged

@benpicco benpicco added this pull request to the merge queue Jan 4, 2024
Merged via the queue into RIOT-OS:master with commit 9b3c39a Jan 4, 2024
26 checks passed
@benpicco benpicco deleted the slipdev-chunked_rb branch January 4, 2024 14:07
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants