Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Mon, Feb 03, 2014 at 09:54:09AM +, David Laight wrote: > From: Mark Lord > > On 14-02-01 09:18 AM, Ming Lei wrote: > > > > > > Even real regressions are easily/often introduced, and we are discussing > > > how to fix that. I suggest to unset the flag only for the known buggy > > > controllers. > > > > It is not the controllers that are particularly "buggy" here. > > But rather the drivers and design of parts of the kernel. > > I suspect that the documentation is describing the actual implementation > of a specific hardware implementation, not necessarily how the hardware was > intended to behave. You are speculating. Please stop speculating without evidence. It does not add to this conversation. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Sat, Feb 01, 2014 at 03:05:21PM -0500, Mark Lord wrote: > On 14-02-01 09:18 AM, Ming Lei wrote: > > > > Even real regressions are easily/often introduced, and we are discussing > > how to fix that. I suggest to unset the flag only for the known buggy > > controllers. Ming, the regression cannot be easily fixed in this case. We tried the "easy, quick fix" and it broke USB storage and usbfs. The patches to paper over those issues started to creep into the upper layers, and I'm not willing to add more code to hack around the issues caused by the "quick fix". We need to do this right, not wall-paper over the issues. > It is not the controllers that are particularly "buggy" here. > But rather the drivers and design of parts of the kernel. As Mark mentioned, the host controllers aren't buggy. The xHCI driver simply doesn't handle a 1.0 host controller requirement, TD fragments, very well. Only the USB ethernet layer triggers this bug, because the USB storage layer hands down scatter-gather lists in multiples of the max packet size. You tested on a 1.0 host controller, and it apparently didn't need the TD fragments requirement. It seems that Intel 1.0 xHCI host controllers do need that requirement. Perhaps we can add an xHCI driver quirk for an exception so that your host can allow any kind of scatter-gather? Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Mark Lord
> On 14-02-01 09:18 AM, Ming Lei wrote:
> >
> > Even real regressions are easily/often introduced, and we are discussing
> > how to fix that. I suggest to unset the flag only for the known buggy
> > controllers.
>
> It is not the controllers that are particularly "buggy" here.
> But rather the drivers and design of parts of the kernel.
I suspect that the documentation is describing the actual implementation
of a specific hardware implementation, not necessarily how the hardware was
intended to behave.
The requirement for two 32bit accesses to a 64bit register is very similar.
This also means that implementations of the hardware that claim conformance
to the 0.96 specification might have similar issues.
Given the small number of xhci controllers and the even smaller number of
VHDL (or similar) sources they will be based on, it really ought to be
possible to tabulate the controller versions and families to get a much
better idea of their behaviour.
I've got two systems with Intel USB3 controllers, linux reports one as
'panther point', the other as '7 Series/C210 Series' (seems to be a Xeon
chipset). I've no idea how the latter relates to the former.
David
N�r��yb�X��ǧv�^�){.n�+{��^n�r���z���h�&�� �G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On 14-02-01 09:18 AM, Ming Lei wrote: > > Even real regressions are easily/often introduced, and we are discussing > how to fix that. I suggest to unset the flag only for the known buggy > controllers. It is not the controllers that are particularly "buggy" here. But rather the drivers and design of parts of the kernel. Cheers -- Mark Lord Real-Time Remedies Inc. ml...@pobox.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Sat, Feb 1, 2014 at 9:30 PM, Mark Lord wrote: > On 14-02-01 02:54 AM, Ming Lei wrote: > .. >> With SG enabled, for the iperf client test case, the average urb size >> for transmission will be increased from ~1500 to ~20K bytes in my >> test case: >> >>iperf -c $SRV -t 30 -P 4 -w 128K >> >> So I am wondering you guys do not care the improvement .. > > No, that's not it. Simply, the recent changes killed the driver I just want to clarify the sg approach does improve performance, instead of no improvement mentioned by your guys. > for some users, something Linus calls a "regression", and does not permit. Even real regressions are easily/often introduced, and we are discussing how to fix that. I suggest to unset the flag only for the known buggy controllers. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On 14-02-01 02:54 AM, Ming Lei wrote: .. > With SG enabled, for the iperf client test case, the average urb size > for transmission will be increased from ~1500 to ~20K bytes in my > test case: > >iperf -c $SRV -t 30 -P 4 -w 128K > > So I am wondering you guys do not care the improvement .. No, that's not it. Simply, the recent changes killed the driver for some users, something Linus calls a "regression", and does not permit. Far better to have it continue to work than not to work. The plan discussed earlier calls for reintroduction of SG here once the problems are solved outside of the main tree. -- Mark Lord Real-Time Remedies Inc. ml...@pobox.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Sat, Feb 1, 2014 at 3:00 AM, Sarah Sharp wrote: > On Fri, Jan 31, 2014 at 08:17:58AM +0800, Ming Lei wrote: >> On Fri, Jan 31, 2014 at 6:15 AM, Sarah Sharp >> wrote: >> > On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: >> >> FWIW, the plan looks fine to me. Just adding a couple of hints to >> >> simplify the implementation. >> >> >> >> Sarah Sharp writes: >> >> >> >> > Let's do this fix the right way, instead of wall papering over the >> >> > issue. Here's what we should do: >> >> > >> >> > 1. Disable scatter-gather for the ax88179_178a driver when it's under an >> >> >xHCI host. >> >> >> >> No need to make this conditional. SG is only enabled in the >> >> ax88179_178a driver if udev->bus->no_sg_constraint is true, so it >> >> applies only to xHCI hosts in the first place. >> > >> > Ah, so you're suggesting just reverting commit >> > 3804fad45411b48233b48003e33a78f290d227c8 "USBNET: ax88179_178a: enable >> > tso if usb host supports sg dma"? >> >> If I understand the problem correctly, the current issue is that xhci driver >> doesn't support the arbitrary dma length not well, but per XHCI spec, it >> should be supported, right? >> >> If the above is correct, reverting the commit isn't correct since there isn't >> any issue about the commit, so I suggest to disable the flag in xhci >> for the buggy devices, and it may be enabled again if the problem is fixed. > > Ok, I like that plan, since it means I don't have to touch any > networking code to fix this. :) > > I believe that means we'll have to disable the flag for all 1.0 xHCI > hosts, since those are the ones that need TD fragments. > >> >> > 2. Revert the following commits: >> >> >f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block >> >> > writes. >> >> >d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many >> >> > trbs >> >> >35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload >> >> > burst >> >> > >> >> > 3. Dan and Mathias can work together to come up with an overall plan to >> >> >change the xHCI driver architecture to be fully compliant with the TD >> >> >fragment rules. That can be done over the next few kernel releases. >> >> > >> >> > The end result is that we don't destabilize storage or break userspace >> >> > USB drivers, we don't break people's xHCI host controllers, >> >> > the ax88179_178a USB ethernet devices still work under xHCI (a bit with >> >> > worse performance), and other USB ethernet devices still get the >> >> > performance improvement introduced in 3.12. >> >> >> >> No other usbnet drivers has enabled SG... Which is why you have only >> >> seen this problem with the ax88179_178a devices. So there is no >> >> performance improvement to keep. >> >> In my test environment, the patch does improve both throughput and >> cpu utilization, if you search the previous email for the patch, you can >> see the data. With SG enabled, for the iperf client test case, the average urb size for transmission will be increased from ~1500 to ~20K bytes in my test case: iperf -c $SRV -t 30 -P 4 -w 128K So I am wondering you guys do not care the improvement, maybe the CPU is powerful enough to not degrade throughout&cpu utilization not much, but there is still the potential CPU wakeup issue, which means extra CPU power consumption might be introduced after disabling SG for usbnet. > > Right, I did see the performance improvement note in that commit. Do > you know if the ARM A15 dual core board was using a 0.96 xHCI host, or a > 1.0 host? You can find out by reloading the xHCI driver with dynamic > debugging turned on: > > # sudo modprobe xhci_hcd dyndbg Looks I can't find the parameter 'dyndbg' for xhci_hcd. > and then look for lines like: > > [25296.765767] xhci_hcd :00:14.0: HCIVERSION: 0x100 I change xhci_dbg.c manually with below: diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c index b016d38..1ae1966 100644 --- a/drivers/usb/host/xhci-dbg.c +++ b/drivers/usb/host/xhci-dbg.c @@ -66,7 +66,7 @@ static void xhci_print_cap_regs(struct xhci_hcd *xhci) (unsigned int) temp); xhci_dbg(xhci, "CAPLENGTH: 0x%x\n", (unsigned int) HC_LENGTH(temp)); - xhci_dbg(xhci, "HCIVERSION: 0x%x\n", + dev_info(xhci_to_hcd(xhci)->self.controller, "HCIVERSION: 0x%x\n", (unsigned int) HC_VERSION(temp)); and got the below output: [tom@ming ~]$ dmesg | grep HCIVERSION xhci-hcd xhci-hcd.2.auto: HCIVERSION: 0x100 Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Fri, Jan 31, 2014 at 08:17:58AM +0800, Ming Lei wrote: > On Fri, Jan 31, 2014 at 6:15 AM, Sarah Sharp > wrote: > > On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: > >> FWIW, the plan looks fine to me. Just adding a couple of hints to > >> simplify the implementation. > >> > >> Sarah Sharp writes: > >> > >> > Let's do this fix the right way, instead of wall papering over the > >> > issue. Here's what we should do: > >> > > >> > 1. Disable scatter-gather for the ax88179_178a driver when it's under an > >> >xHCI host. > >> > >> No need to make this conditional. SG is only enabled in the > >> ax88179_178a driver if udev->bus->no_sg_constraint is true, so it > >> applies only to xHCI hosts in the first place. > > > > Ah, so you're suggesting just reverting commit > > 3804fad45411b48233b48003e33a78f290d227c8 "USBNET: ax88179_178a: enable > > tso if usb host supports sg dma"? > > If I understand the problem correctly, the current issue is that xhci driver > doesn't support the arbitrary dma length not well, but per XHCI spec, it > should be supported, right? > > If the above is correct, reverting the commit isn't correct since there isn't > any issue about the commit, so I suggest to disable the flag in xhci > for the buggy devices, and it may be enabled again if the problem is fixed. Ok, I like that plan, since it means I don't have to touch any networking code to fix this. :) I believe that means we'll have to disable the flag for all 1.0 xHCI hosts, since those are the ones that need TD fragments. > >> > 2. Revert the following commits: > >> >f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block > >> > writes. > >> >d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many > >> > trbs > >> >35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload > >> > burst > >> > > >> > 3. Dan and Mathias can work together to come up with an overall plan to > >> >change the xHCI driver architecture to be fully compliant with the TD > >> >fragment rules. That can be done over the next few kernel releases. > >> > > >> > The end result is that we don't destabilize storage or break userspace > >> > USB drivers, we don't break people's xHCI host controllers, > >> > the ax88179_178a USB ethernet devices still work under xHCI (a bit with > >> > worse performance), and other USB ethernet devices still get the > >> > performance improvement introduced in 3.12. > >> > >> No other usbnet drivers has enabled SG... Which is why you have only > >> seen this problem with the ax88179_178a devices. So there is no > >> performance improvement to keep. > > In my test environment, the patch does improve both throughput and > cpu utilization, if you search the previous email for the patch, you can > see the data. Right, I did see the performance improvement note in that commit. Do you know if the ARM A15 dual core board was using a 0.96 xHCI host, or a 1.0 host? You can find out by reloading the xHCI driver with dynamic debugging turned on: # sudo modprobe xhci_hcd dyndbg and then look for lines like: [25296.765767] xhci_hcd :00:14.0: HCIVERSION: 0x100 Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Sarah Sharp > On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: > > FWIW, the plan looks fine to me. Just adding a couple of hints to > > simplify the implementation. > > > > Sarah Sharp writes: > > > > > Let's do this fix the right way, instead of wall papering over the > > > issue. Here's what we should do: > > > > > > 1. Disable scatter-gather for the ax88179_178a driver when it's under an > > >xHCI host. > > > > No need to make this conditional. SG is only enabled in the > > ax88179_178a driver if udev->bus->no_sg_constraint is true, so it > > applies only to xHCI hosts in the first place. Leave the usbnet code alone and unset udev->bus->no_sg_constraint. David -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Peter Stuge [mailto:pe...@stuge.se] > > Userspace doesn't care since everything gets copied into aligned > > kernel fragments - otherwise the other usb controllers wouldn't work. > > OK, but not so great if someone wants to squeeze the most performance > possible out of USB also from userspace. > > I'm going off on a tangent now but would it make sense to allow > userspace to do alignment if it wants to, and have a way to tell > the kernel when urb buffers are pre-aligned? I can only see that mattering if either: 1) The userspace buffers are (say) 4n+1 aligned and the kernel decides to align the copy_from_user(). 2) The code is doing buffer-loaning. Personally I'm not at all sure how often buffer-loaning helps (given the cost of the TLB shootdowns that it often implies). I guess it might be ok if the memory doesn't have to be given a KVA and the user program avoids any COW. In any case and such code could be limited to page-aligned transfers. And/or the user code would have to know at least some of the constraints. The other usb controllers only support 'message' aligned transfers (512 bytes for USB2). All the code I've found achieves this by using page aligned (maybe 4k aligned) fragments. xhci trivially supports this (it has since the 'ring expansion' code was added). xhci can also easily support: 1) Arbitrary fragmentation for a limited number of fragments (by constraining the fragments to a single ring segment). 2) Arbitrary fragmentation provided all the fragments (except the last) exceed some minimal length (by splitting the current or previous fragment at the appropriate boundary). The code for the second is probably worth adding just in case a 4k fragment crosses a 64k boundary. Since arbitrarily fragmented packets can't be sent to other controllers it does seem sensible for the code generating the urb so say that it is (or might be) fragmented like that. David -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
David Laight wrote: > > We shouldn't need to make userspace start to worry about alignment at > > all. libusb worked in the past, before the link TRB fix went in. We > > *cannot* break userspace USB drivers. The breakage needs to be fixed in > > the USB core or the xHCI driver. > > Userspace doesn't care since everything gets copied into aligned > kernel fragments - otherwise the other usb controllers wouldn't work. OK, but not so great if someone wants to squeeze the most performance possible out of USB also from userspace. I'm going off on a tangent now but would it make sense to allow userspace to do alignment if it wants to, and have a way to tell the kernel when urb buffers are pre-aligned? //Peter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Sarah Sharp > We need to step back and reassess the larger picture here, instead of > trying to fire-fight all the regressions caused by the link TRB commit > (35773dac5f86 "usb: xhci: Link TRB must not occur within a USB payload > burst"). Some of the breakage seems to have been related to the PM and readq/writeq changes. The main problem with that patch is that it limited the number of fragments. > We shouldn't need to make userspace start to worry about alignment at > all. libusb worked in the past, before the link TRB fix went in. We > *cannot* break userspace USB drivers. The breakage needs to be fixed in > the USB core or the xHCI driver. Userspace doesn't care since everything gets copied into aligned kernel fragments - otherwise the other usb controllers wouldn't work. > Commit 35773dac5f86 was meant to be a short-term bandaid fix, but it's > already caused at least four different regressions. Some we've fixed, > some have proposed solutions that David has sent. > > The storage layer is getting borked because it submits scatter-gather > lists longer than what will fit on a segment, and now libusb has the > same issue. One xHCI host controller stopped responding to commands, > and reverting the bandaid fix helped. The implications of this change > just keep coming in, and I'm not comfortable wall-papering over the > issues. The transfers from the storage layer are actually all 'suitably aligned'. Fragments can cross 64k boundaries, but they all start on 4k boundaries. > On the flip side, it seems that the only devices that have been helped > by the bandaid fix patch are USB ethernet devices using the ax88179_178a > driver. (Mark Lord still needs to confirm which device he uses.) I > have not seen any other reports that other USB ethernet chipsets were > broken in 3.12 by the USB networking layer adding scatter-gather > support. That is the only usbnet driver for which SG support is enabled. I believe it was all enabled because the ax88179_178a is the only one that can be expected to saturate Ge and supports TCP segmentation offload (TSO) - where the buffer is almost always fragmented. With TSO transmits are almost certainly single fragments. > It should not matter what alignment or length of scatter-gather list the > upper layers pass the xHCI driver, it should just work. I want to do > this fix right, by changing the fundamental way we queue TRBs to the > rings to fit the TD rules. We should break each TD into fragment-sized > chunks, and add a link TRB in the middle of a segment where necessary. There will always be some transfer requests that make this impossible (without using a bounce buffer). In practise nothing will send such bad transfers. To avoid excessive work you need to be told whether the transfer is aligned (everything from the block layer and libusb is) or misaligned (potentially everything from usbnet). The limits on the number length of SG list has to be different for the two types of request. > Let's do this fix the right way, instead of wall papering over the > issue. Here's what we should do: > > 1. Disable scatter-gather for the ax88179_178a driver when it's under an >xHCI host. That can be done by simple not setting the flag on the xhci driver. However you also need to double check that this disables TSO. > 2. Revert the following commits: >f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block writes. >d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many trbs >35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload burst > > 3. Dan and Mathias can work together to come up with an overall plan to >change the xHCI driver architecture to be fully compliant with the TD >fragment rules. That can be done over the next few kernel releases. Don't forget that these rules can affect isoc transfers as well. Even without SG, the buffer can cross a 64k boundary and thus need splitting into separate TRB - which might need to be in different ring segments. Easily fixable by writing the LINK early (expect that the TRB writing code looks for LINK TRBs). > The end result is that we don't destabilize storage or break userspace > USB drivers, we don't break people's xHCI host controllers, > the ax88179_178a USB ethernet devices still work under xHCI (a bit with > worse performance), and other USB ethernet devices still get the > performance improvement introduced in 3.12. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Peter Stuge > But what about that alignment? It seems that userspace > needs to start caring about alignment with xhci, right? No because there is a copy_to/from_user() in the middle. The ehci/ohci/uhci all require that fragments be a multiple of the usb message size (512 bytes for USB2). So everything (until very recently) would always supply suitable aligned buffers. Mostly they are page aligned. For those who haven't read the xhci spec carefully: The xhci controller removes the requirement on dma segments being aligned to usb messages. However there are two alignment requirements: 1) dma segments must not cross 64k address boundaries. This is documented clearly, even though it is a slight pain. You'd have thought the address counter could have more than 16 bits these days! There only 17 bits for the length, but a length restriction would be less of a problem. 2) The v1.00 version of the specification adds that the end of the transfer ring can only occur at a 'TD fragment' boundary. These are aligned with the payload 'bursts' - which can be sixteen 1k packets. I think that breaking the second of these causes a usb message be split into two small pieces - which will terminate bulk xfers. The asix USB3 Ge silicon gets very confused when this happens. David -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Fri, Jan 31, 2014 at 6:15 AM, Sarah Sharp wrote: > On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: >> FWIW, the plan looks fine to me. Just adding a couple of hints to >> simplify the implementation. >> >> Sarah Sharp writes: >> >> > Let's do this fix the right way, instead of wall papering over the >> > issue. Here's what we should do: >> > >> > 1. Disable scatter-gather for the ax88179_178a driver when it's under an >> >xHCI host. >> >> No need to make this conditional. SG is only enabled in the >> ax88179_178a driver if udev->bus->no_sg_constraint is true, so it >> applies only to xHCI hosts in the first place. > > Ah, so you're suggesting just reverting commit > 3804fad45411b48233b48003e33a78f290d227c8 "USBNET: ax88179_178a: enable > tso if usb host supports sg dma"? If I understand the problem correctly, the current issue is that xhci driver doesn't support the arbitrary dma length not well, but per XHCI spec, it should be supported, right? If the above is correct, reverting the commit isn't correct since there isn't any issue about the commit, so I suggest to disable the flag in xhci for the buggy devices, and it may be enabled again if the problem is fixed. > >> > 2. Revert the following commits: >> >f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block >> > writes. >> >d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many >> > trbs >> >35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload >> > burst >> > >> > 3. Dan and Mathias can work together to come up with an overall plan to >> >change the xHCI driver architecture to be fully compliant with the TD >> >fragment rules. That can be done over the next few kernel releases. >> > >> > The end result is that we don't destabilize storage or break userspace >> > USB drivers, we don't break people's xHCI host controllers, >> > the ax88179_178a USB ethernet devices still work under xHCI (a bit with >> > worse performance), and other USB ethernet devices still get the >> > performance improvement introduced in 3.12. >> >> No other usbnet drivers has enabled SG... Which is why you have only >> seen this problem with the ax88179_178a devices. So there is no >> performance improvement to keep. In my test environment, the patch does improve both throughput and cpu utilization, if you search the previous email for the patch, you can see the data. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: > FWIW, the plan looks fine to me. Just adding a couple of hints to > simplify the implementation. > > Sarah Sharp writes: > > > Let's do this fix the right way, instead of wall papering over the > > issue. Here's what we should do: > > > > 1. Disable scatter-gather for the ax88179_178a driver when it's under an > >xHCI host. > > No need to make this conditional. SG is only enabled in the > ax88179_178a driver if udev->bus->no_sg_constraint is true, so it > applies only to xHCI hosts in the first place. Ah, so you're suggesting just reverting commit 3804fad45411b48233b48003e33a78f290d227c8 "USBNET: ax88179_178a: enable tso if usb host supports sg dma"? > > 2. Revert the following commits: > >f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block writes. > >d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many trbs > >35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload > > burst > > > > 3. Dan and Mathias can work together to come up with an overall plan to > >change the xHCI driver architecture to be fully compliant with the TD > >fragment rules. That can be done over the next few kernel releases. > > > > The end result is that we don't destabilize storage or break userspace > > USB drivers, we don't break people's xHCI host controllers, > > the ax88179_178a USB ethernet devices still work under xHCI (a bit with > > worse performance), and other USB ethernet devices still get the > > performance improvement introduced in 3.12. > > No other usbnet drivers has enabled SG... Which is why you have only > seen this problem with the ax88179_178a devices. So there is no > performance improvement to keep. I see. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Thu, 30 Jan 2014, Sarah Sharp wrote: > > That's a good plan. However _some_ restriction will turn out to be > > necessary. > > > > For example, what will you do if a driver submits an SG list containing > > 300 elements, each 3 bytes long? That's too many to fit in a single > > ring segment, but it's smaller than a TD fragment -- it's even smaller > > than maxpacket -- so there's no place to split it. (Not that I think > > drivers _will_ submit requests like this; this is just to demonstrate > > the point.) > > > > It ought to be acceptable to require, for example, that an SG URB > > contain no more than (say) 100 elements that are smaller than 512 > > bytes. > > At that point, the xHCI driver or USB core should probably use a bounce > buffer. It feels like we should attempt to push down scatter-gather > lists as far down in the stack as possible, so the upper layers don't > have to care what alignment, length, or random 64KB boundary splits we > need. Okay. That should be doable, if awkward. > > ehci-hcd gets along okay with the restriction that each SG element > > except the last has to be a multiple of the maxpacket size. xhci-hcd > > can relax this quite a lot, but not all the way. > > What does the EHCI driver do when it receives a SG list from the USB > networking layer that violates this restriction? It never receives such lists. usb_submit_urb() returns -EINVAL before the request gets sent to ehci-hcd. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
Sarah Sharp writes: > On Thu, Jan 30, 2014 at 04:43:54PM -0500, Alan Stern wrote: > >> ehci-hcd gets along okay with the restriction that each SG element >> except the last has to be a multiple of the maxpacket size. xhci-hcd >> can relax this quite a lot, but not all the way. > > What does the EHCI driver do when it receives a SG list from the USB > networking layer that violates this restriction? The USB networking layer won't use SG with the EHCI driver. Commit bcc48f1a7a0d4 introduced no_sg_constraint so that usbnet could enable SG only for host controllers with no such restrictions. I.e. currently for xHCI only. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Thu, Jan 30, 2014 at 04:43:54PM -0500, Alan Stern wrote: > On Thu, 30 Jan 2014, Sarah Sharp wrote: > > > It should not matter what alignment or length of scatter-gather list the > > upper layers pass the xHCI driver, it should just work. I want to do > > this fix right, by changing the fundamental way we queue TRBs to the > > rings to fit the TD rules. We should break each TD into fragment-sized > > chunks, and add a link TRB in the middle of a segment where necessary. > > That's a good plan. However _some_ restriction will turn out to be > necessary. > > For example, what will you do if a driver submits an SG list containing > 300 elements, each 3 bytes long? That's too many to fit in a single > ring segment, but it's smaller than a TD fragment -- it's even smaller > than maxpacket -- so there's no place to split it. (Not that I think > drivers _will_ submit requests like this; this is just to demonstrate > the point.) > > It ought to be acceptable to require, for example, that an SG URB > contain no more than (say) 100 elements that are smaller than 512 > bytes. At that point, the xHCI driver or USB core should probably use a bounce buffer. It feels like we should attempt to push down scatter-gather lists as far down in the stack as possible, so the upper layers don't have to care what alignment, length, or random 64KB boundary splits we need. > ehci-hcd gets along okay with the restriction that each SG element > except the last has to be a multiple of the maxpacket size. xhci-hcd > can relax this quite a lot, but not all the way. What does the EHCI driver do when it receives a SG list from the USB networking layer that violates this restriction? Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
FWIW, the plan looks fine to me. Just adding a couple of hints to simplify the implementation. Sarah Sharp writes: > Let's do this fix the right way, instead of wall papering over the > issue. Here's what we should do: > > 1. Disable scatter-gather for the ax88179_178a driver when it's under an >xHCI host. No need to make this conditional. SG is only enabled in the ax88179_178a driver if udev->bus->no_sg_constraint is true, so it applies only to xHCI hosts in the first place. > 2. Revert the following commits: >f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block writes. >d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many trbs >35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload burst > > 3. Dan and Mathias can work together to come up with an overall plan to >change the xHCI driver architecture to be fully compliant with the TD >fragment rules. That can be done over the next few kernel releases. > > The end result is that we don't destabilize storage or break userspace > USB drivers, we don't break people's xHCI host controllers, > the ax88179_178a USB ethernet devices still work under xHCI (a bit with > worse performance), and other USB ethernet devices still get the > performance improvement introduced in 3.12. No other usbnet drivers has enabled SG... Which is why you have only seen this problem with the ax88179_178a devices. So there is no performance improvement to keep. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On 14-01-30 04:43 PM, Alan Stern wrote: > On Thu, 30 Jan 2014, Sarah Sharp wrote: > >> It should not matter what alignment or length of scatter-gather list the >> upper layers pass the xHCI driver, it should just work. I want to do >> this fix right, by changing the fundamental way we queue TRBs to the >> rings to fit the TD rules. We should break each TD into fragment-sized >> chunks, and add a link TRB in the middle of a segment where necessary. > > That's a good plan. However _some_ restriction will turn out to be > necessary. > > For example, what will you do if a driver submits an SG list containing > 300 elements, each 3 bytes long? Allocate a contiguous (bounce) buffer and copy the fragments to/from it? -- Mark Lord Real-Time Remedies Inc. ml...@pobox.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Thu, 30 Jan 2014, Sarah Sharp wrote: > It should not matter what alignment or length of scatter-gather list the > upper layers pass the xHCI driver, it should just work. I want to do > this fix right, by changing the fundamental way we queue TRBs to the > rings to fit the TD rules. We should break each TD into fragment-sized > chunks, and add a link TRB in the middle of a segment where necessary. That's a good plan. However _some_ restriction will turn out to be necessary. For example, what will you do if a driver submits an SG list containing 300 elements, each 3 bytes long? That's too many to fit in a single ring segment, but it's smaller than a TD fragment -- it's even smaller than maxpacket -- so there's no place to split it. (Not that I think drivers _will_ submit requests like this; this is just to demonstrate the point.) It ought to be acceptable to require, for example, that an SG URB contain no more than (say) 100 elements that are smaller than 512 bytes. ehci-hcd gets along okay with the restriction that each SG element except the last has to be a multiple of the maxpacket size. xhci-hcd can relax this quite a lot, but not all the way. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
Sarah, on a related note: Is there a parameter or knob of some kind to tell the XHCI driver to treat a specific port as USB2 (480mbit/sec max) rather than USB3 ? The Dell XPS-13 Ultrabooks all suffer from some kind of flaw, whereby the left side USB3 port is unreliable at SuperSpeed; the right side port works flawlessly. The MS-Windows driver has a workaround of some sort, but we don't. Cheers -- Mark Lord Real-Time Remedies Inc. ml...@pobox.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On 14-01-30 04:18 PM, Sarah Sharp wrote: > > Let's do this fix the right way, instead of wall papering over the > issue. Here's what we should do: > > 1. Disable scatter-gather for the ax88179_178a driver when it's under an >xHCI host. > > 2. Revert the following commits: >f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block writes. >d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many trbs >35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload burst > > 3. Dan and Mathias can work together to come up with an overall plan to >change the xHCI driver architecture to be fully compliant with the TD >fragment rules. That can be done over the next few kernel releases. > > The end result is that we don't destabilize storage or break userspace > USB drivers, we don't break people's xHCI host controllers, > the ax88179_178a USB ethernet devices still work under xHCI (a bit with > worse performance), and other USB ethernet devices still get the > performance improvement introduced in 3.12. Performance before 3.12/3.13 was not all that bad either. My ax88179 dongle (yes, that one, using ax88179_178a.ko) manages very close to 1gbit/sec throughput even without SG, and without a huge cpu tax either. SG done Right will make it better eventually. I can wait. Cheers -- Mark Lord Real-Time Remedies Inc. ml...@pobox.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Thu, Jan 30, 2014 at 05:35:08PM +0100, Peter Stuge wrote: > David Laight wrote: > > > Where's the 8k coming from? > > > > My memory, I meant 16k :-( > > No problem. But what about that alignment? It seems that userspace > needs to start caring about alignment with xhci, right? We need to step back and reassess the larger picture here, instead of trying to fire-fight all the regressions caused by the link TRB commit (35773dac5f86 "usb: xhci: Link TRB must not occur within a USB payload burst"). We shouldn't need to make userspace start to worry about alignment at all. libusb worked in the past, before the link TRB fix went in. We *cannot* break userspace USB drivers. The breakage needs to be fixed in the USB core or the xHCI driver. Commit 35773dac5f86 was meant to be a short-term bandaid fix, but it's already caused at least four different regressions. Some we've fixed, some have proposed solutions that David has sent. The storage layer is getting borked because it submits scatter-gather lists longer than what will fit on a segment, and now libusb has the same issue. One xHCI host controller stopped responding to commands, and reverting the bandaid fix helped. The implications of this change just keep coming in, and I'm not comfortable wall-papering over the issues. On the flip side, it seems that the only devices that have been helped by the bandaid fix patch are USB ethernet devices using the ax88179_178a driver. (Mark Lord still needs to confirm which device he uses.) I have not seen any other reports that other USB ethernet chipsets were broken in 3.12 by the USB networking layer adding scatter-gather support. It should not matter what alignment or length of scatter-gather list the upper layers pass the xHCI driver, it should just work. I want to do this fix right, by changing the fundamental way we queue TRBs to the rings to fit the TD rules. We should break each TD into fragment-sized chunks, and add a link TRB in the middle of a segment where necessary. Let's do this fix the right way, instead of wall papering over the issue. Here's what we should do: 1. Disable scatter-gather for the ax88179_178a driver when it's under an xHCI host. 2. Revert the following commits: f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block writes. d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many trbs 35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload burst 3. Dan and Mathias can work together to come up with an overall plan to change the xHCI driver architecture to be fully compliant with the TD fragment rules. That can be done over the next few kernel releases. The end result is that we don't destabilize storage or break userspace USB drivers, we don't break people's xHCI host controllers, the ax88179_178a USB ethernet devices still work under xHCI (a bit with worse performance), and other USB ethernet devices still get the performance improvement introduced in 3.12. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
David Laight wrote: > > Where's the 8k coming from? > > My memory, I meant 16k :-( No problem. But what about that alignment? It seems that userspace needs to start caring about alignment with xhci, right? //Peter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Peter Stuge ... > > code using libusb can generate arbitrarily long transfers that usually > > get split into 8k fragments. > > libusb splits transfers into 16k urbs, or doesn't with newer code > when both kernel and libusb support scatter-gather. > > > In fact libusb always uses 8k fragments. > > Hm? Worst-case libusb-1.0 submits 16k urbs. libusb-0.1 I'm unsure > about, but could check. ... > Where's the 8k coming from? My memory, I meant 16k :-( David -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
David Laight wrote: > Some xhci (USB3) controllers have a constraint on the offset within a > bulk transfer of the end of the transfer ring. Mhm. > code using libusb can generate arbitrarily long transfers that usually > get split into 8k fragments. libusb splits transfers into 16k urbs, or doesn't with newer code when both kernel and libusb support scatter-gather. > In fact libusb always uses 8k fragments. Hm? Worst-case libusb-1.0 submits 16k urbs. libusb-0.1 I'm unsure about, but could check. When both sides support it, scatter-gather is used and a single urb is submitted. IIRC usbfs doesn't mess with urb buffers at all. Where's the 8k coming from? > This all means that the xhci driver needs to accept unlimited numbers > of 'aligned' fragments and only restrict the number of misaligned ones. libusb applications have so far never made efforts to align their buffers to anything. That seems to become relevant for zero-copy? //Peter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
