usbdmx: Add support for having multiple inflight transfers #1627

DanielG · 2020-04-02T19:45:01Z

In developing a custom USB->DMX widget for OLA I've noticed that packet latency isn't as consistent as possible when only using a single libusb transfer at a time. Hence here I add support for more than one and also fix a nasty use after free hazard while I'm at it.

Commit messages copied below:

`usbdmx: Add support for having multiple inflight transfers`

Currently we only use a single active transfer for both sending and
reveiving usb packets.

This is suboptimal in terms of latency, USB actually guarantees certain
turnaround times for interrupt transfers for example but that only really
works if there is always an IRP ready to go in the USB host controller.

According to my measurments interrupt transfers with bInterval=1 can be
delayed by some tens of milliseconds instead of happening evey millisecond
on the dot if there are periods with no transfers ready to go.

In order to support multiple transfers being inflight I replace the
transfer state machinery in AsyncUsbTransceiverBase with a set of inflight
transfers and a queue of idle transfers.

The bookkeeping happens in a new TransferCompleteInternal method which
wraps the TransferComplete() method provided by subclasses. This split also
allowed moving some duplicated logic for LIBUSB_TRANSFER_NO_DEVICE to the
superclass.

The number of transfers to allow is passed to AsyncUsb{Sender,Receiver} as
a constructor argument since supporting more than one needs some
cooperation from the derived classes. Currently everything just defaults to
one transfer which shouldn't change the behaviour one bit.

`usbdmx: Remove redundant CancelTransfer() calls`:

The superclass destructor already calls CancelTransfer, no need to do it in
the derived classes also. To prevent this in the future this also make
CancelTransfer private.

`usbdmx: Fix use after free hazard in destructor`

(see diff for commentary)

DanielG · 2020-04-03T15:06:26Z

Changes:

Actually {fire off,allow} up to the maximum number of transfers in the {receiver,sender}. Since PerformTransfer() is handled in the AsyncUsb{Sender,Receiver} classes we have to do the transfer count handling there.

In theory this should mean widget specific classes shouldn't even be able to notice what the transfer queue depth actually is, so maybe we can just turn this on across all widgets.

DanielG · 2020-04-06T11:47:17Z

Changes:

Fix the use-after-free fix :)
AsyncUsb{Sender,Receiver} have to call CancelTransfer, not the base class, because they close the device handle and we must also not free the device before the transfers complete!

peternewman

Some initial comments

plugins/usbdmx/AsyncUsbReceiver.cpp

plugins/usbdmx/AsyncUsbReceiver.h

plugins/usbdmx/AsyncUsbSender.cpp

peternewman · 2021-02-13T03:11:20Z

plugins/usbdmx/AsyncUsbTransceiverBase.cpp

+    inflight_set::iterator i = m_inflight.find(transfer);
+    if (i == m_inflight.end()) { // not found
+      OLA_WARN << "Mismatched libusb transfer: "
+               << transfer << " not in inflight set";
+      return;
+    }
+
+    m_inflight.erase(i);


Would this be easier replacing with our ola::STLLookupAndRemove? http://docs.openlighting.org/ola/doc/latest/group__stl.html#ga7a896801adfcea56e630bd5007c135e1

That doesn't seem to work for sets? Even if it would it's a bit akward. Since STLLookupAndRemove doesn't check if the value arg is NULL I'd have to provide a dummy.

./include/ola/stl/STLUtils.h:393:6: note: candidate: ‘template<class T1> bool ola::STLLookupAndRemove(T1*, const typename T1::key_type&, typename T1::mapped_type*)’ bool STLLookupAndRemove(T1 *container, ^~~~~~~~~~~~~~~~~~ ./include/ola/stl/STLUtils.h:393:6: note: template argument deduction/substitution failed: plugins/usbdmx/AsyncUsbTransceiverBase.cpp:52:59: note: mismatched types ‘T1*’ and ‘std::set<libusb_transfer*>’ if (STLLookupAndRemove(m_inflight, transfer, transfer_)) { // not found

Apologies, wrong command, after looking more carefully ola::STLRemove will do what you want:
http://docs.openlighting.org/ola/doc/latest/group__stl.html#ga6bab5568a6d6ef14d4faccea9eddd41b

Or is there a risk of multiple equal transfers in the set (I assume they have a unique ID, but maybe not)?

Or is there a risk of multiple equal transfers in the set (I assume they have a unique ID, but maybe not)?

Reading more carefully, they have to be unique, so no risk there.

Sorry to push back on this, but honestly I don't see STLRemove having any advantage over calling .erase() here. All this change would do is invalidate my testing and perform another unnecessary traversal to find the right element again. A combined lookup and remove would have cleaned up the code a bit but for sure, but just with just the removal I don't see the point.

plugins/usbdmx/AsyncUsbTransceiverBase.cpp

plugins/usbdmx/AsyncUsbTransceiverBase.h

DanielG · 2021-02-22T08:23:52Z

Changes:

Fix typo emtpy -> empty

DanielG · 2021-02-22T08:25:18Z

Changes:

Fix cpplint complaint about two spaces after code, before comment.

plugins/usbdmx/AsyncUsbTransceiverBase.cpp

peternewman · 2021-02-28T14:45:02Z

plugins/usbdmx/AsyncUsbTransceiverBase.cpp

+    inflight_set::iterator i = m_inflight.find(transfer);
+    if (i == m_inflight.end()) { // not found
+      OLA_WARN << "Mismatched libusb transfer: "
+               << transfer << " not in inflight set";
+      return;
+    }
+
+    m_inflight.erase(i);


Apologies, wrong command, after looking more carefully ola::STLRemove will do what you want:
http://docs.openlighting.org/ola/doc/latest/group__stl.html#ga6bab5568a6d6ef14d4faccea9eddd41b

Or is there a risk of multiple equal transfers in the set (I assume they have a unique ID, but maybe not)?

peternewman

I still can't get use to C++'s pop not being like this:
https://perldoc.perl.org/functions/pop

A few more, mostly minor, comments.

Currently we only use a single active transfer for both sending and reveiving usb packets. This is suboptimal in terms of latency, USB actually guarantees certain turnaround times for interrupt transfers for example but that only really works if there is always an IRP ready to go in the USB host controller. According to my measurments interrupt transfers with bInterval=1 can be delayed by some tens of milliseconds instead of happening evey millisecond on the dot if there are periods with no transfers ready to go. In order to support multiple transfers being inflight I replace the transfer state machinery in AsyncUsbTransceiverBase with a set of inflight transfers and a queue of idle transfers. The bookkeeping happens in a new TransferCompleteInternal method which wraps the TransferComplete() method provided by subclasses. This split also allowed moving some duplicated logic for LIBUSB_TRANSFER_NO_DEVICE to the superclass. The number of transfers to allow is passed to AsyncUsb{Sender,Receiver} as a constructor argument since supporting more than one needs some cooperation from the derived classes. Currently everything just defaults to one transfer which shouldn't change the behaviour one bit.

DanielG force-pushed the usbdmx-inflight branch from 70843f9 to 40a8aa5 Compare April 3, 2020 15:00

DanielG force-pushed the usbdmx-inflight branch 2 times, most recently from 009b767 to 4d32528 Compare April 6, 2020 11:45

DanielG force-pushed the usbdmx-inflight branch from 4d32528 to 1a0abb6 Compare April 6, 2020 13:37

DanielG force-pushed the usbdmx-inflight branch from 1a0abb6 to 8f0f2e3 Compare February 2, 2021 16:27

peternewman added this to the 0.11.0 milestone Feb 8, 2021

peternewman added Component-Plugin enhancement labels Feb 8, 2021

peternewman requested changes Feb 13, 2021

View reviewed changes

DanielG mentioned this pull request Feb 13, 2021

usbdmx: Move widget deletion to main thread #1712

Merged

DanielG force-pushed the usbdmx-inflight branch 2 times, most recently from e7dab7f to 39198c4 Compare February 22, 2021 08:23

DanielG force-pushed the usbdmx-inflight branch from 39198c4 to 5b3c5c7 Compare February 22, 2021 08:24

DanielG requested a review from peternewman February 28, 2021 08:06

peternewman reviewed Feb 28, 2021

View reviewed changes

peternewman requested changes Feb 28, 2021

View reviewed changes

DanielG force-pushed the usbdmx-inflight branch from 5b3c5c7 to 6147c2e Compare November 28, 2021 19:21

DanielG added 2 commits November 28, 2021 20:32

usbdmx: Fix use after free hazard in AsyncUsb* destructor

8255115

DanielG force-pushed the usbdmx-inflight branch from 6147c2e to 8255115 Compare November 28, 2021 19:32

DanielG requested a review from peternewman November 28, 2021 19:32

usbdmx: Add support for having multiple inflight transfers #1627

Are you sure you want to change the base?

usbdmx: Add support for having multiple inflight transfers #1627

Uh oh!

Conversation

DanielG commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

usbdmx: Add support for having multiple inflight transfers

usbdmx: Remove redundant CancelTransfer() calls:

usbdmx: Fix use after free hazard in destructor

Uh oh!

DanielG commented Apr 3, 2020

Uh oh!

DanielG commented Apr 6, 2020

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peternewman Feb 13, 2021

Choose a reason for hiding this comment

Uh oh!

DanielG Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

peternewman Feb 28, 2021

Choose a reason for hiding this comment

Uh oh!

peternewman Feb 28, 2021

Choose a reason for hiding this comment

Uh oh!

DanielG Nov 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanielG commented Feb 22, 2021

Uh oh!

DanielG commented Feb 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peternewman Feb 28, 2021

Choose a reason for hiding this comment

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DanielG commented Apr 2, 2020 •

edited

Loading

`usbdmx: Add support for having multiple inflight transfers`

`usbdmx: Remove redundant CancelTransfer() calls`:

`usbdmx: Fix use after free hazard in destructor`

DanielG Nov 28, 2021 •

edited

Loading