Re: [PATCH] I/O space write barrier

G'day,

On Mon, Sep 27, 2004 at 11:03:39AM -0700, Jesse Barnes wrote:

On some platforms (e.g. SGI Challenge, Origin, and Altix machines), writes to 
I/O space aren't ordered coming from different CPUs.  For the most part, this 
isn't a problem since drivers generally spinlock around code that does writeX 
calls, but if the last operation a driver does before it releases a lock is a 
write and some other CPU takes the lock and immediately does a write, it's 
possible the second CPU's write could arrive before the first's.

This patch adds a mmiowb() call to deal with this sort of situation, and 
adds some documentation describing I/O ordering issues to deviceiobook.tmpl.  
The idea is to mirror the regular, cacheable memory barrier operation, wmb.  
[...]
Patches to use this new primitive in various drivers will come separately, 
probably via the SCSI tree.

Ok, here's a patch for the tg3 network driver to use mmiowb().  Tests
over the last couple of days has shown that it solves the oopses in
tg3_tx() that I reported and attempted to patch some time ago:

http://marc.theaimsgroup.com/?l=linux-netdev&m=108538612421774&w=2

The CPU usage of the mmiowb() approach is also significantly better
than doing PCI reads to flush the writes (by setting the existing
TG3_FLAG_MBOX_WRITE_REORDER flag).  In an artificial CPU-constrained
test on a ProPack kernel, the same amount of CPU work for the REORDER
solution pushes 85.1 MB/s over 2 NICs compared to 146.5 MB/s for the
mmiowb() solution.

--- linux.orig/drivers/net/tg3.c	2004-09-22 17:20:45.%N +1000
+++ linux/drivers/net/tg3.c	2004-09-29 19:45:16.%N +1000

@@ -44,6 +44,19 @@
 #include <asm/pbm.h>
 #endif
 
+#ifndef mmiowb
+/*
+ * mmiowb() is a memory-mapped I/O write boundary, useful for
+ * preserving send ring update ordering between multiple CPUs
+ * Define it if it doesn't exist.
+ */
+#ifdef CONFIG_IA64_SGI_SN2
+#define mmiowb()    sn_mmiob()
+#else
+#define mmiowb()
+#endif
+#endif
+
 #if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 #define TG3_VLAN_TAG_USED 1
 #else

@@ -2725,6 +2738,7 @@ next_pkt_nopost:
 		tw32_rx_mbox(MAILBOX_RCV_JUMBO_PROD_IDX + TG3_64BIT_REG_LOW,
 			     sw_idx);
 	}
+	mmiowb();
 
 	return received;
 }

@@ -3172,6 +3186,7 @@ static int tg3_start_xmit(struct sk_buff
 		netif_stop_queue(dev);
 
 out_unlock:
+    	mmiowb();
 	spin_unlock_irqrestore(&tp->tx_lock, flags);
 
 	dev->trans_start = jiffies;

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

On Wed, 29 Sep 2004 20:36:46 +1000
Greg Banks [off-list ref] wrote:

Ok, here's a patch for the tg3 network driver to use mmiowb().  Tests
over the last couple of days has shown that it solves the oopses in
tg3_tx() that I reported and attempted to patch some time ago:

http://marc.theaimsgroup.com/?l=linux-netdev&m=108538612421774&w=2

The CPU usage of the mmiowb() approach is also significantly better
than doing PCI reads to flush the writes (by setting the existing
TG3_FLAG_MBOX_WRITE_REORDER flag).  In an artificial CPU-constrained
test on a ProPack kernel, the same amount of CPU work for the REORDER
solution pushes 85.1 MB/s over 2 NICs compared to 146.5 MB/s for the
mmiowb() solution.

Please put this macro in asm/io.h or similar and make sure
every platform has it implemented or provides a NOP version.

A lot of people are going to get this wrong btw.  The only
way it's really going to be cured across the board is if someone
like yourself who understands this audits all of the drivers.

On Wednesday, September 29, 2004 1:35 pm, David S. Miller wrote:

On Wed, 29 Sep 2004 20:36:46 +1000

Greg Banks [off-list ref] wrote:

quoted

Ok, here's a patch for the tg3 network driver to use mmiowb().  Tests
over the last couple of days has shown that it solves the oopses in
tg3_tx() that I reported and attempted to patch some time ago:

http://marc.theaimsgroup.com/?l=linux-netdev&m=108538612421774&w=2

The CPU usage of the mmiowb() approach is also significantly better
than doing PCI reads to flush the writes (by setting the existing
TG3_FLAG_MBOX_WRITE_REORDER flag).  In an artificial CPU-constrained
test on a ProPack kernel, the same amount of CPU work for the REORDER
solution pushes 85.1 MB/s over 2 NICs compared to 146.5 MB/s for the
mmiowb() solution.

Please put this macro in asm/io.h or similar and make sure
every platform has it implemented or provides a NOP version.

The patch that actually implements mmiowb() already does this, I think Greg 
just used his patch for testing.  The proper way to do it of course is to 
just use mmiowb() where needed in tg3 after the write barrier patch gets in.

A lot of people are going to get this wrong btw.  The only
way it's really going to be cured across the board is if someone
like yourself who understands this audits all of the drivers.

Yep, just like PCI posting (though many people seem to have a grasp on that 
now).

Thanks,
Jesse

On Wed, 29 Sep 2004 13:43:55 -0700
Jesse Barnes [off-list ref] wrote:

The patch that actually implements mmiowb() already does this, I think Greg 
just used his patch for testing.  The proper way to do it of course is to 
just use mmiowb() where needed in tg3 after the write barrier patch gets in.

Perfect, please send me a tg3 patch once the mmiowb() bits
go into the tree.

Thanks a lot.

On Thu, 2004-09-30 at 06:50, David S. Miller wrote:

On Wed, 29 Sep 2004 13:43:55 -0700
Jesse Barnes [off-list ref] wrote:

quoted

The patch that actually implements mmiowb() already does this, I think Greg 
just used his patch for testing.  

Yes, that hunk will be unnecessary when Jesse's patch goes in.

The proper way to do it of course is to 

quoted

just use mmiowb() where needed in tg3 after the write barrier patch gets in.

Perfect, please send me a tg3 patch once the mmiowb() bits
go into the tree.

Will do.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.