[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 04 Apr 2013 08:06:27 +0200
From: Giuseppe CAVALLARO <peppe.cavallaro@...com>
To: Shiraz HASHIM <shiraz.hashim@...com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Deepak Sikri <deepak.sikri@...com>,
sergei.shtylyov@...entembedded.com
Subject: Re: [net-next.git 2/7] stmmac: review barriers
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@...com>
>> Cc: Deepak Sikri <deepak.sikri@...com>
>> Cc: Shiraz Hashim <shiraz.hashim@...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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists