pcnet32: Remove pointless memory barriers

These two memory barriers in performance-critical paths are not needed on x86. Even if some other architecture does buffer PCI I/O space writes, the existing memory-mapped I/O barriers are unlikely to be what is needed.

---
 drivers/net/pcnet32.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index e5e8c59..1c35e1d 100644
@@ -1405,7 +1405,7 @@ static int pcnet32_poll(struct napi_struct *napi, int budget)
 
 		/* Set interrupt enable. */
 		lp->a.write_csr(ioaddr, CSR0, CSR0_INTEN);
-		mmiowb();
+
 		spin_unlock_irqrestore(&lp->lock, flags);
 	}
 	return work_done;

@@ -2597,7 +2597,7 @@ pcnet32_interrupt(int irq, void *dev_id)
 			val = lp->a.read_csr(ioaddr, CSR3);
 			val |= 0x5f00;
 			lp->a.write_csr(ioaddr, CSR3, val);
-			mmiowb();
+
 			__napi_schedule(&lp->napi);
 			break;
 		}

1.5.4.3

From: John Dykstra <[email protected]>
Date: Tue, 28 Apr 2009 19:07:39 +0000

Any driver where these things are present usually has them there for a reason. Usually it's because the SGI guys really did run into real problems without them on their huge machines which can reorder PCI MMIO wrt. real memory operations.

I don't feel good applying this at all, given that I see no evidence that there has been any investigation into how these barriers got there in the first place.

Is that relevant when the driver doesn't do any MMIO operations? All pcnet32 register accesses are via PCI I/O space, as implied by the commit description.

Descriptors and buffers are, of course, in consistent physical memory. This patch doesn't touch the barriers associated with those structures.

Before sending out this patch, I determined that the MMIO barriers were added as part of NAPI support. I CCed one of the authors of that patch, so he could NAK if appropriate. I've added the other author on this reply.

I knew there'd be questions about whether removing these two barriers was safe. Submitting the patch seemed the best way to understand why they were needed, if they are.

-- John

My original NAPI implementation did not have the mmiowb() but it was added because of some comments from Francois Romeiu (2006-06-29/30). I do not know if they are required or not. My feeling was/is that they are not as the writes are flushed with the unlocking primitives. However that is not based on knowledge, just "feeling".

How would this be tested on all architectures to find out?

Don

-----Original Message-----
From: John Dykstra <[email protected]>
To: David Miller <[email protected]>
Cc: [email protected], [email protected], [email protected]
Subject: Re: [PATCH net-next-2.6] pcnet32: Remove pointless memory barriers
Date: Wed, 29 Apr 2009 08:48:17 -0500

Is that relevant when the driver doesn't do any MMIO operations? All pcnet32 register accesses are via PCI I/O space, as implied by the commit description.

Descriptors and buffers are, of course, in consistent physical memory. This patch doesn't touch the barriers associated with those structures.

Before sending out this patch, I determined that the MMIO barriers were added as part of NAPI support. I CCed one of the authors of that patch, so he could NAK if appropriate. I've added the other author on this reply.

I knew there'd be questions about whether removing these two barriers was safe. Submitting the patch seemed the best way to understand why they were needed, if they are.

-- John

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Testing could never prove that the barriers _aren't_ needed, because you'd never be sure that you'd hit the right conditions.

-- John

Don Fry <[email protected]> :

(not sure if anyone should care about the loss of performance on x86 for a memory barrier near a couple of PCI I/O accesses but...)

I take back my comment from 2006 as I did not notice that there were PCI I/O space accesses only.

Ueimor

After going back to my original NAPI changes, email exchanges today with Francois and John, and looking through all the architecture implementation of mmiowb and locking; I agree that this mmiowb is unnecessary since a lock is always released which has all the memory barriers needed. I no longer have access to all the equipment I had available at IBM, but it was tested a extensively on Intel and Power platforms without the mmiowb.

-----Original Message-----
From: John Dykstra <[email protected]>
To: netdev <[email protected]>, Don Fry <[email protected]>
Subject: [PATCH net-next-2.6] pcnet32: Remove pointless memory barriers
Date: Tue, 28 Apr 2009 19:07:39 +0000

These two memory barriers in performance-critical paths are not needed on x86. Even if some other architecture does buffer PCI I/O space writes, the existing memory-mapped I/O barriers are unlikely to be what is needed.

---
 drivers/net/pcnet32.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index e5e8c59..1c35e1d 100644
@@ -1405,7 +1405,7 @@ static int pcnet32_poll(struct napi_struct *napi, int budget)
 
 		/* Set interrupt enable. */
 		lp->a.write_csr(ioaddr, CSR0, CSR0_INTEN);
-		mmiowb();
+
 		spin_unlock_irqrestore(&lp->lock, flags);
 	}
 	return work_done;

@@ -2597,7 +2597,7 @@ pcnet32_interrupt(int irq, void *dev_id)
 			val = lp->a.read_csr(ioaddr, CSR3);
 			val |= 0x5f00;
 			lp->a.write_csr(ioaddr, CSR3, val);
-			mmiowb();
+
 			__napi_schedule(&lp->napi);
 			break;
 		}

[Off-list]

Don, I didn't mean to create this additional work for you, but I very much appreciate the help.

-- John

From: John Dykstra <[email protected]>
Date: Wed, 29 Apr 2009 19:16:23 -0500

ROFL, actually, on-list :-)

From: Don Fry <[email protected]>
Date: Wed, 29 Apr 2009 17:09:48 -0700

Looks good to me too, I'll apply this to net-next-2.6

Thanks