[PATCH net-next-2.6] pcnet32: Remove pointless memory barriers

[PATCH net-next-2.6] pcnet32: Remove pointless memory barriers

[John Dykstra]

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.

Signed-off-by: John Dykstra <john.dykstra1@gmail.com>
---
 drivers/net/pcnet32.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcnet32.c b/drivers/net/pcnet32.c
index e5e8c59..1c35e1d 100644
--- a/drivers/net/pcnet32.c
+++ b/drivers/net/pcnet32.c
@@ -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

Re: [PATCH net-next-2.6] pcnet32: Remove pointless memory barriers

[David Miller]

From: John Dykstra <john.dykstra1@gmail.com>
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.
> 
> Signed-off-by: John Dykstra <john.dykstra1@gmail.com>

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.

Re: [PATCH net-next-2.6] pcnet32: Remove pointless memory barriers

[John Dykstra]

On Tue, 2009-04-28 at 22:16 -0700, David Miller wrote:
> 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.

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.

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

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

Re: [PATCH net-next-2.6] pcnet32: Remove pointless memory barriers

[Don Fry]

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 <john.dykstra1@gmail.com>
To: David Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org, pcnet32@verizon.net, jeff@garzik.org
Subject: Re: [PATCH net-next-2.6] pcnet32: Remove pointless memory
barriers
Date: Wed, 29 Apr 2009 08:48:17 -0500

On Tue, 2009-04-28 at 22:16 -0700, David Miller wrote:
> 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.

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.

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

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next-2.6] pcnet32: Remove pointless memory barriers

[John Dykstra]

On Wed, 2009-04-29 at 10:10 -0700, Don Fry wrote:
> How would this be tested on all architectures to find out?

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

  --  John

Re: [PATCH net-next-2.6] pcnet32: Remove pointless memory barriers

[Francois Romieu]

Don Fry <pcnet32@verizon.net> :
> 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".

(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

Re: [PATCH net-next-2.6] pcnet32: Remove pointless memory barriers

[Don Fry]

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.

Acked-by:  Don Fry <pcnet32@verizon.net>

-----Original Message-----
From: John Dykstra <john.dykstra1@gmail.com>
To: netdev <netdev@vger.kernel.org>, Don Fry <pcnet32@verizon.net>
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.

Signed-off-by: John Dykstra <john.dykstra1@gmail.com>
---
 drivers/net/pcnet32.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcnet32.c b/drivers/net/pcnet32.c
index e5e8c59..1c35e1d 100644
--- a/drivers/net/pcnet32.c
+++ b/drivers/net/pcnet32.c
@@ -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;
 		}

Re: [PATCH net-next-2.6] pcnet32: Remove pointless memory barriers

[John Dykstra]

On Wed, 2009-04-29 at 17:09 -0700, Don Fry wrote:
> 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.

[Off-list]

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

  --  John

Re: [PATCH net-next-2.6] pcnet32: Remove pointless memory barriers

[David Miller]

From: Don Fry <pcnet32@verizon.net>
Date: Wed, 29 Apr 2009 17:09:48 -0700

> 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.
> 
> Acked-by:  Don Fry <pcnet32@verizon.net>

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

Thanks

Re: [PATCH net-next-2.6] pcnet32: Remove pointless memory barriers

[David Miller]

From: John Dykstra <john.dykstra1@gmail.com>
Date: Wed, 29 Apr 2009 19:16:23 -0500

> On Wed, 2009-04-29 at 17:09 -0700, Don Fry wrote:
>> 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.
> 
> [Off-list]

ROFL, actually, on-list :-)