Re: [PATCH] gianfar: Fix TX ring processing on SMP machines
On Wed, Mar 3, 2010 at 8:18 PM, Anton Vorontsov wrote: > Starting with commit a3bc1f11e9b867a4f49505 ("gianfar: Revive SKB > recycling") gianfar driver sooner or later stops transmitting any > packets on SMP machines. > > start_xmit() prepares new skb for transmitting, generally it does > three things: > > 1. sets up all BDs (marks them ready to send), except the first one. > 2. stores skb into tx_queue->tx_skbuff so that clean_tx_ring() > would cleanup it later. > 3. sets up the first BD, i.e. marks it ready. > > Here is what clean_tx_ring() does: > > 1. reads skbs from tx_queue->tx_skbuff > 2. checks if the *last* BD is ready. If it's still ready [to send] > then it it isn't transmitted, so clean_tx_ring() returns. > Otherwise it actually cleanups BDs. All is OK. > > Now, if there is just one BD, code flow: > > - start_xmit(): stores skb into tx_skbuff. Note that the first BD > (which is also the last one) isn't marked as ready, yet. > - clean_tx_ring(): sees that skb is not null, *and* its lstatus > says that it is NOT ready (like if BD was sent), so it cleans > it up (bad!) > - start_xmit(): marks BD as ready [to send], but it's too late. > > We can fix this simply by reordering lstatus/tx_skbuff writes. > > Reported-by: Martyn Welch > Bisected-by: Paul Gortmaker > Signed-off-by: Anton Vorontsov > Tested-by: Paul Gortmaker > Tested-by: Martyn Welch > Cc: Sandeep Gopalpet > Cc: Stable [2.6.33] > --- > drivers/net/gianfar.c | 5 - > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c > index 8bd3c9f..cccb409 100644 > --- a/drivers/net/gianfar.c > +++ b/drivers/net/gianfar.c > @@ -2021,7 +2021,6 @@ static int gfar_start_xmit(struct sk_buff *skb, struct > net_device *dev) > } > > /* setup the TxBD length and buffer pointer for the first BD */ > - tx_queue->tx_skbuff[tx_queue->skb_curtx] = skb; > txbdp_start->bufPtr = dma_map_single(&priv->ofdev->dev, skb->data, > skb_headlen(skb), DMA_TO_DEVICE); > > @@ -2053,6 +2052,10 @@ static int gfar_start_xmit(struct sk_buff *skb, struct > net_device *dev) > > txbdp_start->lstatus = lstatus; > > + eieio(); /* force lstatus write before tx_skbuff */ > + > + tx_queue->tx_skbuff[tx_queue->skb_curtx] = skb; > + > /* Update the current skb pointer to the next entry we will use > * (wrapping if necessary) */ > tx_queue->skb_curtx = (tx_queue->skb_curtx + 1) & This patch also makes gianfar work stable on mpc8313 with 2.6.33/RT_PREEMPT. WIthout it, I see exactly the same problems as reported by Anton on SMP. /Esben -- Esben Haabendal, Senior Software Consultant DoréDevelopment ApS, Ved Stranden 1, 9560 Hadsund, DK-Denmark Phone: +45 51 92 53 93, E-mail: e...@doredevelopment.dk WWW: http://www.doredevelopment.dk ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] gianfar: Fix TX ring processing on SMP machines
On Mar 4, 2010, at 2:41 AM, David Miller wrote: > From: Anton Vorontsov > Date: Wed, 3 Mar 2010 21:18:58 +0300 > >> Starting with commit a3bc1f11e9b867a4f49505 ("gianfar: Revive SKB >> recycling") gianfar driver sooner or later stops transmitting any >> packets on SMP machines. >> >> start_xmit() prepares new skb for transmitting, generally it does >> three things: >> >> 1. sets up all BDs (marks them ready to send), except the first one. >> 2. stores skb into tx_queue->tx_skbuff so that clean_tx_ring() >> would cleanup it later. >> 3. sets up the first BD, i.e. marks it ready. >> >> Here is what clean_tx_ring() does: >> >> 1. reads skbs from tx_queue->tx_skbuff >> 2. checks if the *last* BD is ready. If it's still ready [to send] >> then it it isn't transmitted, so clean_tx_ring() returns. >> Otherwise it actually cleanups BDs. All is OK. >> >> Now, if there is just one BD, code flow: >> >> - start_xmit(): stores skb into tx_skbuff. Note that the first BD >> (which is also the last one) isn't marked as ready, yet. >> - clean_tx_ring(): sees that skb is not null, *and* its lstatus >> says that it is NOT ready (like if BD was sent), so it cleans >> it up (bad!) >> - start_xmit(): marks BD as ready [to send], but it's too late. >> >> We can fix this simply by reordering lstatus/tx_skbuff writes. >> >> Reported-by: Martyn Welch >> Bisected-by: Paul Gortmaker >> Signed-off-by: Anton Vorontsov >> Tested-by: Paul Gortmaker >> Tested-by: Martyn Welch > > Applied. Anton, Once this makes it into Linus's tree can you make sure we get it added to -stable. - k ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] gianfar: Fix TX ring processing on SMP machines
From: Anton Vorontsov Date: Wed, 3 Mar 2010 21:18:58 +0300 > Starting with commit a3bc1f11e9b867a4f49505 ("gianfar: Revive SKB > recycling") gianfar driver sooner or later stops transmitting any > packets on SMP machines. > > start_xmit() prepares new skb for transmitting, generally it does > three things: > > 1. sets up all BDs (marks them ready to send), except the first one. > 2. stores skb into tx_queue->tx_skbuff so that clean_tx_ring() >would cleanup it later. > 3. sets up the first BD, i.e. marks it ready. > > Here is what clean_tx_ring() does: > > 1. reads skbs from tx_queue->tx_skbuff > 2. checks if the *last* BD is ready. If it's still ready [to send] >then it it isn't transmitted, so clean_tx_ring() returns. >Otherwise it actually cleanups BDs. All is OK. > > Now, if there is just one BD, code flow: > > - start_xmit(): stores skb into tx_skbuff. Note that the first BD > (which is also the last one) isn't marked as ready, yet. > - clean_tx_ring(): sees that skb is not null, *and* its lstatus > says that it is NOT ready (like if BD was sent), so it cleans > it up (bad!) > - start_xmit(): marks BD as ready [to send], but it's too late. > > We can fix this simply by reordering lstatus/tx_skbuff writes. > > Reported-by: Martyn Welch > Bisected-by: Paul Gortmaker > Signed-off-by: Anton Vorontsov > Tested-by: Paul Gortmaker > Tested-by: Martyn Welch Applied. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] gianfar: Fix TX ring processing on SMP machines
Starting with commit a3bc1f11e9b867a4f49505 ("gianfar: Revive SKB recycling") gianfar driver sooner or later stops transmitting any packets on SMP machines. start_xmit() prepares new skb for transmitting, generally it does three things: 1. sets up all BDs (marks them ready to send), except the first one. 2. stores skb into tx_queue->tx_skbuff so that clean_tx_ring() would cleanup it later. 3. sets up the first BD, i.e. marks it ready. Here is what clean_tx_ring() does: 1. reads skbs from tx_queue->tx_skbuff 2. checks if the *last* BD is ready. If it's still ready [to send] then it it isn't transmitted, so clean_tx_ring() returns. Otherwise it actually cleanups BDs. All is OK. Now, if there is just one BD, code flow: - start_xmit(): stores skb into tx_skbuff. Note that the first BD (which is also the last one) isn't marked as ready, yet. - clean_tx_ring(): sees that skb is not null, *and* its lstatus says that it is NOT ready (like if BD was sent), so it cleans it up (bad!) - start_xmit(): marks BD as ready [to send], but it's too late. We can fix this simply by reordering lstatus/tx_skbuff writes. Reported-by: Martyn Welch Bisected-by: Paul Gortmaker Signed-off-by: Anton Vorontsov Tested-by: Paul Gortmaker Tested-by: Martyn Welch Cc: Sandeep Gopalpet Cc: Stable [2.6.33] --- drivers/net/gianfar.c |5 - 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c index 8bd3c9f..cccb409 100644 --- a/drivers/net/gianfar.c +++ b/drivers/net/gianfar.c @@ -2021,7 +2021,6 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) } /* setup the TxBD length and buffer pointer for the first BD */ - tx_queue->tx_skbuff[tx_queue->skb_curtx] = skb; txbdp_start->bufPtr = dma_map_single(&priv->ofdev->dev, skb->data, skb_headlen(skb), DMA_TO_DEVICE); @@ -2053,6 +2052,10 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) txbdp_start->lstatus = lstatus; + eieio(); /* force lstatus write before tx_skbuff */ + + tx_queue->tx_skbuff[tx_queue->skb_curtx] = skb; + /* Update the current skb pointer to the next entry we will use * (wrapping if necessary) */ tx_queue->skb_curtx = (tx_queue->skb_curtx + 1) & -- 1.7.0 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev