| Message ID | 1364967689-11155-2-git-send-email-peppe.cavallaro@st.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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(); } }
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(-)