Patchwork [net-next.git,2/7] stmmac: review barriers

login
register
mail settings
Submitter Giuseppe CAVALLARO
Date April 3, 2013, 5:41 a.m.
Message ID <1364967689-11155-2-git-send-email-peppe.cavallaro@st.com>
Download mbox | patch
Permalink /patch/233255/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Giuseppe CAVALLARO - April 3, 2013, 5:41 a.m.
In all my tests performed on SH4 and ARM A9 platforms, I've never met problems
that can be fixed by using memory barriers. In the past there was some issues
on SMP ARM but fixed by reviewing xmit spinlock.

Further barriers have been added in the commits too: 8e83989106562326bf

This patch is to use the smp_wbm instead of wbm because the driver
runs on UP systems. Then, IMO it could make sense to only maintain the barriers
just in places where we touch the dma owner bits (that is the
only real critical path as we had seen and fixed in the commit:
eb0dc4bb2e22c04964d).

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Deepak Sikri <deepak.sikri@st.com>
Cc: Shiraz Hashim <shiraz.hashim@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
Eric Dumazet - April 3, 2013, 7:18 a.m.
On Wed, 2013-04-03 at 07:41 +0200, Giuseppe CAVALLARO wrote:
> In all my tests performed on SH4 and ARM A9 platforms, I've never met problems
> that can be fixed by using memory barriers. In the past there was some issues
> on SMP ARM but fixed by reviewing xmit spinlock.
> 
> Further barriers have been added in the commits too: 8e83989106562326bf
> 
> This patch is to use the smp_wbm instead of wbm because the driver
> runs on UP systems. Then, IMO it could make sense to only maintain the barriers
> just in places where we touch the dma owner bits (that is the
> only real critical path as we had seen and fixed in the commit:
> eb0dc4bb2e22c04964d).
> 
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Deepak Sikri <deepak.sikri@st.com>
> Cc: Shiraz Hashim <shiraz.hashim@st.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8b69e3b..c92dcbc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1797,15 +1797,13 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  		priv->tx_skbuff[entry] = NULL;
>  		priv->hw->desc->prepare_tx_desc(desc, 0, len, csum_insertion,
>  						priv->mode);
> -		wmb();
> +		smp_wmb();
>  		priv->hw->desc->set_tx_owner(desc);

This looks pretty bogus to me.

If arch is UP, smp_wmb() is empty.

So why are you using it ?

Barrier here is not to protect the interaction with another cpu, but the
NIC itself.

If there is no need for barrier as you claim in changelog, don't add a
fake smp_wmb().



--
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
Giuseppe CAVALLARO - April 3, 2013, 7:28 a.m.
On 4/3/2013 9:18 AM, Eric Dumazet wrote:
> On Wed, 2013-04-03 at 07:41 +0200, Giuseppe CAVALLARO wrote:
>> In all my tests performed on SH4 and ARM A9 platforms, I've never met problems
>> that can be fixed by using memory barriers. In the past there was some issues
>> on SMP ARM but fixed by reviewing xmit spinlock.
>>
>> Further barriers have been added in the commits too: 8e83989106562326bf
>>
>> This patch is to use the smp_wbm instead of wbm because the driver
>> runs on UP systems. Then, IMO it could make sense to only maintain the barriers
>> just in places where we touch the dma owner bits (that is the
>> only real critical path as we had seen and fixed in the commit:
>> eb0dc4bb2e22c04964d).
>>
>> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Cc: Deepak Sikri <deepak.sikri@st.com>
>> Cc: Shiraz Hashim <shiraz.hashim@st.com>
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    9 +++------
>>   1 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 8b69e3b..c92dcbc 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -1797,15 +1797,13 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>   		priv->tx_skbuff[entry] = NULL;
>>   		priv->hw->desc->prepare_tx_desc(desc, 0, len, csum_insertion,
>>   						priv->mode);
>> -		wmb();
>> +		smp_wmb();
>>   		priv->hw->desc->set_tx_owner(desc);
>
> This looks pretty bogus to me.
>
> If arch is UP, smp_wmb() is empty.
>
> So why are you using it ?
>
> Barrier here is not to protect the interaction with another cpu, but the
> NIC itself.
>
> If there is no need for barrier as you claim in changelog, don't add a
> fake smp_wmb().
>

ok I'll do it because I do not need any barrier when test on my UP and
SMP. Barriers were added for SPEAr but I have never seen problems
although I have not done too many tests on these platforms. I usually
run on other ST ARM box but, as rule of thumb, similar to some SPEAr
for CPU and MAC.
At any rate, if somebody aims to have them we will discuss in this
mailing list to understand why/where these have to be placed in the code

peppe
--
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
Shiraz Hashim - April 3, 2013, 8:51 a.m.
Hi Giuseppe,

On Wed, Apr 03, 2013 at 01:41:24PM +0800, Giuseppe CAVALLARO wrote:
> In all my tests performed on SH4 and ARM A9 platforms, I've never met problems
> that can be fixed by using memory barriers. In the past there was some issues
> on SMP ARM but fixed by reviewing xmit spinlock.

The problem which was addressed was not because of SMP IMO. It was
rather due to the fact that the write to the GMAC descriptor (which
happens to be in normal memory) has to be ordered with respect to GMAC
DMA as observer. Isn't it ?

> Further barriers have been added in the commits too: 8e83989106562326bf
> 
> This patch is to use the smp_wbm instead of wbm because the driver
                           ^^^^^^^^^^^^^^^^^^^^^^
Perhaps you meant smp_wmb and wmb

> runs on UP systems. Then, IMO it could make sense to only maintain the barriers
> just in places where we touch the dma owner bits (that is the
> only real critical path as we had seen and fixed in the commit:
> eb0dc4bb2e22c04964d).

Replacing wmb by smp_wmb may not be a good idea as we need to order
the store transaction to the descriptor with respect to GMAC DMA and
using smp_* version would just be compiler barrier in uniprocessor
systems.

> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Deepak Sikri <deepak.sikri@st.com>
> Cc: Shiraz Hashim <shiraz.hashim@st.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8b69e3b..c92dcbc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1797,15 +1797,13 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  		priv->tx_skbuff[entry] = NULL;
>  		priv->hw->desc->prepare_tx_desc(desc, 0, len, csum_insertion,
>  						priv->mode);
> -		wmb();
> +		smp_wmb();
>  		priv->hw->desc->set_tx_owner(desc);
> -		wmb();

Since it is a loop, shouldn't we ensure that the ownership of a tx
descriptor is set before next descriptor in chain is programmed ?

>  	}
>  
>  	/* Finalize the latest segment. */
>  	priv->hw->desc->close_tx_desc(desc);
>  
> -	wmb();
>  	/* According to the coalesce parameter the IC bit for the latest
>  	 * segment could be reset and the timer re-started to invoke the
>  	 * stmmac_tx function. This approach takes care about the fragments.
> @@ -1821,9 +1819,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	} else
>  		priv->tx_count_frames = 0;
>  
> +	smp_wmb();

Please reconsider, may be keeping wmb is better.

>  	/* To avoid raise condition */
>  	priv->hw->desc->set_tx_owner(first);
> -	wmb();

Not sure about this, perhaps can be removed.

>  
>  	priv->cur_tx++;
>  
> @@ -1899,9 +1897,8 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv)
>  
>  			RX_DBG(KERN_INFO "\trefill entry #%d\n", entry);
>  		}
> -		wmb();
> +		smp_wmb();
>  		priv->hw->desc->set_rx_owner(p);
> -		wmb();

Similarly this is a part of a loop, we need to see if set rx owner
should be reflected before next descriptor program.

--
regards
Shiraz
--
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
Sergei Shtylyov - April 3, 2013, 2:02 p.m.
Hello.

On 03-04-2013 9:41, Giuseppe CAVALLARO wrote:

> In all my tests performed on SH4 and ARM A9 platforms, I've never met problems
> that can be fixed by using memory barriers. In the past there was some issues
> on SMP ARM but fixed by reviewing xmit spinlock.

> Further barriers have been added in the commits too: 8e83989106562326bf

    Please also specify that commit's summary line in parens.

> This patch is to use the smp_wbm instead of wbm because the driver

    It's "wmb".

> runs on UP systems. Then, IMO it could make sense to only maintain the barriers
> just in places where we touch the dma owner bits (that is the
> only real critical path as we had seen and fixed in the commit:
> eb0dc4bb2e22c04964d).

   This one's too.

> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Deepak Sikri <deepak.sikri@st.com>
> Cc: Shiraz Hashim <shiraz.hashim@st.com>

WBR, Sergei


--
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
Giuseppe CAVALLARO - April 4, 2013, 6:06 a.m.
Ciao Shiraz!

On 4/3/2013 10:51 AM, Shiraz HASHIM wrote:
> Hi Giuseppe,
>
> On Wed, Apr 03, 2013 at 01:41:24PM +0800, Giuseppe CAVALLARO wrote:
>> In all my tests performed on SH4 and ARM A9 platforms, I've never met problems
>> that can be fixed by using memory barriers. In the past there was some issues
>> on SMP ARM but fixed by reviewing xmit spinlock.
>
> The problem which was addressed was not because of SMP IMO. It was
> rather due to the fact that the write to the GMAC descriptor (which
> happens to be in normal memory) has to be ordered with respect to GMAC
> DMA as observer. Isn't it ?

Hmm yes you are right, now I remember that this was a code reordering
issue especially when we had looked at the commit:

stmmac: add memory barriers at appropriate places
  eb0dc4bb2e22c04964d

>> Further barriers have been added in the commits too: 8e83989106562326bf
>>
>> This patch is to use the smp_wbm instead of wbm because the driver
>                             ^^^^^^^^^^^^^^^^^^^^^^
> Perhaps you meant smp_wmb and wmb

sure.

from the commit:
stmmac: Fix for nfs hang on multiple reboot
    8e83989106562326bf

I had not understand if the problem was related to the SMP or to the
code ordering.
>
>> runs on UP systems. Then, IMO it could make sense to only maintain the barriers
>> just in places where we touch the dma owner bits (that is the
>> only real critical path as we had seen and fixed in the commit:
>> eb0dc4bb2e22c04964d).
>
> Replacing wmb by smp_wmb may not be a good idea as we need to order
> the store transaction to the descriptor with respect to GMAC DMA and
> using smp_* version would just be compiler barrier in uniprocessor
> systems.

Yes this what I wanted although the main point remains pending.
On my side, on SH4 (UP) and ARM (SMP) with several different
compiler and flags I have never seen problems and no barriers
are needed.

Especially in the commit "stmmac: Fix for nfs hang on multiple reboot"
the description of the problem looks to be quite obscure and I cannot
find any "particular" relation with extra barrier.

In fact, if we can demonstrate that barriers are needed no problem to
keep them in the code. Otherwise I prefer to remove them.

What do you think?

Cheers
peppe

>> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Cc: Deepak Sikri <deepak.sikri@st.com>
>> Cc: Shiraz Hashim <shiraz.hashim@st.com>
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    9 +++------
>>   1 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 8b69e3b..c92dcbc 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -1797,15 +1797,13 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>   		priv->tx_skbuff[entry] = NULL;
>>   		priv->hw->desc->prepare_tx_desc(desc, 0, len, csum_insertion,
>>   						priv->mode);
>> -		wmb();
>> +		smp_wmb();
>>   		priv->hw->desc->set_tx_owner(desc);
>> -		wmb();
>
> Since it is a loop, shouldn't we ensure that the ownership of a tx
> descriptor is set before next descriptor in chain is programmed ?
>
>>   	}
>>
>>   	/* Finalize the latest segment. */
>>   	priv->hw->desc->close_tx_desc(desc);
>>
>> -	wmb();
>>   	/* According to the coalesce parameter the IC bit for the latest
>>   	 * segment could be reset and the timer re-started to invoke the
>>   	 * stmmac_tx function. This approach takes care about the fragments.
>> @@ -1821,9 +1819,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>   	} else
>>   		priv->tx_count_frames = 0;
>>
>> +	smp_wmb();
>
> Please reconsider, may be keeping wmb is better.
>
>>   	/* To avoid raise condition */
>>   	priv->hw->desc->set_tx_owner(first);
>> -	wmb();
>
> Not sure about this, perhaps can be removed.
>
>>
>>   	priv->cur_tx++;
>>
>> @@ -1899,9 +1897,8 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv)
>>
>>   			RX_DBG(KERN_INFO "\trefill entry #%d\n", entry);
>>   		}
>> -		wmb();
>> +		smp_wmb();
>>   		priv->hw->desc->set_rx_owner(p);
>> -		wmb();
>
> Similarly this is a part of a loop, we need to see if set rx owner
> should be reflected before next descriptor program.
>
> --
> regards
> Shiraz
>

--
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
Eric Dumazet - April 4, 2013, 3:08 p.m.
On Thu, 2013-04-04 at 08:06 +0200, Giuseppe CAVALLARO wrote:

> In fact, if we can demonstrate that barriers are needed no problem to
> keep them in the code. Otherwise I prefer to remove them.
> 
> What do you think?

I think there are needed, and its really obvious.

Now maybe your arch can define wmb() as a pure compiler barrier(), but
thats a completely different patch.





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

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8b69e3b..c92dcbc 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1797,15 +1797,13 @@  static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 		priv->tx_skbuff[entry] = NULL;
 		priv->hw->desc->prepare_tx_desc(desc, 0, len, csum_insertion,
 						priv->mode);
-		wmb();
+		smp_wmb();
 		priv->hw->desc->set_tx_owner(desc);
-		wmb();
 	}
 
 	/* Finalize the latest segment. */
 	priv->hw->desc->close_tx_desc(desc);
 
-	wmb();
 	/* According to the coalesce parameter the IC bit for the latest
 	 * segment could be reset and the timer re-started to invoke the
 	 * stmmac_tx function. This approach takes care about the fragments.
@@ -1821,9 +1819,9 @@  static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	} else
 		priv->tx_count_frames = 0;
 
+	smp_wmb();
 	/* To avoid raise condition */
 	priv->hw->desc->set_tx_owner(first);
-	wmb();
 
 	priv->cur_tx++;
 
@@ -1899,9 +1897,8 @@  static inline void stmmac_rx_refill(struct stmmac_priv *priv)
 
 			RX_DBG(KERN_INFO "\trefill entry #%d\n", entry);
 		}
-		wmb();
+		smp_wmb();
 		priv->hw->desc->set_rx_owner(p);
-		wmb();
 	}
 }