-
Notifications
You must be signed in to change notification settings - Fork 200
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
access.direct.demultiplexer: add ability for applets to override default USB transfer settings. #742
base: main
Are you sure you want to change the base?
Conversation
…ult USB transfer settings. Add four keyword arguments to DirectDemultiplexerInterface's constructor: read_packets_per_xfer, read_xfers_per_queue, write_packets_per_xfer, write_xfers_per_queue An applet can set these values through device.demultiplexer.claim_interface's **kwargs.
…B settings on applets that require fixed throughput without overflows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the investigation. It's been a while since I looked at this code in depth, and I'm unsure if this is really the best way to solve it, especially given the analyzer's many other deficiencies.
At this moment other commitments do not leave me enough time to discuss a better approach in depth. Are you in the Matrix channel? If so we should talk about it at some point when I do have more time and work out a way forward.
In particular, it's not clear to me that we can't just change the defaults so that any applet gets a buffer holding 5-10 ms of data. This does go against the DMA memory quotas in the Linux kernel, but:
|
(Also, I think the fixed-throughput applet should perhaps be a part of the benchmark applet instead? It does perform a benchmark, and I don't like adding more top-level names unless there's good justification.) |
Thanks for the fast response!
No problem! It was time well spent, I learned a number of new things about USB in the process.
No worries about not discussing more now. I'm not on matrix, but I am on the 1BitSquared Discord as "SamIAm", so I see Matrix messages via the bridge.
A potential default change sounds good to me. I think one tradeoff could be latency for applets that don't use auto flush / explicitly flush.
I'm sad about the 16 MB limit. Its the default on my system (Ubuntu 24.04, Kernel 6.8). Its easy enough to increase by running
Moving the fixed-throughput applet into the benchmark applet makes sense and I'm happy to make that change. |
Thank you for the responsiveness! I really value this kind of contribution.
My view is that any properly designed applet should be using either auto-flush or explicitly flush. Doing neither and just relying on data to 'fall off the FIFO' does happen to work but it is expected to introduce undesirable performance characteristics.
Ah, that's upsetting. I really don't want to get involved with LKML (especially not after the latest events) so I guess the default gets to stay what it is. Do you want to explore adding a warning asking for this limit to be raised? I think once you exceed a limit you get a certain error from libusb and we could show the warning once we hit that. I recall this error to be annoyingly ambiguous but we could do what we can, now that we collectively know that it is worth bumping against the limit. |
Sure I'll look into detecting the error/making a warning on exceeding the limit. I don't remember what the error was offhand, but I definitely ran into it a few times when I made a mistake calculating the request size. |
Thanks, perfect! If you slice your changes into individual PRs I will likely be able to review and merge them faster (the new benchmark for example could be merged right away as far as I can tell from a very quick glance). |
The first commit in this PR adds a way for an applet to override the default USB transfer size / the number of transfers queued on a
DirectDemultiplexerInterface
at a time.The motivation for this change is that while the default values
_packets_per_xfer
and_xfers_per_queue
work well for most applets, they aren't ideal for applets where backpressure results in the loss of data, such as the analyzer applet. In this case, I found that requesting larger transfers sized to hold ~5-10 ms of data allowed for a higher throughput withoutin_fifo
overflows.The second commit in this PR contains an applet called
fixed-throughput
. This applet outputs data from the device at a fixed rate, and checks forin_fifo
overflows. I used it to test the effect of different combinations of transfer size and transfers queued.The code block below shows the results of some measurements I took. With the default settings, and a rate of 100 Mbps, the fixed-throughput applet overflowed after ~6 seconds. By changing the settings, the fixed-throughput applet was able to run for ~40 minutes at a rate of 300 Mbps without overflows before I terminated it.