Ticket #137 (closed enhancement: fixed)

Opened 11 months ago

Last modified 6 weeks ago

take advantage of new kernel support for large transfers

Reported by: parafin Owned by:
Milestone: 1.0.16 Component: libusb-1.0 Linux backend
Keywords: Cc:
Blocked By: Blocks:

Description

Currently if bulk transfer larger than 16 KB is submitted, it's splitted into several URBs not larger than 16KB. Comments in code suggest, that Linux can't handle larger buffers, which is incorrect (at least now, maybe it was true in the past). I've changed MAX_BULK_BUFFER_LENGTH from 16384 to 1048576 (1 MB) and nothing broke (see attached patch). Actually it solved problems I've been having with USB3. Splitting big transfers increase interrupt and memory load and it seams that xhci kernel driver doesn't handle this situation well which ends in data corruption.
Same issue probably applies to isochronous transfers.

Attachments

libusb-1-increase_buffer_size.patch (450 bytes) - added by parafin 11 months ago.

Download all attachments as: .zip

Change History

Changed 11 months ago by parafin

comment:1 follow-up: Changed 11 months ago by stuge

  • Summary changed from transfers are being splitted to small URBs for no reason to take advantage of new kernel support for large transfers
  • Type changed from defect to enhancement

The reason to split transfers is that older kernels only supported 16kb transfers anyway.

This has changed and is changing even more right this moment, as Hans is working on new usbfs API which will make large transfers from userspace both simpler and more efficient. I believe he may also do the libusb side work for this, so let's see how that turns out.

Increasing the (by now) arbitrary limit of 16kb to another arbitrary limit, although larger, isn't a very nice solution. The kernel work being done is the real fix.

We just have to be careful to remain backwards compatible. libusb has to continue working fine even on the way old kernels which only do 16kb transfers.

In any case thank you for the ticket, this is an important improvement for USB 3 on Linux!

comment:2 in reply to: ↑ 1 Changed 11 months ago by hansdegoede

Replying to stuge:

This has changed and is changing even more right this moment, as Hans is working on new usbfs API which will make large transfers from userspace both simpler and more efficient. I believe he may also do the libusb side work for this, so let's see how that turns out.

Correct, see my patch here:
http://sourceforge.net/mailarchive/message.php?msg_id=29475836

As indicated on that thread the new kernel-APIs this depends on are not yet upstream (but they have got a positive reception, so I expect them to go upstream soon).

As for raising the limit to 1 MB, this is a very bad idea, usbfs needs a bounce buffer to copy the userspace buffer data to/from, and this buffer needs to be dma-able and thus physically contiguous, the chances of there being a 1MB physically contiguous area of memory free on a system which has been running for a while is actually very small, so transfers of 1MB will likely fail when not split. My kernel work allows submitting these kind of transfers in one go by using scatter-gather lists to split the transfer up inside the kernel on usb controllers which support scatter-gather. Note that even with this kernel patch it is still a bad idea to submit large transfers on non scatter-gather capable controllers. So my patchset also adds a new kernel API to query the devices capabilities, and teaches libusb how to use it.

comment:3 follow-up: Changed 11 months ago by timrprobocom

Replying to Hans:

... this buffer needs to be dma-able and thus physically contiguous ...

All USB host controllers support scatter/gather DMA. The kernel-side buffer does not need to be physically contiguous.

comment:4 in reply to: ↑ 3 Changed 11 months ago by hansdegoede

Replying to timrprobocom:

Replying to Hans:

... this buffer needs to be dma-able and thus physically contiguous ...

All USB host controllers support scatter/gather DMA. The kernel-side buffer does not need to be physically contiguous.

<sigh>, it would have been helpful if you read further down before shooting of a quick comment from the hip. I've been only working on USB for a couple of years now, and on the large bulkt ransfers with XHCI controllers issue for a full month...

Besides knowing what I'm talking about the answer to your comment is actually further down in the comment you're replying on: "My kernel work allows submitting these kind of transfers in one go by using scatter-gather lists to split the transfer up inside the kernel on usb controllers which support scatter-gather."

IOW yes the controllers are scatter-gather capable, but the usbfs layer in the kernel is (was) not using this, since until recently the urb data size was limited to 16k, so there was no need to use scatter-gather.

See here for the kernel patches adding scatter-gather support to usbfs:
http://www.spinics.net/lists/linux-usb/msg66567.html

comment:5 Changed 11 months ago by parafin

I found out that 16 KB limit was dropped only in 3.3 kernel (I thought it was much earlier), so of course my patch is a no-go as is. But at least it allowed me to test my code.
I'm very much looking forward to actual fix, I hope it will get into kernel upstream quickly.

comment:6 follow-up: Changed 8 months ago by parafin

As I can see from links below, necessary changes have made it into release candidates of kernel and libusbx:
http://www.mail-archive.com/libusbx-devel@lists.sourceforge.net/msg01147.html
https://github.com/libusbx/libusbx/commit/a109ae8205f0bfc5600711b821d520d6570b2243
https://github.com/libusbx/libusbx/commit/ede02ba91920f9be787a7f3cd006c5a4b92b5eab
So, what are the plans for incorporating these patches into libusb? Though I will probably just switch to libusbx once 3.6 kernel is released.

comment:7 in reply to: ↑ 6 Changed 8 months ago by stuge

Replying to parafin:

what are the plans for incorporating these patches into libusb?

They're significant improvements that will certainly be included into libusb.

comment:8 Changed 3 months ago by hjelmn

Peter, any objection to pulling the two referenced patches in for 1.0.16?

comment:9 Changed 3 months ago by hjelmn

  • Milestone set to 1.0.16

comment:10 Changed 7 weeks ago by hjelmn

Since there i no objection I pulled these commits into libusb-darwin.

comment:11 Changed 6 weeks ago by hjelmn

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.