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

[kfessel]

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

[kfessel]

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

[kfessel]

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

[Teufelchen1]

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

[riot-ci]

Murdock results

✔️ PASSED

ea64ee8 f-trub :spdx

Success	Failures	Total	Runtime
10552	0	10552	13m:12s

Artifacts

• Documentation preview

[crasbe]

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.

[kfessel]

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]

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

[maribu]

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]

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