[PATCH RFT net-next #2 0/6] via-rhine receive buffers rework

[PATCH RFT net-next #2 0/6] via-rhine receive buffers rework

[Francois Romieu]

The series applies without error against v3.19 and davem-next as of
812034f11628aaaab0e2d7af1d3bc50a49eb396b ("cxgb4: Move ethtool related
code to a separate file").

. Changes since #1:
  - turned wmb() into dma_wmb() as suggested by davem and Alexander Duyck
    in patch 1 of 6.
  - forgot to reset rx_head_desc in rhine_reset_rbufs in patch 4 of 6.
  - removed rx_head_desc altogether in (*new*) patch 5 of 6
  - remoed some vlan receive uglyness in (*new*) patch 6 of 6.

Francois Romieu (6):
  via-rhine: commit receive buffer address before descriptor status
    update.
  via-rhine: add allocation helpers.
  via-rhine: gotoize rhine_open error path.
  via-rhine: forbid holes in the receive descriptor ring.
  via-rhine: kiss rx_head_desc goodbye.
  via-rhine: beautify vlan receive code.

 drivers/net/ethernet/via/via-rhine.c | 195 +++++++++++++++++++++--------------
 1 file changed, 115 insertions(+), 80 deletions(-)

-- 
Ueimor

[PATCH net-next #2 1/6] via-rhine: commit receive buffer address before descriptor status update.

[Francois Romieu]

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/via/via-rhine.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index de28504..88b45c7 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -2075,6 +2075,7 @@ static int rhine_rx(struct net_device *dev, int limit)
 				break;
 			}
 			rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]);
+			dma_wmb();
 		}
 		rp->rx_ring[entry].rx_status = cpu_to_le32(DescOwn);
 	}
-- 
2.1.0

[PATCH net-next #2 2/6] via-rhine: add allocation helpers.

[Francois Romieu]

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/via/via-rhine.c | 57 +++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 88b45c7..aa398ea 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -1213,12 +1213,47 @@ static void free_ring(struct net_device* dev)
 
 }
 
-static void alloc_rbufs(struct net_device *dev)
+struct rhine_skb_dma {
+	struct sk_buff *skb;
+	dma_addr_t dma;
+};
+
+static inline int rhine_skb_dma_init(struct net_device *dev,
+				     struct rhine_skb_dma *sd)
 {
 	struct rhine_private *rp = netdev_priv(dev);
 	struct device *hwdev = dev->dev.parent;
+	const int size = rp->rx_buf_sz;
+
+	sd->skb = netdev_alloc_skb(dev, size);
+	if (!sd->skb)
+		return -ENOMEM;
+
+	sd->dma = dma_map_single(hwdev, sd->skb->data, size, DMA_FROM_DEVICE);
+	if (unlikely(dma_mapping_error(hwdev, sd->dma))) {
+		netif_err(rp, drv, dev, "Rx DMA mapping failure\n");
+		dev_kfree_skb_any(sd->skb);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static inline void rhine_skb_dma_nic_store(struct rhine_private *rp,
+					   struct rhine_skb_dma *sd, int entry)
+{
+	rp->rx_skbuff_dma[entry] = sd->dma;
+	rp->rx_skbuff[entry] = sd->skb;
+
+	rp->rx_ring[entry].addr = cpu_to_le32(sd->dma);
+	dma_wmb();
+}
+
+static void alloc_rbufs(struct net_device *dev)
+{
+	struct rhine_private *rp = netdev_priv(dev);
 	dma_addr_t next;
-	int i;
+	int rc, i;
 
 	rp->dirty_rx = rp->cur_rx = 0;
 
@@ -1239,20 +1274,14 @@ static void alloc_rbufs(struct net_device *dev)
 
 	/* Fill in the Rx buffers.  Handle allocation failure gracefully. */
 	for (i = 0; i < RX_RING_SIZE; i++) {
-		struct sk_buff *skb = netdev_alloc_skb(dev, rp->rx_buf_sz);
-		rp->rx_skbuff[i] = skb;
-		if (skb == NULL)
-			break;
+		struct rhine_skb_dma sd;
 
-		rp->rx_skbuff_dma[i] =
-			dma_map_single(hwdev, skb->data, rp->rx_buf_sz,
-				       DMA_FROM_DEVICE);
-		if (dma_mapping_error(hwdev, rp->rx_skbuff_dma[i])) {
-			rp->rx_skbuff_dma[i] = 0;
-			dev_kfree_skb(skb);
+		rc = rhine_skb_dma_init(dev, &sd);
+		if (rc < 0)
 			break;
-		}
-		rp->rx_ring[i].addr = cpu_to_le32(rp->rx_skbuff_dma[i]);
+
+		rhine_skb_dma_nic_store(rp, &sd, i);
+
 		rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);
 	}
 	rp->dirty_rx = (unsigned int)(i - RX_RING_SIZE);
-- 
2.1.0

[PATCH net-next #2 3/6] via-rhine: gotoize rhine_open error path.

[Francois Romieu]

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/via/via-rhine.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index aa398ea..91661e0 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -1688,15 +1688,14 @@ static int rhine_open(struct net_device *dev)
 
 	rc = request_irq(rp->irq, rhine_interrupt, IRQF_SHARED, dev->name, dev);
 	if (rc)
-		return rc;
+		goto out;
 
 	netif_dbg(rp, ifup, dev, "%s() irq %d\n", __func__, rp->irq);
 
 	rc = alloc_ring(dev);
-	if (rc) {
-		free_irq(rp->irq, dev);
-		return rc;
-	}
+	if (rc < 0)
+		goto out_free_irq;
+
 	alloc_rbufs(dev);
 	alloc_tbufs(dev);
 	rhine_chip_reset(dev);
@@ -1709,7 +1708,12 @@ static int rhine_open(struct net_device *dev)
 
 	netif_start_queue(dev);
 
-	return 0;
+out:
+	return rc;
+
+out_free_irq:
+	free_irq(rp->irq, dev);
+	goto out;
 }
 
 static void rhine_reset_task(struct work_struct *work)
-- 
2.1.0

[PATCH net-next #2 4/6] via-rhine: forbid holes in the receive descriptor ring.

[Francois Romieu]

Rationales:
- throttle work under memory pressure;
- lower receive descriptor recycling latency for the network adapter;
- lower the maintenance burden of uncommon paths.

The patch is twofold:
1. it fails early if the receive buffer ring can't be completely
   initialized at dev->open() time
2. it drops packets on the floor in the napi receive handler so as to
   keep the ring full.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/via/via-rhine.c | 103 ++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 50 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 91661e0..8d322bb 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -473,7 +473,7 @@ struct rhine_private {
 	/* Frequently used values: keep some adjacent for cache effect. */
 	u32 quirks;
 	struct rx_desc *rx_head_desc;
-	unsigned int cur_rx, dirty_rx;	/* Producer/consumer ring indices */
+	unsigned int cur_rx;
 	unsigned int cur_tx, dirty_tx;
 	unsigned int rx_buf_sz;		/* Based on MTU+slack. */
 	struct rhine_stats rx_stats;
@@ -1239,6 +1239,17 @@ static inline int rhine_skb_dma_init(struct net_device *dev,
 	return 0;
 }
 
+static void rhine_reset_rbufs(struct rhine_private *rp)
+{
+	int i;
+
+	rp->cur_rx = 0;
+	rp->rx_head_desc = rp->rx_ring;
+
+	for (i = 0; i < RX_RING_SIZE; i++)
+		rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);
+}
+
 static inline void rhine_skb_dma_nic_store(struct rhine_private *rp,
 					   struct rhine_skb_dma *sd, int entry)
 {
@@ -1249,16 +1260,15 @@ static inline void rhine_skb_dma_nic_store(struct rhine_private *rp,
 	dma_wmb();
 }
 
-static void alloc_rbufs(struct net_device *dev)
+static void free_rbufs(struct net_device* dev);
+
+static int alloc_rbufs(struct net_device *dev)
 {
 	struct rhine_private *rp = netdev_priv(dev);
 	dma_addr_t next;
 	int rc, i;
 
-	rp->dirty_rx = rp->cur_rx = 0;
-
 	rp->rx_buf_sz = (dev->mtu <= 1500 ? PKT_BUF_SZ : dev->mtu + 32);
-	rp->rx_head_desc = &rp->rx_ring[0];
 	next = rp->rx_ring_dma;
 
 	/* Init the ring entries */
@@ -1277,14 +1287,17 @@ static void alloc_rbufs(struct net_device *dev)
 		struct rhine_skb_dma sd;
 
 		rc = rhine_skb_dma_init(dev, &sd);
-		if (rc < 0)
-			break;
+		if (rc < 0) {
+			free_rbufs(dev);
+			goto out;
+		}
 
 		rhine_skb_dma_nic_store(rp, &sd, i);
-
-		rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);
 	}
-	rp->dirty_rx = (unsigned int)(i - RX_RING_SIZE);
+
+	rhine_reset_rbufs(rp);
+out:
+	return rc;
 }
 
 static void free_rbufs(struct net_device* dev)
@@ -1696,7 +1709,10 @@ static int rhine_open(struct net_device *dev)
 	if (rc < 0)
 		goto out_free_irq;
 
-	alloc_rbufs(dev);
+	rc = alloc_rbufs(dev);
+	if (rc < 0)
+		goto out_free_ring;
+
 	alloc_tbufs(dev);
 	rhine_chip_reset(dev);
 	rhine_task_enable(rp);
@@ -1711,6 +1727,8 @@ static int rhine_open(struct net_device *dev)
 out:
 	return rc;
 
+out_free_ring:
+	free_ring(dev);
 out_free_irq:
 	free_irq(rp->irq, dev);
 	goto out;
@@ -1733,9 +1751,9 @@ static void rhine_reset_task(struct work_struct *work)
 
 	/* clear all descriptors */
 	free_tbufs(dev);
-	free_rbufs(dev);
 	alloc_tbufs(dev);
-	alloc_rbufs(dev);
+
+	rhine_reset_rbufs(rp);
 
 	/* Reinitialize the hardware. */
 	rhine_chip_reset(dev);
@@ -2033,16 +2051,18 @@ static int rhine_rx(struct net_device *dev, int limit)
 				}
 			}
 		} else {
-			struct sk_buff *skb = NULL;
 			/* Length should omit the CRC */
 			int pkt_len = data_size - 4;
+			struct sk_buff *skb;
 			u16 vlan_tci = 0;
 
 			/* Check if the packet is long enough to accept without
 			   copying to a minimally-sized skbuff. */
-			if (pkt_len < rx_copybreak)
+			if (pkt_len < rx_copybreak) {
 				skb = netdev_alloc_skb_ip_align(dev, pkt_len);
-			if (skb) {
+				if (unlikely(!skb))
+					goto drop;
+
 				dma_sync_single_for_cpu(hwdev,
 							rp->rx_skbuff_dma[entry],
 							rp->rx_buf_sz,
@@ -2051,25 +2071,28 @@ static int rhine_rx(struct net_device *dev, int limit)
 				skb_copy_to_linear_data(skb,
 						 rp->rx_skbuff[entry]->data,
 						 pkt_len);
-				skb_put(skb, pkt_len);
+
 				dma_sync_single_for_device(hwdev,
 							   rp->rx_skbuff_dma[entry],
 							   rp->rx_buf_sz,
 							   DMA_FROM_DEVICE);
 			} else {
+				struct rhine_skb_dma sd;
+
+				if (unlikely(rhine_skb_dma_init(dev, &sd) < 0))
+					goto drop;
+
 				skb = rp->rx_skbuff[entry];
-				if (skb == NULL) {
-					netdev_err(dev, "Inconsistent Rx descriptor chain\n");
-					break;
-				}
-				rp->rx_skbuff[entry] = NULL;
-				skb_put(skb, pkt_len);
+
 				dma_unmap_single(hwdev,
 						 rp->rx_skbuff_dma[entry],
 						 rp->rx_buf_sz,
 						 DMA_FROM_DEVICE);
+				rhine_skb_dma_nic_store(rp, &sd, entry);
 			}
 
+			skb_put(skb, pkt_len);
+
 			if (unlikely(desc_length & DescTag))
 				vlan_tci = rhine_get_vlan_tci(skb, data_size);
 
@@ -2084,36 +2107,17 @@ static int rhine_rx(struct net_device *dev, int limit)
 			rp->rx_stats.packets++;
 			u64_stats_update_end(&rp->rx_stats.syncp);
 		}
+give_descriptor_to_nic:
+		desc->rx_status = cpu_to_le32(DescOwn);
 		entry = (++rp->cur_rx) % RX_RING_SIZE;
 		rp->rx_head_desc = &rp->rx_ring[entry];
 	}
 
-	/* Refill the Rx ring buffers. */
-	for (; rp->cur_rx - rp->dirty_rx > 0; rp->dirty_rx++) {
-		struct sk_buff *skb;
-		entry = rp->dirty_rx % RX_RING_SIZE;
-		if (rp->rx_skbuff[entry] == NULL) {
-			skb = netdev_alloc_skb(dev, rp->rx_buf_sz);
-			rp->rx_skbuff[entry] = skb;
-			if (skb == NULL)
-				break;	/* Better luck next round. */
-			rp->rx_skbuff_dma[entry] =
-				dma_map_single(hwdev, skb->data,
-					       rp->rx_buf_sz,
-					       DMA_FROM_DEVICE);
-			if (dma_mapping_error(hwdev,
-					      rp->rx_skbuff_dma[entry])) {
-				dev_kfree_skb(skb);
-				rp->rx_skbuff_dma[entry] = 0;
-				break;
-			}
-			rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]);
-			dma_wmb();
-		}
-		rp->rx_ring[entry].rx_status = cpu_to_le32(DescOwn);
-	}
-
 	return count;
+
+drop:
+	dev->stats.rx_dropped++;
+	goto give_descriptor_to_nic;
 }
 
 static void rhine_restart_tx(struct net_device *dev) {
@@ -2518,9 +2522,8 @@ static int rhine_resume(struct device *device)
 	enable_mmio(rp->pioaddr, rp->quirks);
 	rhine_power_init(dev);
 	free_tbufs(dev);
-	free_rbufs(dev);
 	alloc_tbufs(dev);
-	alloc_rbufs(dev);
+	rhine_reset_rbufs(rp);
 	rhine_task_enable(rp);
 	spin_lock_bh(&rp->lock);
 	init_registers(dev);
-- 
2.1.0

[PATCH net-next #2 5/6] via-rhine: kiss rx_head_desc goodbye.

[Francois Romieu]

It duplicates cur_rx.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/via/via-rhine.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 8d322bb..fede9fc 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -472,7 +472,6 @@ struct rhine_private {
 
 	/* Frequently used values: keep some adjacent for cache effect. */
 	u32 quirks;
-	struct rx_desc *rx_head_desc;
 	unsigned int cur_rx;
 	unsigned int cur_tx, dirty_tx;
 	unsigned int rx_buf_sz;		/* Based on MTU+slack. */
@@ -1244,7 +1243,6 @@ static void rhine_reset_rbufs(struct rhine_private *rp)
 	int i;
 
 	rp->cur_rx = 0;
-	rp->rx_head_desc = rp->rx_ring;
 
 	for (i = 0; i < RX_RING_SIZE; i++)
 		rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);
@@ -2000,15 +1998,15 @@ static int rhine_rx(struct net_device *dev, int limit)
 {
 	struct rhine_private *rp = netdev_priv(dev);
 	struct device *hwdev = dev->dev.parent;
-	int count;
 	int entry = rp->cur_rx % RX_RING_SIZE;
+	int count;
 
 	netif_dbg(rp, rx_status, dev, "%s(), entry %d status %08x\n", __func__,
-		  entry, le32_to_cpu(rp->rx_head_desc->rx_status));
+		  entry, le32_to_cpu(rp->rx_ring[entry].rx_status));
 
 	/* If EOP is set on the next entry, it's a new packet. Send it up. */
 	for (count = 0; count < limit; ++count) {
-		struct rx_desc *desc = rp->rx_head_desc;
+		struct rx_desc *desc = rp->rx_ring + entry;
 		u32 desc_status = le32_to_cpu(desc->rx_status);
 		u32 desc_length = le32_to_cpu(desc->desc_length);
 		int data_size = desc_status >> 16;
@@ -2026,10 +2024,6 @@ static int rhine_rx(struct net_device *dev, int limit)
 	"entry %#x length %d status %08x!\n",
 					    entry, data_size,
 					    desc_status);
-				netdev_warn(dev,
-					    "Oversized Ethernet frame %p vs %p\n",
-					    rp->rx_head_desc,
-					    &rp->rx_ring[entry]);
 				dev->stats.rx_length_errors++;
 			} else if (desc_status & RxErr) {
 				/* There was a error. */
@@ -2110,7 +2104,6 @@ static int rhine_rx(struct net_device *dev, int limit)
 give_descriptor_to_nic:
 		desc->rx_status = cpu_to_le32(DescOwn);
 		entry = (++rp->cur_rx) % RX_RING_SIZE;
-		rp->rx_head_desc = &rp->rx_ring[entry];
 	}
 
 	return count;
-- 
2.1.0

[PATCH net-next #2 6/6] via-rhine: beautify vlan receive code.

[Francois Romieu]

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/via/via-rhine.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index fede9fc..3e6fdbb 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -1993,6 +1993,17 @@ static inline u16 rhine_get_vlan_tci(struct sk_buff *skb, int data_size)
 	return be16_to_cpup((__be16 *)trailer);
 }
 
+static inline void rhine_rx_vlan_tag(struct sk_buff *skb, struct rx_desc *desc,
+				     int data_size)
+{
+	if (unlikely(desc->desc_length & cpu_to_le32(DescTag))) {
+		u16 vlan_tci;
+
+		vlan_tci = rhine_get_vlan_tci(skb, data_size);
+		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlan_tci);
+	}
+}
+
 /* Process up to limit frames from receive ring */
 static int rhine_rx(struct net_device *dev, int limit)
 {
@@ -2008,7 +2019,6 @@ static int rhine_rx(struct net_device *dev, int limit)
 	for (count = 0; count < limit; ++count) {
 		struct rx_desc *desc = rp->rx_ring + entry;
 		u32 desc_status = le32_to_cpu(desc->rx_status);
-		u32 desc_length = le32_to_cpu(desc->desc_length);
 		int data_size = desc_status >> 16;
 
 		if (desc_status & DescOwn)
@@ -2048,7 +2058,6 @@ static int rhine_rx(struct net_device *dev, int limit)
 			/* Length should omit the CRC */
 			int pkt_len = data_size - 4;
 			struct sk_buff *skb;
-			u16 vlan_tci = 0;
 
 			/* Check if the packet is long enough to accept without
 			   copying to a minimally-sized skbuff. */
@@ -2086,14 +2095,10 @@ static int rhine_rx(struct net_device *dev, int limit)
 			}
 
 			skb_put(skb, pkt_len);
-
-			if (unlikely(desc_length & DescTag))
-				vlan_tci = rhine_get_vlan_tci(skb, data_size);
-
 			skb->protocol = eth_type_trans(skb, dev);
 
-			if (unlikely(desc_length & DescTag))
-				__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlan_tci);
+			rhine_rx_vlan_tag(skb, desc, data_size);
+
 			netif_receive_skb(skb);
 
 			u64_stats_update_begin(&rp->rx_stats.syncp);
-- 
2.1.0

Re: [PATCH RFT net-next #2 0/6] via-rhine receive buffers rework

[Nix]

On 7 Apr 2015, Francois Romieu stated:

> The series applies without error against v3.19 and davem-next as of
> 812034f11628aaaab0e2d7af1d3bc50a49eb396b ("cxgb4: Move ethtool related
> code to a separate file").

I am sorry to report that I just had a watchdog-triggered autoreboot
during testing of this patch series :( with no log messages of any kind.
looks like the underlying bug is still there, or another bug with the
same symptoms (i.e. some way to crash inside the rx handler). I qwish I
could get some debugging output when this happens!

However, to give some good news, CPU usage is much lower than before the
patch: si ~10%, rather than ~80% with spikes of full CPU usage:
ksoftirqd's CPU usage is steady at about 3% rather than 40--60% with
spikes to 100%, and some of that will be USB interrupts from the
continuous USB traffic from my entropy key.

-- 
NULL && (void)

Re: [PATCH RFT net-next #2 0/6] via-rhine receive buffers rework

[Francois Romieu]

Nix <nix@esperi.org.uk> :
[...]
> I am sorry to report that I just had a watchdog-triggered autoreboot
> during testing of this patch series :( with no log messages of any kind.
> looks like the underlying bug is still there, or another bug with the
> same symptoms (i.e. some way to crash inside the rx handler). I qwish I
> could get some debugging output when this happens!

You may add the patch below on top of the current stack. I don't expect
much difference. Increasing RX_RING_SIZE could be a different story.

Did you keep netconsole disabled and did you increse via-rhine verbosity
level ?

No shared IRQ ?

> However, to give some good news, CPU usage is much lower than before the
> patch: si ~10%, rather than ~80% with spikes of full CPU usage:
> ksoftirqd's CPU usage is steady at about 3% rather than 40--60% with
> spikes to 100%, and some of that will be USB interrupts from the
> continuous USB traffic from my entropy key.

Huuuuh ? Which entropy key ?

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 3e6fdbb..d441d8c 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -1849,7 +1849,7 @@ static netdev_tx_t rhine_start_tx(struct sk_buff *skb,
 
 	netdev_sent_queue(dev, skb->len);
 	/* lock eth irq */
-	wmb();
+	dma_wmb();
 	rp->tx_ring[entry].tx_status |= cpu_to_le32(DescOwn);
 	wmb();
 
@@ -1996,6 +1996,7 @@ static inline u16 rhine_get_vlan_tci(struct sk_buff *skb, int data_size)
 static inline void rhine_rx_vlan_tag(struct sk_buff *skb, struct rx_desc *desc,
 				     int data_size)
 {
+	dma_rmb();
 	if (unlikely(desc->desc_length & cpu_to_le32(DescTag))) {
 		u16 vlan_tci;
 
@@ -2161,6 +2162,7 @@ static void rhine_slow_event_task(struct work_struct *work)
 		container_of(work, struct rhine_private, slow_event_task);
 	struct net_device *dev = rp->dev;
 	u32 intr_status;
+	u16 enable_mask;
 
 	mutex_lock(&rp->task_lock);
 
@@ -2176,7 +2178,12 @@ static void rhine_slow_event_task(struct work_struct *work)
 	if (intr_status & IntrPCIErr)
 		netif_warn(rp, hw, dev, "PCI error\n");
 
-	iowrite16(RHINE_EVENT & 0xffff, rp->base + IntrEnable);
+	napi_disable(&rp->napi);
+
+	enable_mask = ioread16(rp->base + IntrEnable) | RHINE_EVENT_SLOW;
+	iowrite16(enable_mask, rp->base + IntrEnable);
+
+	napi_enable(&rp->napi);
 
 out_unlock:
 	mutex_unlock(&rp->task_lock);

Re: [PATCH RFT net-next #2 0/6] via-rhine receive buffers rework

[Nix]

On 8 Apr 2015, Francois Romieu stated:

> Nix <nix@esperi.org.uk> :
> [...]
>> I am sorry to report that I just had a watchdog-triggered autoreboot
>> during testing of this patch series :( with no log messages of any kind.
>> looks like the underlying bug is still there, or another bug with the
>> same symptoms (i.e. some way to crash inside the rx handler). I qwish I
>> could get some debugging output when this happens!
>
> You may add the patch below on top of the current stack. I don't expect
> much difference. Increasing RX_RING_SIZE could be a different story.

Will try it tomorrow.

> Did you keep netconsole disabled and did you increse via-rhine verbosity
> level ?

netconsole is active. The verbosity level is default because I didn't
notice the driver had one to set! I'll push it up and see what happens.
I don't expect too much: this is, after all, a uniprocessor, and if
you're stuck in an interrupt handler there's not much it can do...

> No shared IRQ ?

None relevant that I can see:

           CPU0
  0:     692019    XT-PIC  timer
  2:          0    XT-PIC  cascade
  4:        314    XT-PIC  serial
  5:     114062    XT-PIC  adsl
  7:     329914    XT-PIC  cs5535-clockevt
  9:     137080    XT-PIC  bdsl
 10:       2006    XT-PIC  voip
 11:    1546540    XT-PIC  gordianet
 12:     724187    XT-PIC  wireless
 14:      12313    XT-PIC  pata_cs5536
 15:     219702    XT-PIC  ehci_hcd:usb1, ohci_hcd:usb2

The only shared interrupt is usb2, and since that's an
internal-to-the-case thing that's not plugged in to anything, in effect
there are none shared at all. (The unplugged interfaces *are* shared --
the last four of the eight interfaces on the box run 10, 7, 10, 7 -- but
since only one of those has anything plugged into it, the net effect is
no sharing.)

My load tests are being performed between the gordianet and wireless
interfaces, IRQs 11 and 12, which are entirely unshared.

>> However, to give some good news, CPU usage is much lower than before the
>> patch: si ~10%, rather than ~80% with spikes of full CPU usage:
>> ksoftirqd's CPU usage is steady at about 3% rather than 40--60% with
>> spikes to 100%, and some of that will be USB interrupts from the
>> continuous USB traffic from my entropy key.
>
> Huuuuh ? Which entropy key ?

It's a USB device that gives you about 4KiB/s over USB:
<http://www.entropykey.co.uk/>. No longer manufactured, alas :(
but as it provides entropy continuously it is a continuous source of
interrupts. (But from the USB port, obviously.)

-- 
NULL && (void)

Re: [PATCH RFT net-next #2 0/6] via-rhine receive buffers rework

[Nix]

On 8 Apr 2015, Francois Romieu spake thusly:

> Nix <nix@esperi.org.uk> :
> [...]
>> I am sorry to report that I just had a watchdog-triggered autoreboot
>> during testing of this patch series :( with no log messages of any kind.
>> looks like the underlying bug is still there, or another bug with the
>> same symptoms (i.e. some way to crash inside the rx handler). I qwish I
>> could get some debugging output when this happens!
>
> You may add the patch below on top of the current stack. I don't expect
> much difference. Increasing RX_RING_SIZE could be a different story.

It still crashes with that patch. The lockups are definitely getting
rarer: I have to load the thing for several hours to see a single crash
now (though sometimes I am still (un)lucky and it dies almost at once).

> Did you keep netconsole disabled and did you increse via-rhine verbosity
> level ?

Oops! Netconsole *was* on: I've been using it for so long I'd forgotten
that you pretty much have to whap it with a hammer and turn it off in
.config to turn it off completely, not just stop mentioning it on the
kernel cmdline. It's off now.

The verbosity level is now 16, which should be enough to cover, well,
everything, and indeed I see extra log at initialization time:

[    0.911369] via_rhine: v1.10-LK1.5.1 2010-10-09 Written by Donald Becker
[    0.921653] via-rhine 0000:00:06.0 (unnamed net_device) (uninitialized): Reset succeeded
[    0.936067] via-rhine 0000:00:06.0 eth0: VIA Rhine III (Management Adapter) at 0xe0806000, 00:00:24:cb:c6:a0, IRQ 11
[    0.949911] via-rhine 0000:00:06.0 eth0: MII PHY found at address 1, status 0x786d advertising 05e1 Link cde1
[    0.969400] via-rhine 0000:00:07.0 (unnamed net_device) (uninitialized): Reset succeeded
[    0.983852] via-rhine 0000:00:07.0 eth1: VIA Rhine III (Management Adapter) at 0xe0808100, 00:00:24:cb:c6:a1, IRQ 5
[    0.997168] via-rhine 0000:00:07.0 eth1: MII PHY found at address 1, status 0x786d advertising 05e1 Link 41e1
[    1.006638] via-rhine 0000:00:08.0 (unnamed net_device) (uninitialized): Reset succeeded
[    1.021091] via-rhine 0000:00:08.0 eth2: VIA Rhine III (Management Adapter) at 0xe080a200, 00:00:24:cb:c6:a2, IRQ 9
[    1.034402] via-rhine 0000:00:08.0 eth2: MII PHY found at address 1, status 0x786d advertising 05e1 Link 41e1
[    1.043872] via-rhine 0000:00:09.0 (unnamed net_device) (uninitialized): Reset succeeded
[    1.058294] via-rhine 0000:00:09.0 eth3: VIA Rhine III (Management Adapter) at 0xe080c300, 00:00:24:cb:c6:a3, IRQ 12
[    1.062104] via-rhine 0000:00:09.0 eth3: MII PHY found at address 1, status 0x786d advertising 05e1 Link 4de1
[    1.071608] via-rhine 0000:01:00.0 (unnamed net_device) (uninitialized): Reset succeeded
[    1.086073] via-rhine 0000:01:00.0 eth4: VIA Rhine III (Management Adapter) at 0xe080e000, 00:00:24:d1:2a:3c, IRQ 10
[    1.099911] via-rhine 0000:01:00.0 eth4: MII PHY found at address 1, status 0x786d advertising 05e1 Link 41e1
[    1.119402] via-rhine 0000:01:01.0 (unnamed net_device) (uninitialized): Reset succeeded
[    1.133915] via-rhine 0000:01:01.0 eth5: VIA Rhine III (Management Adapter) at 0xe0810100, 00:00:24:d1:2a:3d, IRQ 7
[    1.147234] via-rhine 0000:01:01.0 eth5: MII PHY found at address 1, status 0x7849 advertising 05e1 Link 0000
[    1.156747] via-rhine 0000:01:02.0 (unnamed net_device) (uninitialized): Reset succeeded
[    1.171262] via-rhine 0000:01:02.0 eth6: VIA Rhine III (Management Adapter) at 0xe0812200, 00:00:24:d1:2a:3e, IRQ 10
[    1.185095] via-rhine 0000:01:02.0 eth6: MII PHY found at address 1, status 0x7849 advertising 05e1 Link 0000
[    1.194599] via-rhine 0000:01:03.0 (unnamed net_device) (uninitialized): Reset succeeded
[    1.209094] via-rhine 0000:01:03.0 eth7: VIA Rhine III (Management Adapter) at 0xe0814300, 00:00:24:d1:2a:3f, IRQ 7
[    1.212436] via-rhine 0000:01:03.0 eth7: MII PHY found at address 1, status 0x7849 advertising 05e1 Link 0000
[...]
[   17.264820] via-rhine 0000:00:06.0 gordianet: Reset succeeded
[   17.299978] via-rhine 0000:00:06.0 gordianet: link up, 100Mbps, full-duplex, lpa 0xCDE1
[   17.347962] via-rhine 0000:00:06.0 gordianet: force_media 0, carrier 1
[   18.221924] via-rhine 0000:00:09.0 wireless: Reset succeeded
[   18.256936] via-rhine 0000:00:09.0 wireless: link up, 100Mbps, full-duplex, lpa 0x4DE1
[   18.304399] via-rhine 0000:00:09.0 wireless: force_media 0, carrier 1
[   18.360046] via-rhine 0000:01:00.0 voip: Reset succeeded
[   18.397168] via-rhine 0000:01:00.0 voip: link up, 100Mbps, full-duplex, lpa 0x41E1
[   18.442578] via-rhine 0000:01:00.0 voip: force_media 0, carrier 1
[   18.510970] via-rhine 0000:00:07.0 adsl: Reset succeeded
[   18.546141] via-rhine 0000:00:07.0 adsl: link up, 100Mbps, full-duplex, lpa 0x41E1
[   18.591511] via-rhine 0000:00:07.0 adsl: force_media 0, carrier 1
[   18.639051] via-rhine 0000:00:08.0 bdsl: Reset succeeded
[   18.671983] via-rhine 0000:00:08.0 bdsl: link up, 100Mbps, full-duplex, lpa 0x41E1
[   18.717363] via-rhine 0000:00:08.0 bdsl: force_media 0, carrier 1

(again, the first two interfaces, gordianet and wireless, are the ones
being stressed by this test.)

Of course now I've done this it's not crashing! Maybe it's netconsole-
related on top of everything else, or I'm just being unlucky... I'll
keep trying.

-- 
NULL && (void)

Re: [PATCH RFT net-next #2 0/6] via-rhine receive buffers rework

[Francois Romieu]

Nix <nix@esperi.org.uk> :
[...]
> The verbosity level is now 16, which should be enough to cover, well,
> everything, and indeed I see extra log at initialization time:

16 ? _warn, _err and _info ought ti be good enough but you should also
check the message level with ethtool as well. You don't want debug
messages from the rx poll handler.

[...]
> Of course now I've done this it's not crashing! Maybe it's netconsole-
> related on top of everything else, or I'm just being unlucky... I'll
> keep trying.

Please no netconsole yet.

-- 
Ueimor

Re: [PATCH RFT net-next #2 0/6] via-rhine receive buffers rework

[Nix]

On 9 Apr 2015, Francois Romieu told this:

> Nix <nix@esperi.org.uk> :
> [...]
>> Of course now I've done this it's not crashing! Maybe it's netconsole-
>> related on top of everything else, or I'm just being unlucky... I'll
>> keep trying.
>
> Please no netconsole yet.

Still trying. It hasn't crashed since I turned netconsole off despite a
couple of days of loading, so it's quite possible that this crash is
netconsole-related. (Without the receive buffers rework, the crash is
much faster and happens even without netconsole, so the rework is
clearly necessary and definitely improves things dramatically.)

-- 
NULL && (void)