usb: ehci: fix update qtd->token in qh_append_tds

From: Ming Lei<[email protected]>

This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...), see details from link of https://bugs.launchpad.net/bugs/709245.

In fact, one mb() on ARM is enough to flush L2 cache, but 'dummy->hw_token = token;' after mb() is added just for obeying correct mb() usage.

The patch has been tested ok on OMAP4 panda A1 board, the performance of 'dd' over usb mass storage can be increased from 4~5MB/sec to 14~16MB/sec after applying this patch.

Signed-off-by Ming Lei<[email protected]>

---
  drivers/usb/host/ehci-q.c |   14 ++++++++++++++
  1 files changed, 14 insertions(+), 0 deletions(-)

drivers/usb/host/ehci-q.c +4 -0

index 0917e3a..65b5021 100644
@@ -1082,6 +1082,20 @@ static struct ehci_qh *qh_append_tds (
  			wmb ();
  			dummy->hw_token = token;
 
+			/* The mb() below is added to make sure that
+			 * 'token' can be writen into qtd, so that ehci
+			 * HC can see the up-to-date qtd descriptor. On
+			 * some archs(at least on ARM Cortex A9 dual core),

Hi,

Quoted 8 lines · ming.lei at canonical.com

From: Ming Lei<[email protected]>

This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...),
see details from link of https://bugs.launchpad.net/bugs/709245.

Quoted 4 lines

In fact, one mb() on ARM is enough to flush L2 cache, but 'dummy->hw_token = token;' after mb() is added just for obeying correct mb() usage.

Who said "one mb() on ARM is enough to flush L2 cache" ? It's just a memory barrier and it doesn't flush any cache. What it cleans is the CPU write buffers and the L2 cache write buffers

Quoted 5 lines

The patch has been tested ok on OMAP4 panda A1 board, the performance of 'dd' over usb mass storage can be increased from 4~5MB/sec to 14~16MB/sec after applying this patch.

Though number looks great, how is the below patch helping to get better numbers.

Quoted 26 lines

Signed-off-by Ming Lei<[email protected]>

---
?drivers/usb/host/ehci-q.c | ? 14 ++++++++++++++ ?1 files changed, 14 insertions(+), 0 deletions(-)

drivers/usb/host/ehci-q.c +11 -0

index 0917e3a..65b5021 100644
@@ -1082,6 +1082,20 @@ static struct ehci_qh *qh_append_tds (
 ? ? ? ? ? ? ? ? ? ? ? ?wmb ();
 ? ? ? ? ? ? ? ? ? ? ? ?dummy->hw_token = token;
 
+ ? ? ? ? ? ? ? ? ? ? ? /* The mb() below is added to make sure that
+ ? ? ? ? ? ? ? ? ? ? ? ?* 'token' can be writen into qtd, so that ehci
+ ? ? ? ? ? ? ? ? ? ? ? ?* HC can see the up-to-date qtd descriptor. On
+ ? ? ? ? ? ? ? ? ? ? ? ?* some archs(at least on ARM Cortex A9 dual
 core),
+ ? ? ? ? ? ? ? ? ? ? ? ?* writing into coherenet memory doesn't mean the
+ ? ? ? ? ? ? ? ? ? ? ? ?* value written can reach physical memory
+ ? ? ? ? ? ? ? ? ? ? ? ?* immediately, and the value may be buffered
+ ? ? ? ? ? ? ? ? ? ? ? ?* inside L2 cache. 'dummy->hw_token = token;'
+ ? ? ? ? ? ? ? ? ? ? ? ?* after mb() is added for obeying correct mb()
+ ? ? ? ? ? ? ? ? ? ? ? ?* usage.
+ ? ? ? ? ? ? ? ? ? ? ? ?* */

drivers/usb/host/ehci-q.c static struct ehci_qh *qh_append_tds ( · line 1099

Quoted 3 lines

drivers/usb/host/ehci-q.c +3 -0

+ ? ? ? ? ? ? ? ? ? ? ? mb();
+ ? ? ? ? ? ? ? ? ? ? ? token = dummy->hw_token;
+

This patch at max fix some corruption if the memory buffer used is buffer-able. Infact I see there is already a write memory barrier above. So just pushing that down by one line should be enough.

drivers/usb/host/ehci-q.c static struct ehci_qh *qh_append_tds ( · line 1101

Quoted 3 lines

drivers/usb/host/ehci-q.c +0 -0

 ? ? ? ? ? ? ? ? ? ? ? dummy->hw_token = token;
 ? ? ? ? ? ? ? ? ? ? ? wmb ();

Is there another patch along with this which removes, some cache clean on this buffer ?

Quoted 11 lines · Santosh

Hi,

Quoted 8 lines · ming.lei at canonical.com

From: Ming Lei<[email protected]>

This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...),
see details from link of https://bugs.launchpad.net/bugs/709245.

Quoted 8 lines

Quoted 4 lines

In fact, one mb() on ARM is enough to flush L2 cache, but 'dummy->hw_token = token;' after mb() is added just for obeying correct mb() usage.

Who said "one mb() on ARM is enough to flush L2 cache" ? It's just a memory barrier and it doesn't flush any cache. What it cleans is the CPU write buffers and the L2 cache write buffers

Yes, your description is more accurate, it should be L2 write buffer, I see mb() will call outer_sync() on ARM.

Quoted 7 lines

Quoted 5 lines

The patch has been tested ok on OMAP4 panda A1 board, the performance of 'dd' over usb mass storage can be increased from 4~5MB/sec to 14~16MB/sec after applying this patch.

Though number looks great, how is the below patch helping to get better numbers.

The patch can make ehci HC see the up-to-date qtd, so make usb transaction executed correctly. If a qtd->token is not updated, maybe IOC is not set or set very
late, so interrupt can't be triggered in time, also mistaken 'total bytes to transfer'
can make HC work badly.

In fact, I have traced the problem and found ehci irq is often delayed by ehci HC.
also sometimes ehci irq is lost, so I start to trace ehci driver and find the problem here.

Quoted 26 lines

Quoted 26 lines

Signed-off-by Ming Lei<[email protected]>

---
  drivers/usb/host/ehci-q.c |   14 ++++++++++++++
  1 files changed, 14 insertions(+), 0 deletions(-)

drivers/usb/host/ehci-q.c +11 -0

index 0917e3a..65b5021 100644
@@ -1082,6 +1082,20 @@ static struct ehci_qh *qh_append_tds (
                        wmb ();
                        dummy->hw_token = token;
 
+                       /* The mb() below is added to make sure that
+                        * 'token' can be writen into qtd, so that ehci
+                        * HC can see the up-to-date qtd descriptor. On
+                        * some archs(at least on ARM Cortex A9 dual
 core),
+                        * writing into coherenet memory doesn't mean the
+                        * value written can reach physical memory
+                        * immediately, and the value may be buffered
+                        * inside L2 cache. 'dummy->hw_token = token;'
+                        * after mb() is added for obeying correct mb()
+                        * usage.
+                        * */

Quoted 8 lines

drivers/usb/host/ehci-q.c static struct ehci_qh *qh_append_tds ( · line 1098

Quoted 3 lines

drivers/usb/host/ehci-q.c +3 -0

+                       mb();
+                       token = dummy->hw_token;
+

This patch at max fix some corruption if the memory buffer used is buffer-able. Infact I see there is already a write memory barrier above. So just pushing that down by one line should be enough.

The above wmb is used to order updating qtd->hw_next and dummy->hw_token.

Quoted 1 line

Quoted 5 lines

drivers/usb/host/ehci-q.c static struct ehci_qh *qh_append_tds ( · line 1101

Quoted 2 lines

drivers/usb/host/ehci-q.c +0 -0

                       dummy->hw_token = token;
                       wmb ();

Is there another patch along with this which removes, some cache clean on this buffer ?

No, I am not sure the wmb should be merged with mb().

From: Ming Lei <[email protected]>

This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...), see details from link of https://bugs.launchpad.net/bugs/709245.

In fact, one mb() on ARM is enough to flush L2 cache, but 'dummy->hw_token = token;' after mb() is added just for obeying correct mb() usage.

The patch has been tested ok on OMAP4 panda A1 board, the performance of 'dd' over usb mass storage can be increased from 4~5MB/sec to 14~16MB/sec after applying this patch.

Signed-off-by Ming Lei <[email protected]>

---
 drivers/usb/host/ehci-q.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

drivers/usb/host/ehci-q.c +3 -0

index 0917e3a..65b5021 100644
@@ -1082,6 +1082,20 @@ static struct ehci_qh *qh_append_tds (
 			wmb ();
 			dummy->hw_token = token;
 
+			/* The mb() below is added to make sure that
+			 * 'token' can be writen into qtd, so that ehci
+			 * HC can see the up-to-date qtd descriptor. On

Quoted 6 lines · ming.lei at canonical.com

From: Ming Lei <[email protected]>

This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...), see details from link of https://bugs.launchpad.net/bugs/709245.

Quoted 3 lines

In fact, one mb() on ARM is enough to flush L2 cache, but 'dummy->hw_token = token;' after mb() is added just for obeying correct mb() usage.

Really? ?A mb() should not be flushing any caches, it's just a memory barrier. ?Or is ARM somehow "special" in this way?

Quoted 4 lines

The patch has been tested ok on OMAP4 panda A1 board, the performance of 'dd' over usb mass storage can be increased from 4~5MB/sec to 14~16MB/sec after applying this patch.

That's impressive, but I don't think this is really the proper way to do this...

Quoted 13 lines

Signed-off-by Ming Lei <[email protected]>

---
?drivers/usb/host/ehci-q.c | ? 14 ++++++++++++++ ?1 files changed, 14 insertions(+), 0 deletions(-)

drivers/usb/host/ehci-q.c +0 -0

index 0917e3a..65b5021 100644
@@ -1082,6 +1082,20 @@ static struct ehci_qh *qh_append_tds (
 ? ? ? ? ? ? ? ? ? ? ? wmb ();
 ? ? ? ? ? ? ? ? ? ? ? dummy->hw_token = token;
 

drivers/usb/host/ehci-q.c static struct ehci_qh *qh_append_tds ( · line 1097

Quoted 13 lines

drivers/usb/host/ehci-q.c +13 -0

+ ? ? ? ? ? ? ? ? ? ? /* The mb() below is added to make sure that
+ ? ? ? ? ? ? ? ? ? ? ?* 'token' can be writen into qtd, so that ehci
+ ? ? ? ? ? ? ? ? ? ? ?* HC can see the up-to-date qtd descriptor. On
+ ? ? ? ? ? ? ? ? ? ? ?* some archs(at least on ARM Cortex A9 dual core),
+ ? ? ? ? ? ? ? ? ? ? ?* writing into coherenet memory doesn't mean the
+ ? ? ? ? ? ? ? ? ? ? ?* value written can reach physical memory
+ ? ? ? ? ? ? ? ? ? ? ?* immediately, and the value may be buffered
+ ? ? ? ? ? ? ? ? ? ? ?* inside L2 cache. 'dummy->hw_token = token;'
+ ? ? ? ? ? ? ? ? ? ? ?* after mb() is added for obeying correct mb()
+ ? ? ? ? ? ? ? ? ? ? ?* usage.
+ ? ? ? ? ? ? ? ? ? ? ?* */
+ ? ? ? ? ? ? ? ? ? ? mb();
+ ? ? ? ? ? ? ? ? ? ? token = dummy->hw_token;

Your comment does not match the code, so something is wrong here.

Hi,

Quoted 11 lines · Greg KH

Quoted 9 lines · ming.lei at canonical.com

From: Ming Lei <[email protected]>

This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...), see details from link of https://bugs.launchpad.net/bugs/709245.

In fact, one mb() on ARM is enough to flush L2 cache, but 'dummy->hw_token = token;' after mb() is added just for obeying correct mb() usage.

Quoted 2 lines

Really? ?A mb() should not be flushing any caches, it's just a memory barrier. ?Or is ARM somehow "special" in this way?

As Santosh pointed out, mb on ARM will flush L2 write buffer. The description here is wrong.

I think the below should make the writing reach into memory on all ARCH after ' token = dummy->hw_token;' is executed.

                       dummy->hw_token = token;
                       mb()
                       token = dummy->hw_token;

The above is the idea introduced to fix the problem.

Quoted 20 lines

Quoted 3 lines

The patch has been tested ok on OMAP4 panda A1 board, the performance of 'dd' over usb mass storage can be increased from 4~5MB/sec to 14~16MB/sec after applying this patch.

That's impressive, but I don't think this is really the proper way to do this...

Quoted 13 lines

Signed-off-by Ming Lei <[email protected]>

---
?drivers/usb/host/ehci-q.c | ? 14 ++++++++++++++ ?1 files changed, 14 insertions(+), 0 deletions(-)

drivers/usb/host/ehci-q.c +0 -0

index 0917e3a..65b5021 100644
@@ -1082,6 +1082,20 @@ static struct ehci_qh *qh_append_tds (
 ? ? ? ? ? ? ? ? ? ? ? wmb ();
 ? ? ? ? ? ? ? ? ? ? ? dummy->hw_token = token;
 

Quoted 15 lines

drivers/usb/host/ehci-q.c static struct ehci_qh *qh_append_tds ( · line 1097

Quoted 13 lines

drivers/usb/host/ehci-q.c +13 -0

+ ? ? ? ? ? ? ? ? ? ? /* The mb() below is added to make sure that
+ ? ? ? ? ? ? ? ? ? ? ?* 'token' can be writen into qtd, so that ehci
+ ? ? ? ? ? ? ? ? ? ? ?* HC can see the up-to-date qtd descriptor. On
+ ? ? ? ? ? ? ? ? ? ? ?* some archs(at least on ARM Cortex A9 dual core),
+ ? ? ? ? ? ? ? ? ? ? ?* writing into coherenet memory doesn't mean the
+ ? ? ? ? ? ? ? ? ? ? ?* value written can reach physical memory
+ ? ? ? ? ? ? ? ? ? ? ?* immediately, and the value may be buffered
+ ? ? ? ? ? ? ? ? ? ? ?* inside L2 cache. 'dummy->hw_token = token;'
+ ? ? ? ? ? ? ? ? ? ? ?* after mb() is added for obeying correct mb()
+ ? ? ? ? ? ? ? ? ? ? ?* usage.
+ ? ? ? ? ? ? ? ? ? ? ?* */
+ ? ? ? ? ? ? ? ? ? ? mb();
+ ? ? ? ? ? ? ? ? ? ? token = dummy->hw_token;

Your comment does not match the code, so something is wrong here.

If you mean "L2 cache flush", I confess to the mistaken description, and will update it later. If you mean others, could you help to point it out?

Quoted 2 lines

As Santosh pointed out, mb on ARM will flush L2 write buffer. The description here is wrong.

Then this can't be accepted as-is :)

Quoted 8 lines

I think the below should make the writing reach into memory on all ARCH after ' token = dummy->hw_token;' is executed.

? ? ? ? ? ? ? ? ? ? ? ?dummy->hw_token = token;
? ? ? ? ? ? ? ? ? ? ? ?mb()
? ? ? ? ? ? ? ? ? ? ? ?token = dummy->hw_token;

The above is the idea introduced to fix the problem.

Are you sure? ?Have you read the documentation about memory barriers to confirm this?

I mean others, please read the the last 3 lines of the comment and compare that to the code lines you added.

Quoted 2 lines

Are you sure? ?Have you read the documentation about memory barriers to confirm this?

I read the doc again, :-), and it mentions few about mb/wmb/rmb, I think my above description is still not correct. Generally speaking, mb only means there is a order between two accesses.

Now I think only one mb() after 'dummy->hw_token = token;' is enough:
HC will read the up-to-date value of qtd->hw_token after mb() is executed because of the effect of the mb(), which should be guaranteed by mb.

Looks like there is still another similar problem in qh_link_async():
the last wmb
should be changed into mb, because HC will read 'head->hw->hw_next' from qh descriptor and this pointer in qh is read only for HC. But this problem can't be observed on ARM, since wmb on ARM is same with mb.

Quoted 5 lines · Ming Lei

Looks like there is still another similar problem in qh_link_async():
the last wmb
should be changed into mb, because HC will read 'head->hw->hw_next' from qh descriptor and this pointer in qh is read only for HC. But this problem can't be observed on ARM, since wmb on ARM is same with mb.

It doesn't matter what the HC does -- the wmb() instruction is executed by the CPU, not the HC. ?The point of that instruction is to make sure that the

? ? ? ?qh->hw->hw_next = head->hw->hw_next;

line (and all the preceding lines as well) is ordered before the

? ? ? ?head->hw->hw_next = dma;
3C
line. ?Since both of these lines are writes, not reads, it suffices to use wmb() rather than mb().

I read the doc again, :-), and it mentions few about mb/wmb/rmb, I think my above description is still not correct. Generally speaking, mb only means there is a order between two accesses.

Now I think only one mb() after 'dummy->hw_token = token;' is enough:
HC will read the up-to-date value of qtd->hw_token after mb() is executed because of the effect of the mb(), which should be guaranteed by mb.

Quoted 2 lines

I mean others, please read the the last 3 lines of the comment and compare that to the code lines you added.

I see now, the comment of the last 3 lines is wrong, should be

* inside L2 cache. 'token = dummy->hw_token'
* after mb() is added for obeying correct mb()
* usage.

But the 'token = dummy->hw_token' after mb() isn't needed any more as described above, is it?

The patch can make ehci HC see the up-to-date qtd, so make usb transaction executed correctly. If a qtd->token is not updated, maybe IOC is not set or set very late, so interrupt can't be triggered in time, also mistaken 'total bytes to transfer' can make HC work badly.

In fact, I have traced the problem and found ehci irq is often delayed by ehci HC. also sometimes ehci irq is lost, so I start to trace ehci driver and find the problem here.

Quoted 7 lines · Ming Lei

I read the doc again, :-), and it mentions few about mb/wmb/rmb, I think my above description is still not correct. Generally speaking, mb only means there is a order between two accesses.

Now I think only one mb() after 'dummy->hw_token = token;' is enough:
HC will read the up-to-date value of qtd->hw_token after mb() is executed because of the effect of the mb(), which should be guaranteed by mb.

I've been following this whole discussion. ?I didn't understand the idea behind the original patch, and I don't understand this. ?What reason is there for adding a memory barrier after the "dummy->hw_token = token" line? ?Such a barrier would not accomplish anything useful, because there are no later memory accesses which need to be ordered with respect to that line.

(By the way, Santosh appears to be right about the earlier wmb() in this routine. ?As far as I can see, it isn't needed. ?David Brownell probably wrote it just as a precaution.)

Quoted 9 lines

Quoted 2 lines

I mean others, please read the the last 3 lines of the comment and compare that to the code lines you added.

I see now, the comment of the last 3 lines is wrong, should be

? ? ? ? ? ? ? ? ? ? ? ? * inside L2 cache. 'token = dummy->hw_token' ? ? ? ? ? ? ? ? ? ? ? ? ?* after mb() is added for obeying correct mb() ? ? ? ? ? ? ? ? ? ? ? ? ?* usage.

Quoted 2 lines

But the 'token = dummy->hw_token' after mb() isn't needed any more as described above, ?is it?

I don't see why it was ever needed at all. ?The compiler will realize that token is a dead variable at that point in the routine and will optimize the new line away.

Quoted 5 lines

The patch can make ehci HC see the up-to-date qtd, so make usb transaction executed correctly. If a qtd->token is not updated, maybe IOC is not set or set very late, so interrupt can't be triggered in time, also mistaken 'total bytes to transfer' can make HC work badly.

This doesn't make any sense. ?qtd becomes the new dummy; by the time the HC sees qtd the only important bit set in qtd->hw_token is the HALT bit.

Quoted 3 lines

In fact, I have traced the problem and found ehci irq is often delayed by ehci HC. also sometimes ehci irq is lost, so I start to trace ehci driver and find the problem here.

I don't think you have identified the underlying cause correctly. Something else must be responsible.

Quoted 6 lines

I've been following this whole discussion. ?I didn't understand the idea behind the original patch, and I don't understand this. ?What reason is there for adding a memory barrier after the "dummy->hw_token = token" line? ?Such a barrier would not accomplish anything useful, because there are no later memory accesses which need to be ordered with respect to that line.

Here, mb is used to synchronize between writing of CPU and reading of ehci HC, see below:

	CPU								EHCI
	dummy->hw_token = token;
	mb
									read  dummy->hw_token

The mb() introduced was supposed to be used to make sure that EHCI can see the correct value of dummy->hw_token.

If EHCI can't get the correct
hw_token in time, it will work badly, such as irq delay or irq lost which may be caused by mistaken 'IOC' or 'total bytes to transfer' in hw_token.

This is just
what the patch is to fix.

But I think the above is still not correct, and the correct way may be like below:

	CPU								EHCI
	dummy->hw_token = token;
	wmb
	qh->hw->hw_qtd_next = QTD_NEXT(ehci, dummy->qtd_dma);

									fetch next qtd from qh->hw->hw_qtd_next
									read  qtd->hw_token

The problem is that qh has already linked dummy into its queue before(as did in qh_update), so even after 'dummy->hw_token = token;' is executed on CPU, EHCI still may not fetch the up-to-date value of the qtd(dummy in here), and this should
be the root cause, IMO.

It won't do that. All it will do is guarantee that the CPU writes out dumy->hw_token before it writes out or reads in any values executed after the mb.

Quoted 3 lines · Alan Stern

It won't do that. All it will do is guarantee that the CPU writes out dumy->hw_token before it writes out or reads in any values executed after the mb.

You're right from the perspective of how things are defined today. However, that isn't how things work on ARM.

With ARMv6 and ARMv7, we have weak memory ordering. This includes so called "DMA coherent" memory. This means that the architecture does not guarantee the order of writes to DMA coherent memory (which is non- cacheable normal memory) without an intervening 'data synchronization barrier' (dsb). Even that may not be sufficient without also poking at the L2 cache controller.

We get around some of that by ensuring that our MMIO read/write macros contain the necessary barriers to ensure that DMA memory is up to date before the DMA agent is programmed. However, this doesn't cater for agents which continue to run in the background.

These agents will need some kind of barrier to ensure that the write becomes visible - there's no way to get around that. Maybe we need yet another new barrier macro...

Hmmm. Although the semantics of the various mb() macros were originally defined only for inter-CPU synchronization, I believe they are also supposed to work for guaranteeing the order of accesses to DMA-coherent memory. If that's not the case with ARM, something is seriously wrong. (Maybe I'm wrong about this, but if I am then there's currently _no_ way for the kernel to order DMA-coherent accesses on ARM.)

You know better than I do what is needed to resolve the ordering issue. However, contrary to what the original patch description said, this isn't entirely a matter of making the write visible to the host
controller: No doubt in time the write will eventually become visible anyway. It's a matter of making the write become visible reasonably quickly and in the correct order with respect to other writes.

Is this extra L2-cache "poke" needed for proper ordering, or is it needed merely to flush the write out to memory in a timely manner?

Quoted 6 lines

You know better than I do what is needed to resolve the ordering issue. However, contrary to what the original patch description said, this isn't entirely a matter of making the write visible to the host
controller: No doubt in time the write will eventually become visible anyway. It's a matter of making the write become visible reasonably quickly and in the correct order with respect to other writes.

I'm not entirely sure what the problem is - I think its about a write by the CPU to dma coherent memory being delayed and not being visible to the HC in a timely manner. Either mb() or wmb() placed after the write on ARM will do that - and ARM has no requirement to do a read- back after the barrier.

Quoted 2 lines

Is this extra L2-cache "poke" needed for proper ordering, or is it needed merely to flush the write out to memory in a timely manner?

Both, though primerily it's about ensuring correct ordering. A side effect of it is that it will flush all pending writes in L2 before completing.

From the theoretical viewpoint, I think I'm right to say that mb() doesn't need to provide that level of ordering as its supposed to be an inter-CPU barrier - which probably means we need to invent a new barrier to deal with DMA memory ordering. However, given the difficulty of getting the existing barriers placed correctly, I don't think inventing new barriers is a very good idea.

What we can do is view devices which perform DMA as being strongly ordered with respect to their memory accesses - iow, they have an implicit memory barrier before and after their accesses to memory. This would make the CPUs use of mb() have a conceptual pairing with the DMA agents.

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines

You know better than I do what is needed to resolve the ordering issue. However, contrary to what the original patch description said, this isn't entirely a matter of making the write visible to the host
controller: No doubt in time the write will eventually become visible anyway. ?It's a matter of making the write become visible reasonably quickly and in the correct order with respect to other writes.

Quoted 5 lines

I'm not entirely sure what the problem is - I think its about a write by the CPU to dma coherent memory being delayed and not being visible to the HC in a timely manner. ?Either mb() or wmb() placed after the write on ARM will do that - and ARM has no requirement to do a read- back after the barrier.

Okay, then this needs to be done in a way that won't slow down other architectures with an unnecessary memory barrier. ?And there needs to be a comment in the code explaining that the new mb() instruction isn't being used as a memory barrier but rather to expedite writeback of the L2 cache.

This certainly is starting to sound like something that needs to be addressed in the arch-specific #include files...

Quoted 14 lines

Quoted 2 lines

Is this extra L2-cache "poke" needed for proper ordering, or is it needed merely to flush the write out to memory in a timely manner?

Both, though primerily it's about ensuring correct ordering. ?A side effect of it is that it will flush all pending writes in L2 before completing.

From the theoretical viewpoint, I think I'm right to say that mb() doesn't need to provide that level of ordering as its supposed to be an inter-CPU barrier - which probably means we need to invent a new barrier to deal with DMA memory ordering. ?However, given the difficulty of getting the existing barriers placed correctly, I don't think inventing new barriers is a very good idea.

Quoted 5 lines

What we can do is view devices which perform DMA as being strongly ordered with respect to their memory accesses - iow, they have an implicit memory barrier before and after their accesses to memory. This would make the CPUs use of mb() have a conceptual pairing with the DMA agents.

Yes, that's the model I have been using all along. ?After all, if a DMA master carries out its memory accesses in some random order then it's impossible for the CPU to make any guarantees.

Alan Stern

If writing to coherent memory can't reach physical memory immediately on other ARCHs, the problem can still happen on these ARCHs. But I am not sure if there are these kind of ARCHs except for ARM.

Quoted 7 lines · Ming Lei

Quoted 6 lines

I've been following this whole discussion. ?I didn't understand the idea behind the original patch, and I don't understand this. ?What reason is there for adding a memory barrier after the "dummy->hw_token = token" line? ?Such a barrier would not accomplish anything useful, because there are no later memory accesses which need to be ordered with respect to that line.

Quoted 2 lines

Here, mb is used to synchronize ?between writing of CPU and reading of ehci HC, see below:

A memory barrier never synchronizes events between two processors (for example, between the CPU and HC). ?It only affects ordering for the processor it executes on. ?That's why you always have to use memory barriers in pairs for inter-CPU synchronization: one memory barrier instruction for each CPU.

Quoted 8 lines

? ? ? CPU ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? EHCI
? ? ? dummy->hw_token = token;
? ? ? mb
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? read ?dummy->hw_token

The mb() introduced ?was supposed to be used to make sure that EHCI can see the correct value of ?dummy->hw_token.

It won't do that. ?All it will do is guarantee that the CPU writes out dumy->hw_token before it writes out or reads in any values executed after the mb.

Quoted 3 lines

?If EHCI can't get the correct
hw_token in time, it will work badly, such as irq delay or irq lost which may be caused by mistaken 'IOC' or 'total bytes to transfer' in hw_token.

No. ?All that will happen is that the HC will execute the qTD a little later, when it does read the correct hw_token. ?No IRQs will be lost, and there will be no mistaken IOC or "total bytes to transfer" values.

Quoted 3 lines

?This is just
what the patch is to fix.

The patch won't fix this. ?You're trying to force the CPU to write out dummy->hw_token more quickly. ?A memory barrier won't do that. ?And even if the CPU does write out the value more quickly, that doesn't guarantee it will be in time for the HC to see the new value right away.

Quoted 3 lines

But I think the above is still not correct, and the correct way may be like below:

Quoted 4 lines

? ? ? CPU ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? EHCI ? ? ? dummy->hw_token = token;
? ? ? wmb
? ? ? qh->hw->hw_qtd_next = QTD_NEXT(ehci, dummy->qtd_dma);

If you do this write, you will corrupt the hardware list. ?You _must_ _not_ modify the hardware fields of an active QH.

Besides, qh_link_async() calls qh_refresh() -> qh_update(), which does this already. ?And it does this correctly, with the appropriate checks to make sure the QH isn't active.

Quoted 8 lines

? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? fetch next qtd from qh->hw->hw_qtd_next ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? read ?qtd->hw_token

The problem is that qh has already linked dummy into its queue before(as did in qh_update), so even after 'dummy->hw_token = token;' is executed on CPU, EHCI still may not fetch the up-to-date value of the qtd(dummy in here), and this should
be the root cause, IMO.

There's no way to fix this. ?The CPU and the HC are independent. ?The HC is allowed to cache values whenever it likes (within certain limits). ?If it has already cached an old value from the qTD, there's no way to force it to use a new value. ?You just have to wait until it refreshes its cache.

IMO, the dummy has been linked into queue pointed by qh->hw->hw_qtd_next, so EHCI will fetch dummy qtd and execute the transaction and will not have any delay on the transaction.

Let me explain the problem again: On ARM, the wmb() before 'dummy->hw_token = token;'
will flush l2 write buffer into memory and all parts of 'dummy' except for hw_token field
have reached into memory already, but dummy->hw_token will stay at l2 write buffer
and not reach into memory at this time, so ehci may fetch a inconsistent qtd and execute it,
then mistaken IOC or "total bytes to transfer" are read by EHCI and cause delayed irq
or lost irq.

It is not only a reasoning or guess, and I have traced this kind of fact certainly.

Quoted 4 lines · Ming Lei

IMO, the dummy has been linked into queue pointed by qh->hw->hw_qtd_next, so EHCI will fetch dummy qtd and execute the transaction and will not have any delay on the transaction.

Quoted 11 lines

Let me explain the problem again: On ARM, the wmb() before 'dummy->hw_token = token;'
will flush l2 write buffer into memory and all parts of 'dummy' except for hw_token field
have reached into memory already, but dummy->hw_token will stay at l2 write buffer
and not reach into memory at this time, so ehci may fetch a inconsistent qtd and execute it,
then mistaken IOC or "total bytes to transfer" ?are read by EHCI and cause delayed irq
or lost irq.

No. ?Even if the HC reads dummy before dummy->hw_token has been written out to memory from the L2 cache, it will not see any inconsistencies. It will see the old value in hw_token, which has the ACTIVE bit clear. Therefore it will not try to execute the qTD but will move on to the next QH. ?See what the fourth paragraph in section 4.10.2 of the EHCI spec says about the case where a qTD's ACTIVE bit is set to 0.

Suppose HC can see the old value in hw_token, which has the ACTIVE bit clear.

The qtd transaction will not be executed until the new token is flushed into memory.
From view of CPU, the irq is still be delayed because ioc irq is generated after the qtd transaction is executed when new token is flushed into memory. The delay
depends on how long the token will be flushed into memory.

From my observation, the delay is commonly over 5ms for CSW, sometimes the delay is more than 20ms, so caused the bad performance.

Also I am not sure if EHCI can read the old hw_token correctly in this kind of inconsistent memory state.

Nevertheless, it remains true that you want to add a memory barrier instruction simply in order to speed up a cache writeback, not to force any sort of ordering. ?This needs to be explained carefully in the code (not just in the patch description!) and it needs to be done in a way that won't affect other architectures.

Also, as you mentioned before, you may want to do the same thing in qh_link_async() just after the

? ? ? ?head->hw->hw_next = dma;

line. ?Delays in flushing that write would also slow down performance.

From: Ming Lei<[email protected]>

This patch fixs one performance bug on ARM Cortex A9 dual core platform, which has been reported on quite a few ARM machines(OMAP4, Tegra 2, snowball...), see details from link of https://bugs.launchpad.net/bugs/709245.

In fact, one mb() on ARM is enough to flush L2 cache, but 'dummy->hw_token = token;' after mb() is added just for obeying correct mb() usage.

The patch has been tested ok on OMAP4 panda A1 board, the performance of 'dd' over usb mass storage can be increased from 4~5MB/sec to 14~16MB/sec after applying this patch.

Signed-off-by Ming Lei<[email protected]>

---
  drivers/usb/host/ehci-q.c |   14 ++++++++++++++
  1 files changed, 14 insertions(+), 0 deletions(-)

drivers/usb/host/ehci-q.c +0 -0

index 0917e3a..65b5021 100644
@@ -1082,6 +1082,20 @@ static struct ehci_qh *qh_append_tds (
  			wmb ();