Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer
From: Lino Sanfilippo <hidden>
Date: 2016-05-22 11:30:27
Also in:
linux-rockchip
Possibly related (same subject, not in this thread)
- 2016-06-08 · Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer · Lino Sanfilippo <hidden>
- 2016-06-05 · Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer · Shuyu Wei <hidden>
- 2016-05-30 · Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer · Lino Sanfilippo <LinoSanfilippo@gmx.de>
- 2016-05-28 · Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer · Shuyu Wei <hidden>
- 2016-05-24 · Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer · Lino Sanfilippo <hidden>
On 22.05.2016 11:17, Shuyu Wei wrote:
Hi Lino, I tested this patch, it still got panic under stress. Just wget 2 large files simultaneously and it failed. Looks like the problem comes from the if statement in tx_clean(). I changed your patch to use - if (info & FOR_EMAC) + if ((info & FOR_EMAC) || !txbd->data || !skb) and it worked.
Thanks for testing. However that extra check for skb not being NULL should not be necessary if the code were correct. The changes I suggested were all about having skb and info consistent with txbd_curr. But I just realized that there is still a big flaw in the last changes. While tx() looks correct now (we first set up the descriptor and assign the skb and _then_ advance txbd_curr) tx_clean still is not: We _first_ have to read tx_curr and _then_ read the corresponding descriptor and its skb. (The last patch implemented just the reverse - and thus wrong - order, first get skb and descriptor and then read tx_curr). So the patch below hopefully handles also tx_clean correctly. Could you please do once more a test with this one?
After further test, my patch to barrier timestamp() didn't work. Just like the original code in the tree, the emac still got stuck under high load, even if I changed the smp_wmb() to dma_wmb(). So the original code do have race somewhere.
So to make this clear: with the current code in net-next you still see a problem (lockup), right?
I'm new to kernel development, and still trying to understand how memory barrier works
Its an interresting topic and thats what I am trying to understand, too :)
... and why Francois' fix worked. Please be patient with me :-).
So which fix(es) exactly work for you and solve your lockup issue?
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c@@ -159,12 +159,22 @@ static void arc_emac_tx_clean(struct net_device *ndev) unsigned int *txbd_dirty = &priv->txbd_dirty; struct arc_emac_bd *txbd = &priv->txbd[*txbd_dirty]; struct buffer_state *tx_buff = &priv->tx_buff[*txbd_dirty]; - struct sk_buff *skb = tx_buff->skb; unsigned int info = le32_to_cpu(txbd->info); + struct sk_buff *skb; - if ((info & FOR_EMAC) || !txbd->data || !skb) + if (*txbd_dirty == priv->txbd_curr) break; + /* Make sure curr pointer is consistent with info */ + rmb(); + + info = le32_to_cpu(txbd->info); + + if (info & FOR_EMAC) + break; + + skb = tx_buff->skb; + if (unlikely(info & (DROP | DEFR | LTCL | UFLO))) { stats->tx_errors++; stats->tx_dropped++;
@@ -195,8 +205,8 @@ static void arc_emac_tx_clean(struct net_device *ndev) *txbd_dirty = (*txbd_dirty + 1) % TX_BD_NUM; } - /* Ensure that txbd_dirty is visible to tx() before checking - * for queue stopped. + /* Ensure that txbd_dirty is visible to tx() and we see the most recent + * value for txbd_curr. */ smp_mb();
@@ -680,35 +690,29 @@ static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev) dma_unmap_len_set(&priv->tx_buff[*txbd_curr], len, len); priv->txbd[*txbd_curr].data = cpu_to_le32(addr); - - /* Make sure pointer to data buffer is set */ - wmb(); + priv->tx_buff[*txbd_curr].skb = skb; skb_tx_timestamp(skb); *info = cpu_to_le32(FOR_EMAC | FIRST_OR_LAST_MASK | len); - /* Make sure info word is set */ + /* 1. Make sure that with respect to tx_clean everything is set up + * properly before we advance txbd_curr. + * 2. Make sure writes to DMA descriptors are completed before we inform + * the hardware. + */ wmb(); - priv->tx_buff[*txbd_curr].skb = skb; - /* Increment index to point to the next BD */ *txbd_curr = (*txbd_curr + 1) % TX_BD_NUM; - /* Ensure that tx_clean() sees the new txbd_curr before - * checking the queue status. This prevents an unneeded wake - * of the queue in tx_clean(). + /* Ensure we see the most recent value of txbd_dirty and tx_clean() sees + * the updated value of txbd_curr. */ smp_mb(); - if (!arc_emac_tx_avail(priv)) { + if (!arc_emac_tx_avail(priv)) netif_stop_queue(ndev); - /* Refresh tx_dirty */ - smp_mb(); - if (arc_emac_tx_avail(priv)) - netif_start_queue(ndev); - } arc_reg_set(priv, R_STATUS, TXPL_MASK);
--
1.9.1