[PATCH] use mmiowb in tg3.c

[PATCH] use mmiowb in tg3.c

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

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 <gnb@sgi.com>
Signed-off-by: Jesse Barnes <jbarnes@sgi.com

Thanks,
Jesse

[-- Attachment #2: tg3-mmiowb-2.patch --]
[-- Type: text/plain, Size: 459 bytes --]

===== drivers/net/tg3.c 1.210 vs edited =====
--- 1.210/drivers/net/tg3.c	2004-10-06 09:42:56 -07:00
+++ edited/drivers/net/tg3.c	2004-10-21 16:06:37 -07:00
@@ -2729,6 +2729,7 @@
 		tw32_rx_mbox(MAILBOX_RCV_JUMBO_PROD_IDX + TG3_64BIT_REG_LOW,
 			     sw_idx);
 	}
+	mmiowb();
 
 	return received;
 }
@@ -3176,6 +3177,7 @@
 		netif_stop_queue(dev);
 
 out_unlock:
+    	mmiowb();
 	spin_unlock_irqrestore(&tp->tx_lock, flags);
 
 	dev->trans_start = jiffies;

Re: [PATCH] use mmiowb in tg3.c

[David S. Miller]

On Thu, 21 Oct 2004 16:28:06 -0700
Jesse Barnes <jbarnes@engr.sgi.com> 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.

Re: [PATCH] use mmiowb in tg3.c

[Benjamin Herrenschmidt]

On Fri, 2004-10-22 at 09:40, David S. Miller wrote:
> On Thu, 21 Oct 2004 16:28:06 -0700
> Jesse Barnes <jbarnes@engr.sgi.com> 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.

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.

Re: [PATCH] use mmiowb in tg3.c

[akepner]

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

Re: [PATCH] use mmiowb in tg3.c

[Benjamin Herrenschmidt]

On Fri, 2004-10-22 at 11:33, akepner@sgi.com wrote:
> 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? 

Right.

Ben.

Re: [PATCH] use mmiowb in tg3.c

[Jesse Barnes]

On Thursday, October 21, 2004 6:40 pm, David S. Miller wrote:
> On Thu, 21 Oct 2004 16:28:06 -0700
>
> Jesse Barnes <jbarnes@engr.sgi.com> 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.

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

Re: [PATCH] use mmiowb in tg3.c

[Paul Mackerras]

Jesse Barnes writes:

> On Thursday, October 21, 2004 6:40 pm, David S. Miller wrote:
> > On Thu, 21 Oct 2004 16:28:06 -0700
> >
> > Jesse Barnes <jbarnes@engr.sgi.com> 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.
> 
> 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.

[PATCH] use mmiowb in tg3_poll

[akepner]

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 <akepner@sgi.com>
---

--- 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

Re: [PATCH] use mmiowb in tg3_poll

[Lennert Buytenhek]

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

Re: [PATCH] use mmiowb in tg3_poll

[Arthur Kepner]

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

Re: [PATCH] use mmiowb in tg3_poll

[jamal]

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

Re: [PATCH] use mmiowb in tg3_poll

[Jesse Barnes]

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