[PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Scott Wood]

The hardware must not see that is given ownership of a buffer until it is
completely written, and when the driver receives ownership of a buffer,
it must ensure that any other reads to the buffer reflect its final
state.  Thus, I/O barriers are added where required.

Without this patch, I have observed GCC reordering the setting of
bdp->length and bdp->status in gfar_new_skb.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
This is version 2 of this patch.  I was told that eieio doesn't order
loads from main memory, so a sync is used instead.  Also, due to
objectons that iobarrier_* shouldn't be used for main memory, I used
eieio() instead (a wmb() would be unnecessarily heavy).

 drivers/net/gianfar.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index b666a0c..b014d27 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1025,6 +1025,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	dev->trans_start = jiffies;
 
+	eieio();
 	txbdp->status = status;
 
 	/* If this was the last BD in the ring, the next one */
@@ -1301,6 +1302,7 @@ struct sk_buff * gfar_new_skb(struct net_device *dev, struct rxbd8 *bdp)
 	bdp->length = 0;
 
 	/* Mark the buffer empty */
+	eieio();
 	bdp->status |= (RXBD_EMPTY | RXBD_INTERRUPT);
 
 	return skb;
@@ -1484,6 +1486,7 @@ int gfar_clean_rx_ring(struct net_device *dev, int rx_work_limit)
 	bdp = priv->cur_rx;
 
 	while (!((bdp->status & RXBD_EMPTY) || (--rx_work_limit < 0))) {
+		rmb();
 		skb = priv->rx_skbuff[priv->skb_currx];
 
 		if (!(bdp->status &
-- 
1.5.0.3

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Kumar Gala]

On May 2, 2007, at 2:57 PM, Scott Wood wrote:

> The hardware must not see that is given ownership of a buffer until  
> it is
> completely written, and when the driver receives ownership of a  
> buffer,
> it must ensure that any other reads to the buffer reflect its final
> state.  Thus, I/O barriers are added where required.
>
> Without this patch, I have observed GCC reordering the setting of
> bdp->length and bdp->status in gfar_new_skb.
>
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> This is version 2 of this patch.  I was told that eieio doesn't order
> loads from main memory, so a sync is used instead.  Also, due to
> objectons that iobarrier_* shouldn't be used for main memory, I used
> eieio() instead (a wmb() would be unnecessarily heavy).

I'd rather see a wmb() instead of eieio() to keep this code non-ppc  
specific.  (also, we implement wmb as eieio, so I don't keep the  
comment about it being too heavy, unless you mean generically).

- k

>
>  drivers/net/gianfar.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> index b666a0c..b014d27 100644
> --- a/drivers/net/gianfar.c
> +++ b/drivers/net/gianfar.c
> @@ -1025,6 +1025,7 @@ static int gfar_start_xmit(struct sk_buff  
> *skb, struct net_device *dev)
>
>  	dev->trans_start = jiffies;
>
> +	eieio();
>  	txbdp->status = status;
>
>  	/* If this was the last BD in the ring, the next one */
> @@ -1301,6 +1302,7 @@ struct sk_buff * gfar_new_skb(struct  
> net_device *dev, struct rxbd8 *bdp)
>  	bdp->length = 0;
>
>  	/* Mark the buffer empty */
> +	eieio();
>  	bdp->status |= (RXBD_EMPTY | RXBD_INTERRUPT);
>
>  	return skb;
> @@ -1484,6 +1486,7 @@ int gfar_clean_rx_ring(struct net_device  
> *dev, int rx_work_limit)
>  	bdp = priv->cur_rx;
>
>  	while (!((bdp->status & RXBD_EMPTY) || (--rx_work_limit < 0))) {
> +		rmb();
>  		skb = priv->rx_skbuff[priv->skb_currx];
>
>  		if (!(bdp->status &
> -- 
> 1.5.0.3
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Scott Wood]

Kumar Gala wrote:
> I'd rather see a wmb() instead of eieio() to keep this code non-ppc  
> specific.  (also, we implement wmb as eieio, so I don't keep the  
> comment about it being too heavy, unless you mean generically).

wmb() is a sync, smp_wmb() is an eieio.  Andy told me he would not 
accept a sync in those spots.

And the driver is already ppc-specific; it uses in/out_be32.

-Scott

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Kumar Gala]

On May 2, 2007, at 3:12 PM, Scott Wood wrote:

> Kumar Gala wrote:
>> I'd rather see a wmb() instead of eieio() to keep this code non- 
>> ppc  specific.  (also, we implement wmb as eieio, so I don't keep  
>> the  comment about it being too heavy, unless you mean generically).
>
> wmb() is a sync, smp_wmb() is an eieio.  Andy told me he would not  
> accept a sync in those spots.

Sorry, was looking at the iobarrier code.

> And the driver is already ppc-specific; it uses in/out_be32.

True, but its hidden behind the gfar_read/write accessors.

Your change is a bit more blatant.

- k

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Scott Wood]

Kumar Gala wrote:
> On May 2, 2007, at 3:12 PM, Scott Wood wrote:
>> wmb() is a sync, smp_wmb() is an eieio.  Andy told me he would not  
>> accept a sync in those spots.
> 
> 
> Sorry, was looking at the iobarrier code.
> 
>> And the driver is already ppc-specific; it uses in/out_be32.
> 
> 
> True, but its hidden behind the gfar_read/write accessors.
> 
> Your change is a bit more blatant.

Well, Segher doesn't want me to use iobarrier (because it's not I/O). 
Andy doesn't want me to use wmb() (because it's sync).  I don't think 
something like gfar_wmb() would be appropriate.  So the remaining 
options are either eieio(), or a new non-arch-specific, 
non-driver-specific mem_wmb() (or whatever).

While I like the latter option, I don't think this bugfix should have to 
wait for it.

-Scott

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Kumar Gala]

On May 2, 2007, at 3:40 PM, Scott Wood wrote:

> Kumar Gala wrote:
>> On May 2, 2007, at 3:12 PM, Scott Wood wrote:
>>> wmb() is a sync, smp_wmb() is an eieio.  Andy told me he would  
>>> not  accept a sync in those spots.
>> Sorry, was looking at the iobarrier code.
>>> And the driver is already ppc-specific; it uses in/out_be32.
>> True, but its hidden behind the gfar_read/write accessors.
>> Your change is a bit more blatant.
>
> Well, Segher doesn't want me to use iobarrier (because it's not I/ 
> O). Andy doesn't want me to use wmb() (because it's sync).  I don't  
> think something like gfar_wmb() would be appropriate.  So the  
> remaining options are either eieio(), or a new non-arch-specific,  
> non-driver-specific mem_wmb() (or whatever).
>
> While I like the latter option, I don't think this bugfix should  
> have to wait for it.

Ok, I've resigned to the eieio's.  If we end up respinning the patch  
again for any reason I'd like to see something in the commit comment  
to the fact that we are adding ppc specific sync operations.

Why doesn't marking the bdp pointer volatile resolve the issue in  
gfar_clean_rx_ring() to ensure load ordering?

- k

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Scott Wood]

Kumar Gala wrote:
> Why doesn't marking the bdp pointer volatile resolve the issue in  
> gfar_clean_rx_ring() to ensure load ordering?

Because that only addresses compiler reordering (and does so in a rather 
clumsy way -- not all accesses need to be strongly ordered), not 
hardware reordering.

-Scott

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Kumar Gala]

On Wed, 2 May 2007, Scott Wood wrote:

> Kumar Gala wrote:
> > Why doesn't marking the bdp pointer volatile resolve the issue in
> > gfar_clean_rx_ring() to ensure load ordering?
>
> Because that only addresses compiler reordering (and does so in a rather
> clumsy way -- not all accesses need to be strongly ordered), not hardware
> reordering.
>
> -Scott
>

So what about some thing like this where we do the read only once?

- k

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index a06d8d1..9cd7d1e 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1438,31 +1438,35 @@ int gfar_clean_rx_ring(struct net_device *dev, int rx_work_limit)
 {
 	struct rxbd8 *bdp;
 	struct sk_buff *skb;
-	u16 pkt_len;
+	u16 pkt_len, status;
+	u32 bd_info;
 	int howmany = 0;
 	struct gfar_private *priv = netdev_priv(dev);

 	/* Get the first full descriptor */
 	bdp = priv->cur_rx;

-	while (!((bdp->status & RXBD_EMPTY) || (--rx_work_limit < 0))) {
+	bd_info = *bdp;
+
+	status = bd_info >> 16;
+	/* Remove the FCS from the packet length */
+	pkt_len = (bd_info & 0xffff) - 4;
+
+	while (!((status & RXBD_EMPTY) || (--rx_work_limit < 0))) {
 		skb = priv->rx_skbuff[priv->skb_currx];

-		if (!(bdp->status &
+		if (!(status &
 		      (RXBD_LARGE | RXBD_SHORT | RXBD_NONOCTET
 		       | RXBD_CRCERR | RXBD_OVERRUN | RXBD_TRUNCATED))) {
 			/* Increment the number of packets */
 			priv->stats.rx_packets++;
 			howmany++;

-			/* Remove the FCS from the packet length */
-			pkt_len = bdp->length - 4;
-
 			gfar_process_frame(dev, skb, pkt_len);

 			priv->stats.rx_bytes += pkt_len;
 		} else {
-			count_errors(bdp->status, priv);
+			count_errors(status, priv);

 			if (skb)
 				dev_kfree_skb_any(skb);
@@ -1480,7 +1484,7 @@ int gfar_clean_rx_ring(struct net_device *dev, int rx_work_limit)
 		priv->rx_skbuff[priv->skb_currx] = skb;

 		/* Update to the next pointer */
-		if (bdp->status & RXBD_WRAP)
+		if (status & RXBD_WRAP)
 			bdp = priv->rx_bd_base;
 		else
 			bdp++;
@@ -1490,6 +1494,11 @@ int gfar_clean_rx_ring(struct net_device *dev, int rx_work_limit)
 		    (priv->skb_currx +
 		     1) & RX_RING_MOD_MASK(priv->rx_ring_size);

+		bd_info = *bdp;
+
+		status = bd_info >> 16;
+		/* Remove the FCS from the packet length */
+		pkt_len = (bd_info & 0xffff) - 4;
 	}

 	/* Update the current rxbd pointer to be the next one */

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Scott Wood]

Kumar Gala wrote:
> So what about some thing like this where we do the read only once?
> 
> - k
> 
> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> index a06d8d1..9cd7d1e 100644
> --- a/drivers/net/gianfar.c
> +++ b/drivers/net/gianfar.c
> @@ -1438,31 +1438,35 @@ int gfar_clean_rx_ring(struct net_device *dev, int rx_work_limit)
>  {
>  	struct rxbd8 *bdp;
>  	struct sk_buff *skb;
> -	u16 pkt_len;
> +	u16 pkt_len, status;
> +	u32 bd_info;


I suggested that on IRC yesterday, and Segher was concerned that the 
compiler might, in theory, "optimize" it into to two lhz instructions. 
I'm rather skeptical that it would actually do so (even if it needs to 
load twice due to register pressure, why not just use lwz both times?), 
and there's probably many other places that would break if it did, but I 
wasn't up for digging around GCC to prove otherwise.

Plus, that wouldn't synchronize the bd_info read with the buffer data reads.

-Scott

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Segher Boessenkool]

>> So what about some thing like this where we do the read only once?
>>
>> - k
>>
>> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
>> index a06d8d1..9cd7d1e 100644
>> --- a/drivers/net/gianfar.c
>> +++ b/drivers/net/gianfar.c
>> @@ -1438,31 +1438,35 @@ int gfar_clean_rx_ring(struct net_device 
>> *dev, int rx_work_limit)
>>  {
>>  	struct rxbd8 *bdp;
>>  	struct sk_buff *skb;
>> -	u16 pkt_len;
>> +	u16 pkt_len, status;
>> +	u32 bd_info;
>
>
> I suggested that on IRC yesterday, and Segher was concerned that the
> compiler might, in theory, "optimize" it into to two lhz instructions.

Yes.  The same is true of the original code btw, but since
you test only one bit there, all is fine.

> I'm rather skeptical that it would actually do so (even if it needs to
> load twice due to register pressure, why not just use lwz both times?),

Sure.  That doesn't make this code correct though.

> and there's probably many other places that would break if it did,

Most other network drivers read from an MMIO reg to see
which RX ring entries are kernel owned AFAICS.

> but I wasn't up for digging around GCC to prove otherwise.

It doesn't matter what current GCC does -- simply look
at what it is *allowed* to do instead.

If you want a 32-bit read to be atomic, you should
do the read via a (volatile u32 *).  Doing this with
a cast in the places where you need the atomic access
makes sure you don't get unnecessary rereads.

> Plus, that wouldn't synchronize the bd_info read with the buffer data 
> reads.

Yes, you still need the rmb().


Segher

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Segher Boessenkool]

>>> And the driver is already ppc-specific; it uses in/out_be32.
>>
>> True, but its hidden behind the gfar_read/write accessors.
>>
>> Your change is a bit more blatant.
>
> Well, Segher doesn't want me to use iobarrier (because it's not I/O).
> Andy doesn't want me to use wmb() (because it's sync).

You should use wmb(), but unfortunately too strong
semantics are required for that (ordering wrt I/O)
so it's a full sync on PowerPC.  I don't believe
a priori that that would be notably slower, but if
actually is, you could use eieio() I suppose since
you say the driver is powerpc specific -- but please
put a comment in the source code then saying why you
don't use wmb() there.


Segher

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Linas Vepstas]

On Wed, May 02, 2007 at 03:40:20PM -0500, Scott Wood wrote:
> 
> Well, Segher doesn't want me to use iobarrier (because it's not I/O). 
> Andy doesn't want me to use wmb() (because it's sync).  I don't think 
> something like gfar_wmb() would be appropriate.  So the remaining 
> options are either eieio(), 

? Just curious... the original intent of eieio was to order I/O, 
such as MMIO; it has no effect on memory that isn't marked 
cache-inhibited or write-trhough or guarded. Has this changed?
I guess I haven't kept up with the times ... is eieio now
being used to provide some other kind of barrier?
Is eieio providing some sort of SMP synchronization side-effect?

Point being: if Segher doesn't let you "use iobarrier (because 
it's not I/O)", then I don't understand why eieio would work (since
that's for io only).  

--linas

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Olof Johansson]

On Fri, May 04, 2007 at 05:13:09PM -0500, Linas Vepstas wrote:
> On Wed, May 02, 2007 at 03:40:20PM -0500, Scott Wood wrote:
> > 
> > Well, Segher doesn't want me to use iobarrier (because it's not I/O). 
> > Andy doesn't want me to use wmb() (because it's sync).  I don't think 
> > something like gfar_wmb() would be appropriate.  So the remaining 
> > options are either eieio(), 
> 
> ? Just curious... the original intent of eieio was to order I/O, 
> such as MMIO; it has no effect on memory that isn't marked 
> cache-inhibited or write-trhough or guarded. Has this changed?
> I guess I haven't kept up with the times ... is eieio now
> being used to provide some other kind of barrier?
> Is eieio providing some sort of SMP synchronization side-effect?
> 
> Point being: if Segher doesn't let you "use iobarrier (because 
> it's not I/O)", then I don't understand why eieio would work (since
> that's for io only).  

Eieio has always worked for regular cachable memory as well, it just never
orders _between_ cache inhibited/guarded and cachable memory.

Book II 2.03 has a pretty good description of it on page 367.



-Olof

Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

[Segher Boessenkool]

>> Well, Segher doesn't want me to use iobarrier (because it's not I/O).
>> Andy doesn't want me to use wmb() (because it's sync).  I don't think
>> something like gfar_wmb() would be appropriate.  So the remaining
>> options are either eieio(),
>
> ? Just curious... the original intent of eieio was to order I/O,
> such as MMIO; it has no effect on memory that isn't marked
> cache-inhibited or write-trhough or guarded. Has this changed?

eieio orders all accesses to address space that is WIMG=1xxx
or WIMG=x1x1; separately, it orders stores to address space
that is WIMG=001x.

> I guess I haven't kept up with the times ... is eieio now
> being used to provide some other kind of barrier?

Nothing changed.

> Is eieio providing some sort of SMP synchronization side-effect?

It orders stores to "well-behaved" memory yes.

> Point being: if Segher doesn't let you "use iobarrier (because
> it's not I/O)", then I don't understand why eieio would work (since
> that's for io only).

iobarrier() is a kernel-level primitive, meant for ordering
I/O only, as its name indicates.  eieio is a CPU insn that
orders stores to main memory (amongst other things), not that
its name would tell you.


Segher