Skip to content

sys/usb/cdc_acm, sys/tsrb: speedup usb_cdc ringbuffer handling #21788

Merged
benpicco merged 4 commits intoRIOT-OS:masterfrom
kfessel:p-usb-turb
Nov 7, 2025
Merged

sys/usb/cdc_acm, sys/tsrb: speedup usb_cdc ringbuffer handling #21788
benpicco merged 4 commits intoRIOT-OS:masterfrom
kfessel:p-usb-turb

Conversation

@kfessel
Copy link
Copy Markdown
Contributor

@kfessel kfessel commented Oct 13, 2025

Contribution description

speedup usb_cdc_acm tsrb writing and reading by avoid jumps to tsrb and superfluous irq_disable calls.

Testing procedure

read
usb cdc acm still working

Issues/PRs references

#12402 fixed tsrb hadling for usb ( loss of chars for acm) by adding irq disable around tsrb access
#15218 added isr_guards thoughout tsrb

@github-actions github-actions bot added Area: USB Area: Universal Serial Bus Area: sys Area: System labels Oct 13, 2025
@kfessel kfessel requested a review from maribu October 13, 2025 10:11
kfessel
kfessel commented Oct 13, 2025
View reviewed changes
sys/tsrb/tsrb.c Outdated
Comment on lines +62 to +69
size_t tmp = n;
int cnt = 0;
unsigned irq_state = irq_disable();
while (tmp && !tsrb_empty(rb)) {
while (n-- && !tsrb_empty(rb)) {
*dst++ = _pop(rb);
tmp--;
cnt++;
}
irq_restore(irq_state);
return (n - tmp);
return (cnt);
Copy link
Copy Markdown
Contributor Author

@kfessel kfessel Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

why are you renaming and retyping variables and restructure their use?
-- this question goes for other function you do the same thing to as well

Copy link
Copy Markdown
Contributor Author

@kfessel kfessel Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

rename to ease readbility
retyping to fit return type
slightly change their use for readability for humans and compilers godbolt

Copy link
Copy Markdown
Contributor

@Teufelchen1 Teufelchen1 Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: You could go the extra mile and rename n to length/size/remaining_bytes and cnt to copied_bytes 😏

@kfessel kfessel force-pushed the p-usb-turb branch 2 times, most recently from a9ea365 to 6ddd967 Compare October 13, 2025 10:27
@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 13, 2025
@riot-ci
Copy link
Copy Markdown

riot-ci commented Oct 13, 2025 •
edited
Loading

Murdock results

✔️ PASSED

d893fc9 sys/usb/cdc_acm: replace tsrb with turb

Success Failures Total Runtime
10913 0 10915 10m:10s

Artifacts

@crasbe
Copy link
Copy Markdown
Contributor

crasbe commented Oct 14, 2025

Do you have any benchmark numbers or so to show that it actually improves the speed?

Also I'm not to keen on the turb header idea, IMO it would be more elegant to add tsrb_functionname_unsafe or so in the same header. This would avoid a bit of duplication I guess.

Teufelchen1
Teufelchen1 reviewed Oct 15, 2025
View reviewed changes
Teufelchen1
Teufelchen1 reviewed Oct 15, 2025
View reviewed changes
@kfessel
Copy link
Copy Markdown
Contributor Author

kfessel commented Oct 15, 2025 •
edited
Loading

Do you have any benchmark numbers or so to show that it actually improves the speed?

Also I'm not to keen on the turb header idea, IMO it would be more elegant to add tsrb_functionname_unsafe or so in the same header. This would avoid a bit of duplication I guess.

i would not like to have them accessible through the same header ( could go for tsrb_unsafe.h though thread safe ... unsafe sound kindof strange ( first modifications went that route but then turb became) )

benchmarking the irq enable and disable -which this is mostly about- is very difficult we got the debug_irq_disable but it itself bogs down the irq enable and disable very much especially when its called as often as in the usb function ( basically with usb_acm the debug_irq_disable was not usable even when i replaced its printout) with this change it is

when i did debug_irq_disable the two functions where regularly exceeding 5000 cycles on an same5 mcu @120MHz with this change they are barely exceeding 1500 (rest on the application stayed the same) -- but as i said this does not tell much as debug_irq_disable cause a bunch of these cycles and its mostly gone with this change.

kfessel
kfessel commented Oct 16, 2025
View reviewed changes
sys/include/turb.h
/*
* SPDX-FileCopyrightText: 2015 Kaspar Schleiser <[email protected]>
* SPDX-FileCopyrightText: 2025 Karl Fessel <[email protected]> ML!PA Consulting GmbH
* SPDX-License-Identifier: LGPL-2.1-only
Copy link
Copy Markdown
Contributor Author

@kfessel kfessel Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i am a bit unsure about that only -- just a bit if its ok like this its fine for me

@maribu
Copy link
Copy Markdown
Member

maribu commented Oct 16, 2025

What is the motivation to have this in addition to https://doc.riot-os.org/cib_8h.html ?

Wouldn't cib already solve all the non-thread-safe ringbuffer needs?

@kfessel
Copy link
Copy Markdown
Contributor Author

kfessel commented Oct 16, 2025 •
edited
Loading

What is the motivation to have this in addition to https://doc.riot-os.org/cib_8h.html ?

Wouldn't cib already solve all the non-thread-safe ringbuffer needs?

cib seems to have a very strange usage pattern, does not do the buffer handling but just the index,

  • has a put function that does not put
  • a get function that does not get
  • is called buffer without addressing the buffer or its content

turb is created to provide access to tsrb without extra thread safety measures ( cause they are taken/ensured by the caller )

  • it extends the interface to tsrb and is useful for mixed access ( tsrb and turb functions combined)

  • tsrb (and thus turb) could probaly have used cib for its counter handling needs (they seem to work slightly different but similar) but the author of tsrb probably saw cib and decided its better to not do that

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Nov 6, 2025

I suppose you could now implement tsrb by wrapping turb calls with irq_disable()/irq_restore(), right?

@kfessel
Copy link
Copy Markdown
Contributor Author

kfessel commented Nov 7, 2025 •
edited
Loading

I suppose you could now implement tsrb by wrapping turb calls with irq_disable()/irq_restore(), right?

done that

sys/tsrb: use turb.h for c-file implemetation
replaces the prior
sys/tsrb: ease readability

@benpicco benpicco added this pull request to the merge queue Nov 7, 2025
Merged via the queue into RIOT-OS:master with commit a6cfc3f Nov 7, 2025
26 checks passed
@kfessel
Copy link
Copy Markdown
Contributor Author

kfessel commented Nov 10, 2025

Thanks

@leandrolanzieri leandrolanzieri added this to the Release 2026.01 milestone Jan 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crasbe crasbe crasbe left review comments

@Teufelchen1 Teufelchen1 Teufelchen1 left review comments

@benpicco benpicco benpicco approved these changes

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

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

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

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

@maribu maribu Awaiting requested review from maribu

Assignees

No one assigned

Labels

Area: sys Area: System Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Milestone

Release 2026.01

Development

Successfully merging this pull request may close these issues.

7 participants

@kfessel @riot-ci @crasbe @maribu @benpicco @Teufelchen1 @leandrolanzieri