[PATCH] use mmiowb in tg3.c

This patch originally from Greg Banks.  Some parts of the tg3 driver depend on 
PIO writes arriving in order.  This patch ensures that in two key places 
using the new mmiowb macro.  This not only prevents bugs (the queues can be 
corrupted), but is much faster than ensuring ordering using PIO reads (which 
involve a few round trips to the target bus on some platforms).

Arthur has another patch that uses mmiowb in tg3 that he posted earlier as 
well.

Signed-off-by: Greg Banks <redacted>
Signed-off-by: Jesse Barnes <jbarnes@sgi.com

Thanks,
Jesse

On Thu, 21 Oct 2004 16:28:06 -0700
Jesse Barnes [off-list ref] wrote:

This patch originally from Greg Banks.  Some parts of the tg3 driver depend on 
PIO writes arriving in order.  This patch ensures that in two key places 
using the new mmiowb macro.  This not only prevents bugs (the queues can be 
corrupted), but is much faster than ensuring ordering using PIO reads (which 
involve a few round trips to the target bus on some platforms).

Do other PCI systems which post PIO writes also potentially reorder
them just like this SGI system does?  Just trying to get this situation
straight in my head.

On Fri, 2004-10-22 at 09:40, David S. Miller wrote:

On Thu, 21 Oct 2004 16:28:06 -0700
Jesse Barnes [off-list ref] wrote:

quoted

This patch originally from Greg Banks.  Some parts of the tg3 driver depend on 
PIO writes arriving in order.  This patch ensures that in two key places 
using the new mmiowb macro.  This not only prevents bugs (the queues can be 
corrupted), but is much faster than ensuring ordering using PIO reads (which 
involve a few round trips to the target bus on some platforms).

Do other PCI systems which post PIO writes also potentially reorder
them just like this SGI system does?  Just trying to get this situation
straight in my head.

I think the problem they have is really related to their spinlock, that
is the IO leaking out of the spinlock vs. other CPUs.

On the other hand, as I discussed with Jesse in the past, I'd like to
extend the semantics of mmiowb() to also define full barrier between
cacheable and non-cacheable accesses. That would help fixing a lot of issues
on ppc and ppc64.

Typically, our normal "light" write barrier doesn't reorder between cacheable
and non-cacheable (MMIO) stores, which is why we had to put some heavy sync
barrier in our MMIO writes macros.

By having an mmiowb() that allow to explicitely do this ordering, it would
allow us to relax the barriers in the MMIO macros, and so get back a few
percent of perfs that we lost with the "fix".

I haven't had time to work on that yet though, I need to review with paulus
all the possible race cases, but hopefully, I'll have a patch on top of
Jesse next week or so and will start converting more drivers.

Ben.

On Fri, 22 Oct 2004, Benjamin Herrenschmidt wrote:

... 
Typically, our normal "light" write barrier doesn't reorder between cacheable
and non-cacheable (MMIO) stores, which is why we had to put some heavy sync
barrier in our MMIO writes macros.
...

Do you mean "impose order" rather than "reorder" here? 

-- 
Arthur

On Fri, 2004-10-22 at 11:33, akepner@sgi.com wrote:

On Fri, 22 Oct 2004, Benjamin Herrenschmidt wrote:

quoted

... 
Typically, our normal "light" write barrier doesn't reorder between cacheable
and non-cacheable (MMIO) stores, which is why we had to put some heavy sync
barrier in our MMIO writes macros.
...

Do you mean "impose order" rather than "reorder" here? 

Right.

Ben.

On Thursday, October 21, 2004 6:40 pm, David S. Miller wrote:

On Thu, 21 Oct 2004 16:28:06 -0700

Jesse Barnes [off-list ref] wrote:

quoted

This patch originally from Greg Banks.  Some parts of the tg3 driver
depend on PIO writes arriving in order.  This patch ensures that in two
key places using the new mmiowb macro.  This not only prevents bugs (the
queues can be corrupted), but is much faster than ensuring ordering using
PIO reads (which involve a few round trips to the target bus on some
platforms).

Do other PCI systems which post PIO writes also potentially reorder
them just like this SGI system does?  Just trying to get this situation
straight in my head.

The HP guys claim that theirs don't, but PPC does, afaik.  And clearly any 
large system that posts PCI writes has the *potential* of reordering them.

Thanks,
Jesse

Jesse Barnes writes:

On Thursday, October 21, 2004 6:40 pm, David S. Miller wrote:

quoted

On Thu, 21 Oct 2004 16:28:06 -0700

Jesse Barnes [off-list ref] wrote:

quoted

This patch originally from Greg Banks.  Some parts of the tg3 driver
depend on PIO writes arriving in order.  This patch ensures that in two
key places using the new mmiowb macro.  This not only prevents bugs (the
queues can be corrupted), but is much faster than ensuring ordering using
PIO reads (which involve a few round trips to the target bus on some
platforms).

Do other PCI systems which post PIO writes also potentially reorder
them just like this SGI system does?  Just trying to get this situation
straight in my head.

The HP guys claim that theirs don't, but PPC does, afaik.  And clearly any 
large system that posts PCI writes has the *potential* of reordering them.

No, PPC systems don't reorder writes to PCI devices.  Provided you use
inl/outl/readl/writel et al., all PCI accesses from one processor are
strictly ordered, and if you use a spinlock, that gives you strict
access ordering between processors.

Our barrier instructions mostly order cacheable accesses separately
from non-cacheable accesses, except for the strongest barrier
instruction, which orders everything.  Thus it would be useful for us
to have an explicit indication of when a cacheable write (i.e. to main
memory) has to be completed (from a PCI device's point of view) before
a non-cacheable device read or write (e.g. to kick off DMA).

Paul.

Returning from tg3_poll() without flushing the PIO write which
reenables interrupts can result in lower cpu utilization and higher
throughput. So use a memory barrier, mmiowb(), instead of flushing the
write with a PIO read.

Signed-off-by: Arthur Kepner <redacted>
---

--- linux-2.6.9-rc4-mm1.orig/drivers/net/tg3.c	2004-10-22 13:51:10.000000000 -0700
+++ linux-2.6.9-rc4-mm1/drivers/net/tg3.c	2004-10-22 13:53:36.000000000 -0700

@@ -417,6 +417,20 @@
 	tg3_cond_int(tp);
 }
 
+/* tg3_restart_ints
+ *  similar to tg3_enable_ints, but it can return without flushing the 
+ *  PIO write which reenables interrupts
+ */
+static void tg3_restart_ints(struct tg3 *tp)
+{
+	tw32(TG3PCI_MISC_HOST_CTRL,
+		(tp->misc_host_ctrl & ~MISC_HOST_CTRL_MASK_PCI_INT));
+	tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW, 0x00000000);
+	mmiowb();
+
+	tg3_cond_int(tp);
+}
+
 static inline void tg3_netif_stop(struct tg3 *tp)
 {
 	netif_poll_disable(tp->dev);

@@ -2787,7 +2801,7 @@
 	if (done) {
 		spin_lock_irqsave(&tp->lock, flags);
 		__netif_rx_complete(netdev);
-		tg3_enable_ints(tp);
+		tg3_restart_ints(tp);
 		spin_unlock_irqrestore(&tp->lock, flags);
 	}
 

-- 
Arthur

On Fri, Oct 22, 2004 at 01:51:01PM -0700, akepner@sgi.com wrote:

Returning from tg3_poll() without flushing the PIO write which
reenables interrupts can result in lower cpu utilization and higher
throughput.

I'm quite curious what kind of MMIO read latency you see on your
Altix boxen.  This app is quite useful for determining those figures
on x86{,_64} machines:

	http://svn.gnumonks.org/trunk/mmio_test/mmio_test.c

I'm not sure if it'll run on Itanium out of the box (it currently
assumes PAGE_SIZE <= 4096, you'd probably need to tweak rdtscl(),
and if there's a weird phys:pci address correspondence you might
have to teach it about that as well), but it would be nice if you
could give an approximate indication of exactly how expensive an
MMIO read is on your platform.


cheers,
Lennert

On Sun, 29 May 2005, Lennert Buytenhek wrote:

.....
I'm quite curious what kind of MMIO read latency you see on your
Altix boxen.  This app is quite useful for determining those figures
on x86{,_64} machines:

	http://svn.gnumonks.org/trunk/mmio_test/mmio_test.c
....

[cc list change: jbarnes@engr.sgi.com -> jesse.barnes@intel.com]

Haven't tried this program yet (though it looks interesting.) 
In the past I've instrumented the driver with get_cycles() to 
acquire the PIO read and mmiowb() latencies.

I quickly looked through the records that I have and, unfortunately, 
wasn't able to locate raw data. But I found some scrawlings in my 
notes which say the most recent measurements for an Altix are: 

	i)  PIO read latency ~ 2.4 usec
	ii) mmiowb() latency ~ 1.1 usec

(This data was taken when the PIOs/mmiowb()s were coming from a CPU 
which was "nearest" to the NIC.)

If you have data for x86{,_64}, I'd be interested in that, too.

--
Arthur

On Mon, 2005-30-05 at 09:30 -0700, Arthur Kepner wrote:

I quickly looked through the records that I have and, unfortunately, 
wasn't able to locate raw data. But I found some scrawlings in my 
notes which say the most recent measurements for an Altix are: 

	i)  PIO read latency ~ 2.4 usec
	ii) mmiowb() latency ~ 1.1 usec

In terms of CPU cycles, how many do the above compute to be?
The absolute time numbers you have are also useful for quantification
purposes.

Did i read correctly that your reads are about 2x more expensive than
the writes?

cheers,
jamal

On Tuesday, May 31, 2005 5:53 am, jamal wrote:

Did i read correctly that your reads are about 2x more expensive than
the writes?

The test above just measures the difference between using a full PIO 
read to ensure write ordering vs. the lighter weight mmiowb call.  As 
for your question though, non-relaxed reads are very expensive on the 
Altix platform due to its highly distributed NUMA I/O architecture.  
The _relaxed variants can be quite fast however, and can be used 
anytime a PIO read doesn't imply anything about previous DMA 
transactions.

Jesse