[PATCH net-next v2 0/2] net: stmmac: Coalesce and tail addr fixes

[PATCH net-next v2 0/2] net: stmmac: Coalesce and tail addr fixes

[Jose Abreu]

The fix for coalesce timer and a fix in tail address setting that impacts
XGMAC2 operation.

Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Joao Pinto <jpinto@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>

Jose Abreu (2):
  net: stmmac: Rework coalesce timer and fix multi-queue races
  net: stmmac: Fixup the tail addr setting in xmit path

 drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   6 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 212 ++++++++++++++--------
 3 files changed, 138 insertions(+), 84 deletions(-)

-- 
2.7.4

[PATCH net-next v2 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Jose Abreu]

This follows David Miller advice and tries to fix coalesce timer in
multi-queue scenarios.

We are now using per-queue coalesce values and per-queue TX timer.

Coalesce timer default values was changed to 1ms and the coalesce frames
to 25.

Tested in B2B setup between XGMAC2 and GMAC5.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Joao Pinto <jpinto@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
---
Jerome,

Can you please test if this one is okay ?

Thanks and Best Regards,
Jose Miguel Abreu
---
 drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   6 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 207 ++++++++++++++--------
 3 files changed, 135 insertions(+), 82 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 1854f270ad66..b1b305f8f414 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -258,10 +258,10 @@ struct stmmac_safety_stats {
 #define MAX_DMA_RIWT		0xff
 #define MIN_DMA_RIWT		0x20
 /* Tx coalesce parameters */
-#define STMMAC_COAL_TX_TIMER	40000
+#define STMMAC_COAL_TX_TIMER	1000
 #define STMMAC_MAX_COAL_TX_TICK	100000
 #define STMMAC_TX_MAX_FRAMES	256
-#define STMMAC_TX_FRAMES	64
+#define STMMAC_TX_FRAMES	25
 
 /* Packets types */
 enum packets_types {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index c0a855b7ab3b..957030cfb833 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -48,6 +48,9 @@ struct stmmac_tx_info {
 
 /* Frequently used values are kept adjacent for cache effect */
 struct stmmac_tx_queue {
+	u32 tx_count_frames;
+	int tx_timer_active;
+	struct timer_list txtimer;
 	u32 queue_index;
 	struct stmmac_priv *priv_data;
 	struct dma_extended_desc *dma_etx ____cacheline_aligned_in_smp;
@@ -59,6 +62,7 @@ struct stmmac_tx_queue {
 	dma_addr_t dma_tx_phy;
 	u32 tx_tail_addr;
 	u32 mss;
+	struct napi_struct napi ____cacheline_aligned_in_smp;
 };
 
 struct stmmac_rx_queue {
@@ -109,14 +113,12 @@ struct stmmac_pps_cfg {
 
 struct stmmac_priv {
 	/* Frequently used values are kept adjacent for cache effect */
-	u32 tx_count_frames;
 	u32 tx_coal_frames;
 	u32 tx_coal_timer;
 
 	int tx_coalesce;
 	int hwts_tx_en;
 	bool tx_path_in_lpi_mode;
-	struct timer_list txtimer;
 	bool tso;
 
 	unsigned int dma_buf_sz;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9f458bb16f2a..9809c2b319fe 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -148,6 +148,7 @@ static void stmmac_verify_args(void)
 static void stmmac_disable_all_queues(struct stmmac_priv *priv)
 {
 	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
+	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
 	u32 queue;
 
 	for (queue = 0; queue < rx_queues_cnt; queue++) {
@@ -155,6 +156,12 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
 
 		napi_disable(&rx_q->napi);
 	}
+
+	for (queue = 0; queue < tx_queues_cnt; queue++) {
+		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
+
+		napi_disable(&tx_q->napi);
+	}
 }
 
 /**
@@ -164,6 +171,7 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
 static void stmmac_enable_all_queues(struct stmmac_priv *priv)
 {
 	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
+	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
 	u32 queue;
 
 	for (queue = 0; queue < rx_queues_cnt; queue++) {
@@ -171,6 +179,12 @@ static void stmmac_enable_all_queues(struct stmmac_priv *priv)
 
 		napi_enable(&rx_q->napi);
 	}
+
+	for (queue = 0; queue < tx_queues_cnt; queue++) {
+		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
+
+		napi_enable(&tx_q->napi);
+	}
 }
 
 /**
@@ -1843,7 +1857,8 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
  * @queue: TX queue index
  * Description: it reclaims the transmit resources after transmission completes.
  */
-static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
+static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue,
+			   bool *more)
 {
 	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
 	unsigned int bytes_compl = 0, pkts_compl = 0;
@@ -1851,10 +1866,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
 
 	netif_tx_lock(priv->dev);
 
+	if (more)
+		*more = false;
+
 	priv->xstats.tx_clean++;
 
 	entry = tx_q->dirty_tx;
-	while (entry != tx_q->cur_tx) {
+	while ((entry != tx_q->cur_tx) && (pkts_compl < limit)) {
 		struct sk_buff *skb = tx_q->tx_skbuff[entry];
 		struct dma_desc *p;
 		int status;
@@ -1937,7 +1955,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
 		stmmac_enable_eee_mode(priv);
 		mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(eee_timer));
 	}
+
+	if (more && (tx_q->dirty_tx != tx_q->cur_tx))
+		*more = true;
+
 	netif_tx_unlock(priv->dev);
+
+	return pkts_compl;
 }
 
 /**
@@ -2020,6 +2044,34 @@ static bool stmmac_safety_feat_interrupt(struct stmmac_priv *priv)
 	return false;
 }
 
+static int stmmac_napi_check(struct stmmac_priv *priv, u32 chan)
+{
+	int status = stmmac_dma_interrupt_status(priv, priv->ioaddr,
+						 &priv->xstats, chan);
+
+	if ((status & handle_rx) && (chan < priv->plat->rx_queues_to_use)) {
+		struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
+
+		if (likely(napi_schedule_prep(&rx_q->napi))) {
+			stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
+			__napi_schedule(&rx_q->napi);
+		}
+	} else {
+		status &= ~handle_rx;
+	}
+
+	if ((status & handle_tx) && (chan < priv->plat->tx_queues_to_use)) {
+		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
+
+		if (likely(napi_schedule_prep(&tx_q->napi)))
+			__napi_schedule(&tx_q->napi);
+	} else {
+		status &= ~handle_tx;
+	}
+
+	return status;
+}
+
 /**
  * stmmac_dma_interrupt - DMA ISR
  * @priv: driver private structure
@@ -2034,57 +2086,14 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
 	u32 channels_to_check = tx_channel_count > rx_channel_count ?
 				tx_channel_count : rx_channel_count;
 	u32 chan;
-	bool poll_scheduled = false;
 	int status[max_t(u32, MTL_MAX_TX_QUEUES, MTL_MAX_RX_QUEUES)];
 
 	/* Make sure we never check beyond our status buffer. */
 	if (WARN_ON_ONCE(channels_to_check > ARRAY_SIZE(status)))
 		channels_to_check = ARRAY_SIZE(status);
 
-	/* Each DMA channel can be used for rx and tx simultaneously, yet
-	 * napi_struct is embedded in struct stmmac_rx_queue rather than in a
-	 * stmmac_channel struct.
-	 * Because of this, stmmac_poll currently checks (and possibly wakes)
-	 * all tx queues rather than just a single tx queue.
-	 */
 	for (chan = 0; chan < channels_to_check; chan++)
-		status[chan] = stmmac_dma_interrupt_status(priv, priv->ioaddr,
-				&priv->xstats, chan);
-
-	for (chan = 0; chan < rx_channel_count; chan++) {
-		if (likely(status[chan] & handle_rx)) {
-			struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
-
-			if (likely(napi_schedule_prep(&rx_q->napi))) {
-				stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
-				__napi_schedule(&rx_q->napi);
-				poll_scheduled = true;
-			}
-		}
-	}
-
-	/* If we scheduled poll, we already know that tx queues will be checked.
-	 * If we didn't schedule poll, see if any DMA channel (used by tx) has a
-	 * completed transmission, if so, call stmmac_poll (once).
-	 */
-	if (!poll_scheduled) {
-		for (chan = 0; chan < tx_channel_count; chan++) {
-			if (status[chan] & handle_tx) {
-				/* It doesn't matter what rx queue we choose
-				 * here. We use 0 since it always exists.
-				 */
-				struct stmmac_rx_queue *rx_q =
-					&priv->rx_queue[0];
-
-				if (likely(napi_schedule_prep(&rx_q->napi))) {
-					stmmac_disable_dma_irq(priv,
-							priv->ioaddr, chan);
-					__napi_schedule(&rx_q->napi);
-				}
-				break;
-			}
-		}
-	}
+		status[chan] = stmmac_napi_check(priv, chan);
 
 	for (chan = 0; chan < tx_channel_count; chan++) {
 		if (unlikely(status[chan] & tx_hard_error_bump_tc)) {
@@ -2241,13 +2250,11 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
  */
 static void stmmac_tx_timer(struct timer_list *t)
 {
-	struct stmmac_priv *priv = from_timer(priv, t, txtimer);
-	u32 tx_queues_count = priv->plat->tx_queues_to_use;
-	u32 queue;
+	struct stmmac_tx_queue *tx_q = from_timer(tx_q, t, txtimer);
 
-	/* let's scan all the tx queues */
-	for (queue = 0; queue < tx_queues_count; queue++)
-		stmmac_tx_clean(priv, queue);
+	if (likely(napi_schedule_prep(&tx_q->napi)))
+		__napi_schedule(&tx_q->napi);
+	tx_q->tx_timer_active = 0;
 }
 
 /**
@@ -2260,11 +2267,17 @@ static void stmmac_tx_timer(struct timer_list *t)
  */
 static void stmmac_init_tx_coalesce(struct stmmac_priv *priv)
 {
+	u32 tx_channel_count = priv->plat->tx_queues_to_use;
+	u32 chan;
+
 	priv->tx_coal_frames = STMMAC_TX_FRAMES;
 	priv->tx_coal_timer = STMMAC_COAL_TX_TIMER;
-	timer_setup(&priv->txtimer, stmmac_tx_timer, 0);
-	priv->txtimer.expires = STMMAC_COAL_TIMER(priv->tx_coal_timer);
-	add_timer(&priv->txtimer);
+
+	for (chan = 0; chan < tx_channel_count; chan++) {
+		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
+
+		timer_setup(&tx_q->txtimer, stmmac_tx_timer, 0);
+	}
 }
 
 static void stmmac_set_rings_length(struct stmmac_priv *priv)
@@ -2592,6 +2605,7 @@ static void stmmac_hw_teardown(struct net_device *dev)
 static int stmmac_open(struct net_device *dev)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
+	u32 chan;
 	int ret;
 
 	stmmac_check_ether_addr(priv);
@@ -2688,7 +2702,9 @@ static int stmmac_open(struct net_device *dev)
 	if (dev->phydev)
 		phy_stop(dev->phydev);
 
-	del_timer_sync(&priv->txtimer);
+	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
+		del_timer_sync(&priv->tx_queue[chan].txtimer);
+
 	stmmac_hw_teardown(dev);
 init_error:
 	free_dma_desc_resources(priv);
@@ -2708,6 +2724,7 @@ static int stmmac_open(struct net_device *dev)
 static int stmmac_release(struct net_device *dev)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
+	u32 chan;
 
 	if (priv->eee_enabled)
 		del_timer_sync(&priv->eee_ctrl_timer);
@@ -2722,7 +2739,8 @@ static int stmmac_release(struct net_device *dev)
 
 	stmmac_disable_all_queues(priv);
 
-	del_timer_sync(&priv->txtimer);
+	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
+		del_timer_sync(&priv->tx_queue[chan].txtimer);
 
 	/* Free the IRQ lines */
 	free_irq(dev->irq, dev);
@@ -2936,14 +2954,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	priv->xstats.tx_tso_nfrags += nfrags;
 
 	/* Manage tx mitigation */
-	priv->tx_count_frames += nfrags + 1;
-	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
-		mod_timer(&priv->txtimer,
-			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
-	} else {
-		priv->tx_count_frames = 0;
+	tx_q->tx_count_frames += nfrags + 1;
+	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
 		stmmac_set_tx_ic(priv, desc);
 		priv->xstats.tx_set_ic_bit++;
+		tx_q->tx_count_frames = 0;
 	}
 
 	skb_tx_timestamp(skb);
@@ -2994,6 +3009,12 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
 
+	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
+		tx_q->tx_timer_active = 1;
+		mod_timer(&tx_q->txtimer,
+				STMMAC_COAL_TIMER(priv->tx_coal_timer));
+	}
+
 	return NETDEV_TX_OK;
 
 dma_map_err:
@@ -3146,14 +3167,11 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	 * This approach takes care about the fragments: desc is the first
 	 * element in case of no SG.
 	 */
-	priv->tx_count_frames += nfrags + 1;
-	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
-		mod_timer(&priv->txtimer,
-			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
-	} else {
-		priv->tx_count_frames = 0;
+	tx_q->tx_count_frames += nfrags + 1;
+	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
 		stmmac_set_tx_ic(priv, desc);
 		priv->xstats.tx_set_ic_bit++;
+		tx_q->tx_count_frames = 0;
 	}
 
 	skb_tx_timestamp(skb);
@@ -3199,8 +3217,15 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	netdev_tx_sent_queue(netdev_get_tx_queue(dev, queue), skb->len);
 
 	stmmac_enable_dma_transmission(priv, priv->ioaddr);
+
 	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
 
+	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
+		tx_q->tx_timer_active = 1;
+		mod_timer(&tx_q->txtimer,
+				STMMAC_COAL_TIMER(priv->tx_coal_timer));
+	}
+
 	return NETDEV_TX_OK;
 
 dma_map_err:
@@ -3514,27 +3539,41 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
  *  Description :
  *  To look at the incoming frames and clear the tx resources.
  */
-static int stmmac_poll(struct napi_struct *napi, int budget)
+static int stmmac_rx_poll(struct napi_struct *napi, int budget)
 {
 	struct stmmac_rx_queue *rx_q =
 		container_of(napi, struct stmmac_rx_queue, napi);
 	struct stmmac_priv *priv = rx_q->priv_data;
-	u32 tx_count = priv->plat->tx_queues_to_use;
 	u32 chan = rx_q->queue_index;
 	int work_done = 0;
-	u32 queue;
 
 	priv->xstats.napi_poll++;
 
-	/* check all the queues */
-	for (queue = 0; queue < tx_count; queue++)
-		stmmac_tx_clean(priv, queue);
-
 	work_done = stmmac_rx(priv, budget, rx_q->queue_index);
+	if (work_done < budget && napi_complete_done(napi, work_done))
+		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
+
+	return work_done;
+}
+
+static int stmmac_tx_poll(struct napi_struct *napi, int budget)
+{
+	struct stmmac_tx_queue *tx_q =
+		container_of(napi, struct stmmac_tx_queue, napi);
+	struct stmmac_priv *priv = tx_q->priv_data;
+	u32 chan = tx_q->queue_index;
+	int work_done = 0;
+	bool more;
+
+	priv->xstats.napi_poll++;
+
+	work_done = stmmac_tx_clean(priv, budget, chan, &more);
 	if (work_done < budget) {
 		napi_complete_done(napi, work_done);
-		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
+		if (more)
+			napi_reschedule(napi);
 	}
+
 	return work_done;
 }
 
@@ -4325,10 +4364,17 @@ int stmmac_dvr_probe(struct device *device,
 	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
 		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
 
-		netif_napi_add(ndev, &rx_q->napi, stmmac_poll,
+		netif_napi_add(ndev, &rx_q->napi, stmmac_rx_poll,
 			       (8 * priv->plat->rx_queues_to_use));
 	}
 
+	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
+		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
+
+		netif_napi_add(ndev, &tx_q->napi, stmmac_tx_poll,
+			       (8 * priv->plat->tx_queues_to_use));
+	}
+
 	mutex_init(&priv->lock);
 
 	/* If a specific clk_csr value is passed from the platform
@@ -4377,6 +4423,11 @@ int stmmac_dvr_probe(struct device *device,
 
 		netif_napi_del(&rx_q->napi);
 	}
+	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
+		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
+
+		netif_napi_del(&tx_q->napi);
+	}
 error_hw_init:
 	destroy_workqueue(priv->wq);
 error_wq:
-- 
2.7.4

Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Neil Armstrong]

Hi Jose,

On 10/09/2018 11:14, Jose Abreu wrote:
> This follows David Miller advice and tries to fix coalesce timer in
> multi-queue scenarios.
> 
> We are now using per-queue coalesce values and per-queue TX timer.
> 
> Coalesce timer default values was changed to 1ms and the coalesce frames
> to 25.
> 
> Tested in B2B setup between XGMAC2 and GMAC5.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> ---
> Jerome,

Jerome is in holidays...

> 
> Can you please test if this one is okay ?


I tested this patch on top of 4.18.7 with the previous patch (4ae0169fd1b3) reverted.

The TX or RX stopped a few seconds after iperf3 started :
(iperf3 is running in server mode on the Amlogic A113D board)

$ iperf3 -c 10.1.4.95
Connecting to host 10.1.4.95, port 5201
[  4] local 10.1.2.12 port 39906 connected to 10.1.4.95 port 5201
[ ID] Interval           Transfer     Bandwidth       Retr  Cwnd
[  4]   0.00-1.00   sec  56.2 MBytes   472 Mbits/sec    0    660 KBytes
[  4]   1.00-2.00   sec  56.2 MBytes   472 Mbits/sec    0    660 KBytes
[  4]   2.00-3.00   sec  38.8 MBytes   325 Mbits/sec    1   1.41 KBytes
[  4]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
[  4]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
^C[  4]   5.00-5.61   sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bandwidth       Retr
[  4]   0.00-5.61   sec   151 MBytes   226 Mbits/sec    3             sender
[  4]   0.00-5.61   sec  0.00 Bytes  0.00 bits/sec                  receiver
iperf3: interrupt - the client has terminated

$ iperf3 -c 10.1.4.95 -R
Connecting to host 10.1.4.95, port 5201
Reverse mode, remote host 10.1.4.95 is sending
[  4] local 10.1.2.12 port 39894 connected to 10.1.4.95 port 5201
[ ID] Interval           Transfer     Bandwidth
[  4]   0.00-1.00   sec  82.2 MBytes   690 Mbits/sec
[  4]   1.00-2.00   sec  82.6 MBytes   693 Mbits/sec
[  4]   2.00-3.00   sec  24.2 MBytes   203 Mbits/sec
[  4]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec
[  4]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec
^C[  4]   5.00-5.53   sec  0.00 Bytes  0.00 bits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bandwidth
[  4]   0.00-5.53   sec  0.00 Bytes  0.00 bits/sec                  sender
[  4]   0.00-5.53   sec   189 MBytes   287 Mbits/sec                  receiver
iperf3: interrupt - the client has terminated

These works for hours without this patch applied.

Neil

> 
> Thanks and Best Regards,
> Jose Miguel Abreu
> ---
>  drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   6 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 207 ++++++++++++++--------
>  3 files changed, 135 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> index 1854f270ad66..b1b305f8f414 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -258,10 +258,10 @@ struct stmmac_safety_stats {
>  #define MAX_DMA_RIWT		0xff
>  #define MIN_DMA_RIWT		0x20
>  /* Tx coalesce parameters */
> -#define STMMAC_COAL_TX_TIMER	40000
> +#define STMMAC_COAL_TX_TIMER	1000
>  #define STMMAC_MAX_COAL_TX_TICK	100000
>  #define STMMAC_TX_MAX_FRAMES	256
> -#define STMMAC_TX_FRAMES	64
> +#define STMMAC_TX_FRAMES	25
>  
>  /* Packets types */
>  enum packets_types {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index c0a855b7ab3b..957030cfb833 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -48,6 +48,9 @@ struct stmmac_tx_info {
>  
>  /* Frequently used values are kept adjacent for cache effect */
>  struct stmmac_tx_queue {
> +	u32 tx_count_frames;
> +	int tx_timer_active;
> +	struct timer_list txtimer;
>  	u32 queue_index;
>  	struct stmmac_priv *priv_data;
>  	struct dma_extended_desc *dma_etx ____cacheline_aligned_in_smp;
> @@ -59,6 +62,7 @@ struct stmmac_tx_queue {
>  	dma_addr_t dma_tx_phy;
>  	u32 tx_tail_addr;
>  	u32 mss;
> +	struct napi_struct napi ____cacheline_aligned_in_smp;
>  };
>  
>  struct stmmac_rx_queue {
> @@ -109,14 +113,12 @@ struct stmmac_pps_cfg {
>  
>  struct stmmac_priv {
>  	/* Frequently used values are kept adjacent for cache effect */
> -	u32 tx_count_frames;
>  	u32 tx_coal_frames;
>  	u32 tx_coal_timer;
>  
>  	int tx_coalesce;
>  	int hwts_tx_en;
>  	bool tx_path_in_lpi_mode;
> -	struct timer_list txtimer;
>  	bool tso;
>  
>  	unsigned int dma_buf_sz;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9f458bb16f2a..9809c2b319fe 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -148,6 +148,7 @@ static void stmmac_verify_args(void)
>  static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>  {
>  	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>  	u32 queue;
>  
>  	for (queue = 0; queue < rx_queues_cnt; queue++) {
> @@ -155,6 +156,12 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>  
>  		napi_disable(&rx_q->napi);
>  	}
> +
> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		napi_disable(&tx_q->napi);
> +	}
>  }
>  
>  /**
> @@ -164,6 +171,7 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>  static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>  {
>  	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>  	u32 queue;
>  
>  	for (queue = 0; queue < rx_queues_cnt; queue++) {
> @@ -171,6 +179,12 @@ static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>  
>  		napi_enable(&rx_q->napi);
>  	}
> +
> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		napi_enable(&tx_q->napi);
> +	}
>  }
>  
>  /**
> @@ -1843,7 +1857,8 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
>   * @queue: TX queue index
>   * Description: it reclaims the transmit resources after transmission completes.
>   */
> -static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
> +static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue,
> +			   bool *more)
>  {
>  	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>  	unsigned int bytes_compl = 0, pkts_compl = 0;
> @@ -1851,10 +1866,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>  
>  	netif_tx_lock(priv->dev);
>  
> +	if (more)
> +		*more = false;
> +
>  	priv->xstats.tx_clean++;
>  
>  	entry = tx_q->dirty_tx;
> -	while (entry != tx_q->cur_tx) {
> +	while ((entry != tx_q->cur_tx) && (pkts_compl < limit)) {
>  		struct sk_buff *skb = tx_q->tx_skbuff[entry];
>  		struct dma_desc *p;
>  		int status;
> @@ -1937,7 +1955,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>  		stmmac_enable_eee_mode(priv);
>  		mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(eee_timer));
>  	}
> +
> +	if (more && (tx_q->dirty_tx != tx_q->cur_tx))
> +		*more = true;
> +
>  	netif_tx_unlock(priv->dev);
> +
> +	return pkts_compl;
>  }
>  
>  /**
> @@ -2020,6 +2044,34 @@ static bool stmmac_safety_feat_interrupt(struct stmmac_priv *priv)
>  	return false;
>  }
>  
> +static int stmmac_napi_check(struct stmmac_priv *priv, u32 chan)
> +{
> +	int status = stmmac_dma_interrupt_status(priv, priv->ioaddr,
> +						 &priv->xstats, chan);
> +
> +	if ((status & handle_rx) && (chan < priv->plat->rx_queues_to_use)) {
> +		struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
> +
> +		if (likely(napi_schedule_prep(&rx_q->napi))) {
> +			stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
> +			__napi_schedule(&rx_q->napi);
> +		}
> +	} else {
> +		status &= ~handle_rx;
> +	}
> +
> +	if ((status & handle_tx) && (chan < priv->plat->tx_queues_to_use)) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
> +
> +		if (likely(napi_schedule_prep(&tx_q->napi)))
> +			__napi_schedule(&tx_q->napi);
> +	} else {
> +		status &= ~handle_tx;
> +	}
> +
> +	return status;
> +}
> +
>  /**
>   * stmmac_dma_interrupt - DMA ISR
>   * @priv: driver private structure
> @@ -2034,57 +2086,14 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
>  	u32 channels_to_check = tx_channel_count > rx_channel_count ?
>  				tx_channel_count : rx_channel_count;
>  	u32 chan;
> -	bool poll_scheduled = false;
>  	int status[max_t(u32, MTL_MAX_TX_QUEUES, MTL_MAX_RX_QUEUES)];
>  
>  	/* Make sure we never check beyond our status buffer. */
>  	if (WARN_ON_ONCE(channels_to_check > ARRAY_SIZE(status)))
>  		channels_to_check = ARRAY_SIZE(status);
>  
> -	/* Each DMA channel can be used for rx and tx simultaneously, yet
> -	 * napi_struct is embedded in struct stmmac_rx_queue rather than in a
> -	 * stmmac_channel struct.
> -	 * Because of this, stmmac_poll currently checks (and possibly wakes)
> -	 * all tx queues rather than just a single tx queue.
> -	 */
>  	for (chan = 0; chan < channels_to_check; chan++)
> -		status[chan] = stmmac_dma_interrupt_status(priv, priv->ioaddr,
> -				&priv->xstats, chan);
> -
> -	for (chan = 0; chan < rx_channel_count; chan++) {
> -		if (likely(status[chan] & handle_rx)) {
> -			struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
> -
> -			if (likely(napi_schedule_prep(&rx_q->napi))) {
> -				stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
> -				__napi_schedule(&rx_q->napi);
> -				poll_scheduled = true;
> -			}
> -		}
> -	}
> -
> -	/* If we scheduled poll, we already know that tx queues will be checked.
> -	 * If we didn't schedule poll, see if any DMA channel (used by tx) has a
> -	 * completed transmission, if so, call stmmac_poll (once).
> -	 */
> -	if (!poll_scheduled) {
> -		for (chan = 0; chan < tx_channel_count; chan++) {
> -			if (status[chan] & handle_tx) {
> -				/* It doesn't matter what rx queue we choose
> -				 * here. We use 0 since it always exists.
> -				 */
> -				struct stmmac_rx_queue *rx_q =
> -					&priv->rx_queue[0];
> -
> -				if (likely(napi_schedule_prep(&rx_q->napi))) {
> -					stmmac_disable_dma_irq(priv,
> -							priv->ioaddr, chan);
> -					__napi_schedule(&rx_q->napi);
> -				}
> -				break;
> -			}
> -		}
> -	}
> +		status[chan] = stmmac_napi_check(priv, chan);
>  
>  	for (chan = 0; chan < tx_channel_count; chan++) {
>  		if (unlikely(status[chan] & tx_hard_error_bump_tc)) {
> @@ -2241,13 +2250,11 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
>   */
>  static void stmmac_tx_timer(struct timer_list *t)
>  {
> -	struct stmmac_priv *priv = from_timer(priv, t, txtimer);
> -	u32 tx_queues_count = priv->plat->tx_queues_to_use;
> -	u32 queue;
> +	struct stmmac_tx_queue *tx_q = from_timer(tx_q, t, txtimer);
>  
> -	/* let's scan all the tx queues */
> -	for (queue = 0; queue < tx_queues_count; queue++)
> -		stmmac_tx_clean(priv, queue);
> +	if (likely(napi_schedule_prep(&tx_q->napi)))
> +		__napi_schedule(&tx_q->napi);
> +	tx_q->tx_timer_active = 0;
>  }
>  
>  /**
> @@ -2260,11 +2267,17 @@ static void stmmac_tx_timer(struct timer_list *t)
>   */
>  static void stmmac_init_tx_coalesce(struct stmmac_priv *priv)
>  {
> +	u32 tx_channel_count = priv->plat->tx_queues_to_use;
> +	u32 chan;
> +
>  	priv->tx_coal_frames = STMMAC_TX_FRAMES;
>  	priv->tx_coal_timer = STMMAC_COAL_TX_TIMER;
> -	timer_setup(&priv->txtimer, stmmac_tx_timer, 0);
> -	priv->txtimer.expires = STMMAC_COAL_TIMER(priv->tx_coal_timer);
> -	add_timer(&priv->txtimer);
> +
> +	for (chan = 0; chan < tx_channel_count; chan++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
> +
> +		timer_setup(&tx_q->txtimer, stmmac_tx_timer, 0);
> +	}
>  }
>  
>  static void stmmac_set_rings_length(struct stmmac_priv *priv)
> @@ -2592,6 +2605,7 @@ static void stmmac_hw_teardown(struct net_device *dev)
>  static int stmmac_open(struct net_device *dev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> +	u32 chan;
>  	int ret;
>  
>  	stmmac_check_ether_addr(priv);
> @@ -2688,7 +2702,9 @@ static int stmmac_open(struct net_device *dev)
>  	if (dev->phydev)
>  		phy_stop(dev->phydev);
>  
> -	del_timer_sync(&priv->txtimer);
> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
> +
>  	stmmac_hw_teardown(dev);
>  init_error:
>  	free_dma_desc_resources(priv);
> @@ -2708,6 +2724,7 @@ static int stmmac_open(struct net_device *dev)
>  static int stmmac_release(struct net_device *dev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> +	u32 chan;
>  
>  	if (priv->eee_enabled)
>  		del_timer_sync(&priv->eee_ctrl_timer);
> @@ -2722,7 +2739,8 @@ static int stmmac_release(struct net_device *dev)
>  
>  	stmmac_disable_all_queues(priv);
>  
> -	del_timer_sync(&priv->txtimer);
> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
>  
>  	/* Free the IRQ lines */
>  	free_irq(dev->irq, dev);
> @@ -2936,14 +2954,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  	priv->xstats.tx_tso_nfrags += nfrags;
>  
>  	/* Manage tx mitigation */
> -	priv->tx_count_frames += nfrags + 1;
> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> -		mod_timer(&priv->txtimer,
> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> -	} else {
> -		priv->tx_count_frames = 0;
> +	tx_q->tx_count_frames += nfrags + 1;
> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>  		stmmac_set_tx_ic(priv, desc);
>  		priv->xstats.tx_set_ic_bit++;
> +		tx_q->tx_count_frames = 0;
>  	}
>  
>  	skb_tx_timestamp(skb);
> @@ -2994,6 +3009,12 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>  
> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
> +		tx_q->tx_timer_active = 1;
> +		mod_timer(&tx_q->txtimer,
> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
> +	}
> +
>  	return NETDEV_TX_OK;
>  
>  dma_map_err:
> @@ -3146,14 +3167,11 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	 * This approach takes care about the fragments: desc is the first
>  	 * element in case of no SG.
>  	 */
> -	priv->tx_count_frames += nfrags + 1;
> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> -		mod_timer(&priv->txtimer,
> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> -	} else {
> -		priv->tx_count_frames = 0;
> +	tx_q->tx_count_frames += nfrags + 1;
> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>  		stmmac_set_tx_ic(priv, desc);
>  		priv->xstats.tx_set_ic_bit++;
> +		tx_q->tx_count_frames = 0;
>  	}
>  
>  	skb_tx_timestamp(skb);
> @@ -3199,8 +3217,15 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	netdev_tx_sent_queue(netdev_get_tx_queue(dev, queue), skb->len);
>  
>  	stmmac_enable_dma_transmission(priv, priv->ioaddr);
> +
>  	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>  
> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
> +		tx_q->tx_timer_active = 1;
> +		mod_timer(&tx_q->txtimer,
> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
> +	}
> +
>  	return NETDEV_TX_OK;
>  
>  dma_map_err:
> @@ -3514,27 +3539,41 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>   *  Description :
>   *  To look at the incoming frames and clear the tx resources.
>   */
> -static int stmmac_poll(struct napi_struct *napi, int budget)
> +static int stmmac_rx_poll(struct napi_struct *napi, int budget)
>  {
>  	struct stmmac_rx_queue *rx_q =
>  		container_of(napi, struct stmmac_rx_queue, napi);
>  	struct stmmac_priv *priv = rx_q->priv_data;
> -	u32 tx_count = priv->plat->tx_queues_to_use;
>  	u32 chan = rx_q->queue_index;
>  	int work_done = 0;
> -	u32 queue;
>  
>  	priv->xstats.napi_poll++;
>  
> -	/* check all the queues */
> -	for (queue = 0; queue < tx_count; queue++)
> -		stmmac_tx_clean(priv, queue);
> -
>  	work_done = stmmac_rx(priv, budget, rx_q->queue_index);
> +	if (work_done < budget && napi_complete_done(napi, work_done))
> +		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
> +
> +	return work_done;
> +}
> +
> +static int stmmac_tx_poll(struct napi_struct *napi, int budget)
> +{
> +	struct stmmac_tx_queue *tx_q =
> +		container_of(napi, struct stmmac_tx_queue, napi);
> +	struct stmmac_priv *priv = tx_q->priv_data;
> +	u32 chan = tx_q->queue_index;
> +	int work_done = 0;
> +	bool more;
> +
> +	priv->xstats.napi_poll++;
> +
> +	work_done = stmmac_tx_clean(priv, budget, chan, &more);
>  	if (work_done < budget) {
>  		napi_complete_done(napi, work_done);
> -		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
> +		if (more)
> +			napi_reschedule(napi);
>  	}
> +
>  	return work_done;
>  }
>  
> @@ -4325,10 +4364,17 @@ int stmmac_dvr_probe(struct device *device,
>  	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
>  		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
>  
> -		netif_napi_add(ndev, &rx_q->napi, stmmac_poll,
> +		netif_napi_add(ndev, &rx_q->napi, stmmac_rx_poll,
>  			       (8 * priv->plat->rx_queues_to_use));
>  	}
>  
> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		netif_napi_add(ndev, &tx_q->napi, stmmac_tx_poll,
> +			       (8 * priv->plat->tx_queues_to_use));
> +	}
> +
>  	mutex_init(&priv->lock);
>  
>  	/* If a specific clk_csr value is passed from the platform
> @@ -4377,6 +4423,11 @@ int stmmac_dvr_probe(struct device *device,
>  
>  		netif_napi_del(&rx_q->napi);
>  	}
> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		netif_napi_del(&tx_q->napi);
> +	}
>  error_hw_init:
>  	destroy_workqueue(priv->wq);
>  error_wq:
> 

Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Jose Abreu]

[-- Attachment #1: Type: text/plain, Size: 19320 bytes --]

Hi Neil,

On 10-09-2018 12:43, Neil Armstrong wrote:
> Hi Jose,
>
> On 10/09/2018 11:14, Jose Abreu wrote:
>> This follows David Miller advice and tries to fix coalesce timer in
>> multi-queue scenarios.
>>
>> We are now using per-queue coalesce values and per-queue TX timer.
>>
>> Coalesce timer default values was changed to 1ms and the coalesce frames
>> to 25.
>>
>> Tested in B2B setup between XGMAC2 and GMAC5.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Jerome Brunet <jbrunet@baylibre.com>
>> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Cc: Alexandre Torgue <alexandre.torgue@st.com>
>> ---
>> Jerome,
> Jerome is in holidays...
>
>> Can you please test if this one is okay ?
>
> I tested this patch on top of 4.18.7 with the previous patch (4ae0169fd1b3) reverted.
>
> The TX or RX stopped a few seconds after iperf3 started :
> (iperf3 is running in server mode on the Amlogic A113D board)
>
> $ iperf3 -c 10.1.4.95
> Connecting to host 10.1.4.95, port 5201
> [  4] local 10.1.2.12 port 39906 connected to 10.1.4.95 port 5201
> [ ID] Interval           Transfer     Bandwidth       Retr  Cwnd
> [  4]   0.00-1.00   sec  56.2 MBytes   472 Mbits/sec    0    660 KBytes
> [  4]   1.00-2.00   sec  56.2 MBytes   472 Mbits/sec    0    660 KBytes
> [  4]   2.00-3.00   sec  38.8 MBytes   325 Mbits/sec    1   1.41 KBytes
> [  4]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
> [  4]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
> ^C[  4]   5.00-5.61   sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bandwidth       Retr
> [  4]   0.00-5.61   sec   151 MBytes   226 Mbits/sec    3             sender
> [  4]   0.00-5.61   sec  0.00 Bytes  0.00 bits/sec                  receiver
> iperf3: interrupt - the client has terminated
>
> $ iperf3 -c 10.1.4.95 -R
> Connecting to host 10.1.4.95, port 5201
> Reverse mode, remote host 10.1.4.95 is sending
> [  4] local 10.1.2.12 port 39894 connected to 10.1.4.95 port 5201
> [ ID] Interval           Transfer     Bandwidth
> [  4]   0.00-1.00   sec  82.2 MBytes   690 Mbits/sec
> [  4]   1.00-2.00   sec  82.6 MBytes   693 Mbits/sec
> [  4]   2.00-3.00   sec  24.2 MBytes   203 Mbits/sec
> [  4]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec
> [  4]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec
> ^C[  4]   5.00-5.53   sec  0.00 Bytes  0.00 bits/sec
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bandwidth
> [  4]   0.00-5.53   sec  0.00 Bytes  0.00 bits/sec                  sender
> [  4]   0.00-5.53   sec   189 MBytes   287 Mbits/sec                  receiver
> iperf3: interrupt - the client has terminated
>
> These works for hours without this patch applied.

Damn... I'm able to replicate the issue if I turn SMP off but
it's not consistent ...

Can you please try attached follow-up patch ? It's been running
for 10min now and I'm getting ~700Mbps on the same GMAC as you
have (3.71).

Thanks and Best Regards,
Jose Miguel Abreu

>
> Neil
>
>> Thanks and Best Regards,
>> Jose Miguel Abreu
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
>>  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   6 +-
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 207 ++++++++++++++--------
>>  3 files changed, 135 insertions(+), 82 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
>> index 1854f270ad66..b1b305f8f414 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
>> @@ -258,10 +258,10 @@ struct stmmac_safety_stats {
>>  #define MAX_DMA_RIWT		0xff
>>  #define MIN_DMA_RIWT		0x20
>>  /* Tx coalesce parameters */
>> -#define STMMAC_COAL_TX_TIMER	40000
>> +#define STMMAC_COAL_TX_TIMER	1000
>>  #define STMMAC_MAX_COAL_TX_TICK	100000
>>  #define STMMAC_TX_MAX_FRAMES	256
>> -#define STMMAC_TX_FRAMES	64
>> +#define STMMAC_TX_FRAMES	25
>>  
>>  /* Packets types */
>>  enum packets_types {
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> index c0a855b7ab3b..957030cfb833 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> @@ -48,6 +48,9 @@ struct stmmac_tx_info {
>>  
>>  /* Frequently used values are kept adjacent for cache effect */
>>  struct stmmac_tx_queue {
>> +	u32 tx_count_frames;
>> +	int tx_timer_active;
>> +	struct timer_list txtimer;
>>  	u32 queue_index;
>>  	struct stmmac_priv *priv_data;
>>  	struct dma_extended_desc *dma_etx ____cacheline_aligned_in_smp;
>> @@ -59,6 +62,7 @@ struct stmmac_tx_queue {
>>  	dma_addr_t dma_tx_phy;
>>  	u32 tx_tail_addr;
>>  	u32 mss;
>> +	struct napi_struct napi ____cacheline_aligned_in_smp;
>>  };
>>  
>>  struct stmmac_rx_queue {
>> @@ -109,14 +113,12 @@ struct stmmac_pps_cfg {
>>  
>>  struct stmmac_priv {
>>  	/* Frequently used values are kept adjacent for cache effect */
>> -	u32 tx_count_frames;
>>  	u32 tx_coal_frames;
>>  	u32 tx_coal_timer;
>>  
>>  	int tx_coalesce;
>>  	int hwts_tx_en;
>>  	bool tx_path_in_lpi_mode;
>> -	struct timer_list txtimer;
>>  	bool tso;
>>  
>>  	unsigned int dma_buf_sz;
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 9f458bb16f2a..9809c2b319fe 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -148,6 +148,7 @@ static void stmmac_verify_args(void)
>>  static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>>  {
>>  	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
>> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>>  	u32 queue;
>>  
>>  	for (queue = 0; queue < rx_queues_cnt; queue++) {
>> @@ -155,6 +156,12 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>>  
>>  		napi_disable(&rx_q->napi);
>>  	}
>> +
>> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		napi_disable(&tx_q->napi);
>> +	}
>>  }
>>  
>>  /**
>> @@ -164,6 +171,7 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>>  static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>>  {
>>  	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
>> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>>  	u32 queue;
>>  
>>  	for (queue = 0; queue < rx_queues_cnt; queue++) {
>> @@ -171,6 +179,12 @@ static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>>  
>>  		napi_enable(&rx_q->napi);
>>  	}
>> +
>> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		napi_enable(&tx_q->napi);
>> +	}
>>  }
>>  
>>  /**
>> @@ -1843,7 +1857,8 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
>>   * @queue: TX queue index
>>   * Description: it reclaims the transmit resources after transmission completes.
>>   */
>> -static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>> +static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue,
>> +			   bool *more)
>>  {
>>  	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>>  	unsigned int bytes_compl = 0, pkts_compl = 0;
>> @@ -1851,10 +1866,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>>  
>>  	netif_tx_lock(priv->dev);
>>  
>> +	if (more)
>> +		*more = false;
>> +
>>  	priv->xstats.tx_clean++;
>>  
>>  	entry = tx_q->dirty_tx;
>> -	while (entry != tx_q->cur_tx) {
>> +	while ((entry != tx_q->cur_tx) && (pkts_compl < limit)) {
>>  		struct sk_buff *skb = tx_q->tx_skbuff[entry];
>>  		struct dma_desc *p;
>>  		int status;
>> @@ -1937,7 +1955,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>>  		stmmac_enable_eee_mode(priv);
>>  		mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(eee_timer));
>>  	}
>> +
>> +	if (more && (tx_q->dirty_tx != tx_q->cur_tx))
>> +		*more = true;
>> +
>>  	netif_tx_unlock(priv->dev);
>> +
>> +	return pkts_compl;
>>  }
>>  
>>  /**
>> @@ -2020,6 +2044,34 @@ static bool stmmac_safety_feat_interrupt(struct stmmac_priv *priv)
>>  	return false;
>>  }
>>  
>> +static int stmmac_napi_check(struct stmmac_priv *priv, u32 chan)
>> +{
>> +	int status = stmmac_dma_interrupt_status(priv, priv->ioaddr,
>> +						 &priv->xstats, chan);
>> +
>> +	if ((status & handle_rx) && (chan < priv->plat->rx_queues_to_use)) {
>> +		struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
>> +
>> +		if (likely(napi_schedule_prep(&rx_q->napi))) {
>> +			stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
>> +			__napi_schedule(&rx_q->napi);
>> +		}
>> +	} else {
>> +		status &= ~handle_rx;
>> +	}
>> +
>> +	if ((status & handle_tx) && (chan < priv->plat->tx_queues_to_use)) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
>> +
>> +		if (likely(napi_schedule_prep(&tx_q->napi)))
>> +			__napi_schedule(&tx_q->napi);
>> +	} else {
>> +		status &= ~handle_tx;
>> +	}
>> +
>> +	return status;
>> +}
>> +
>>  /**
>>   * stmmac_dma_interrupt - DMA ISR
>>   * @priv: driver private structure
>> @@ -2034,57 +2086,14 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
>>  	u32 channels_to_check = tx_channel_count > rx_channel_count ?
>>  				tx_channel_count : rx_channel_count;
>>  	u32 chan;
>> -	bool poll_scheduled = false;
>>  	int status[max_t(u32, MTL_MAX_TX_QUEUES, MTL_MAX_RX_QUEUES)];
>>  
>>  	/* Make sure we never check beyond our status buffer. */
>>  	if (WARN_ON_ONCE(channels_to_check > ARRAY_SIZE(status)))
>>  		channels_to_check = ARRAY_SIZE(status);
>>  
>> -	/* Each DMA channel can be used for rx and tx simultaneously, yet
>> -	 * napi_struct is embedded in struct stmmac_rx_queue rather than in a
>> -	 * stmmac_channel struct.
>> -	 * Because of this, stmmac_poll currently checks (and possibly wakes)
>> -	 * all tx queues rather than just a single tx queue.
>> -	 */
>>  	for (chan = 0; chan < channels_to_check; chan++)
>> -		status[chan] = stmmac_dma_interrupt_status(priv, priv->ioaddr,
>> -				&priv->xstats, chan);
>> -
>> -	for (chan = 0; chan < rx_channel_count; chan++) {
>> -		if (likely(status[chan] & handle_rx)) {
>> -			struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
>> -
>> -			if (likely(napi_schedule_prep(&rx_q->napi))) {
>> -				stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
>> -				__napi_schedule(&rx_q->napi);
>> -				poll_scheduled = true;
>> -			}
>> -		}
>> -	}
>> -
>> -	/* If we scheduled poll, we already know that tx queues will be checked.
>> -	 * If we didn't schedule poll, see if any DMA channel (used by tx) has a
>> -	 * completed transmission, if so, call stmmac_poll (once).
>> -	 */
>> -	if (!poll_scheduled) {
>> -		for (chan = 0; chan < tx_channel_count; chan++) {
>> -			if (status[chan] & handle_tx) {
>> -				/* It doesn't matter what rx queue we choose
>> -				 * here. We use 0 since it always exists.
>> -				 */
>> -				struct stmmac_rx_queue *rx_q =
>> -					&priv->rx_queue[0];
>> -
>> -				if (likely(napi_schedule_prep(&rx_q->napi))) {
>> -					stmmac_disable_dma_irq(priv,
>> -							priv->ioaddr, chan);
>> -					__napi_schedule(&rx_q->napi);
>> -				}
>> -				break;
>> -			}
>> -		}
>> -	}
>> +		status[chan] = stmmac_napi_check(priv, chan);
>>  
>>  	for (chan = 0; chan < tx_channel_count; chan++) {
>>  		if (unlikely(status[chan] & tx_hard_error_bump_tc)) {
>> @@ -2241,13 +2250,11 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
>>   */
>>  static void stmmac_tx_timer(struct timer_list *t)
>>  {
>> -	struct stmmac_priv *priv = from_timer(priv, t, txtimer);
>> -	u32 tx_queues_count = priv->plat->tx_queues_to_use;
>> -	u32 queue;
>> +	struct stmmac_tx_queue *tx_q = from_timer(tx_q, t, txtimer);
>>  
>> -	/* let's scan all the tx queues */
>> -	for (queue = 0; queue < tx_queues_count; queue++)
>> -		stmmac_tx_clean(priv, queue);
>> +	if (likely(napi_schedule_prep(&tx_q->napi)))
>> +		__napi_schedule(&tx_q->napi);
>> +	tx_q->tx_timer_active = 0;
>>  }
>>  
>>  /**
>> @@ -2260,11 +2267,17 @@ static void stmmac_tx_timer(struct timer_list *t)
>>   */
>>  static void stmmac_init_tx_coalesce(struct stmmac_priv *priv)
>>  {
>> +	u32 tx_channel_count = priv->plat->tx_queues_to_use;
>> +	u32 chan;
>> +
>>  	priv->tx_coal_frames = STMMAC_TX_FRAMES;
>>  	priv->tx_coal_timer = STMMAC_COAL_TX_TIMER;
>> -	timer_setup(&priv->txtimer, stmmac_tx_timer, 0);
>> -	priv->txtimer.expires = STMMAC_COAL_TIMER(priv->tx_coal_timer);
>> -	add_timer(&priv->txtimer);
>> +
>> +	for (chan = 0; chan < tx_channel_count; chan++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
>> +
>> +		timer_setup(&tx_q->txtimer, stmmac_tx_timer, 0);
>> +	}
>>  }
>>  
>>  static void stmmac_set_rings_length(struct stmmac_priv *priv)
>> @@ -2592,6 +2605,7 @@ static void stmmac_hw_teardown(struct net_device *dev)
>>  static int stmmac_open(struct net_device *dev)
>>  {
>>  	struct stmmac_priv *priv = netdev_priv(dev);
>> +	u32 chan;
>>  	int ret;
>>  
>>  	stmmac_check_ether_addr(priv);
>> @@ -2688,7 +2702,9 @@ static int stmmac_open(struct net_device *dev)
>>  	if (dev->phydev)
>>  		phy_stop(dev->phydev);
>>  
>> -	del_timer_sync(&priv->txtimer);
>> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
>> +
>>  	stmmac_hw_teardown(dev);
>>  init_error:
>>  	free_dma_desc_resources(priv);
>> @@ -2708,6 +2724,7 @@ static int stmmac_open(struct net_device *dev)
>>  static int stmmac_release(struct net_device *dev)
>>  {
>>  	struct stmmac_priv *priv = netdev_priv(dev);
>> +	u32 chan;
>>  
>>  	if (priv->eee_enabled)
>>  		del_timer_sync(&priv->eee_ctrl_timer);
>> @@ -2722,7 +2739,8 @@ static int stmmac_release(struct net_device *dev)
>>  
>>  	stmmac_disable_all_queues(priv);
>>  
>> -	del_timer_sync(&priv->txtimer);
>> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
>>  
>>  	/* Free the IRQ lines */
>>  	free_irq(dev->irq, dev);
>> @@ -2936,14 +2954,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	priv->xstats.tx_tso_nfrags += nfrags;
>>  
>>  	/* Manage tx mitigation */
>> -	priv->tx_count_frames += nfrags + 1;
>> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
>> -		mod_timer(&priv->txtimer,
>> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> -	} else {
>> -		priv->tx_count_frames = 0;
>> +	tx_q->tx_count_frames += nfrags + 1;
>> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>>  		stmmac_set_tx_ic(priv, desc);
>>  		priv->xstats.tx_set_ic_bit++;
>> +		tx_q->tx_count_frames = 0;
>>  	}
>>  
>>  	skb_tx_timestamp(skb);
>> @@ -2994,6 +3009,12 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>>  
>>  	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>>  
>> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
>> +		tx_q->tx_timer_active = 1;
>> +		mod_timer(&tx_q->txtimer,
>> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> +	}
>> +
>>  	return NETDEV_TX_OK;
>>  
>>  dma_map_err:
>> @@ -3146,14 +3167,11 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	 * This approach takes care about the fragments: desc is the first
>>  	 * element in case of no SG.
>>  	 */
>> -	priv->tx_count_frames += nfrags + 1;
>> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
>> -		mod_timer(&priv->txtimer,
>> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> -	} else {
>> -		priv->tx_count_frames = 0;
>> +	tx_q->tx_count_frames += nfrags + 1;
>> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>>  		stmmac_set_tx_ic(priv, desc);
>>  		priv->xstats.tx_set_ic_bit++;
>> +		tx_q->tx_count_frames = 0;
>>  	}
>>  
>>  	skb_tx_timestamp(skb);
>> @@ -3199,8 +3217,15 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	netdev_tx_sent_queue(netdev_get_tx_queue(dev, queue), skb->len);
>>  
>>  	stmmac_enable_dma_transmission(priv, priv->ioaddr);
>> +
>>  	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>>  
>> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
>> +		tx_q->tx_timer_active = 1;
>> +		mod_timer(&tx_q->txtimer,
>> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> +	}
>> +
>>  	return NETDEV_TX_OK;
>>  
>>  dma_map_err:
>> @@ -3514,27 +3539,41 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>>   *  Description :
>>   *  To look at the incoming frames and clear the tx resources.
>>   */
>> -static int stmmac_poll(struct napi_struct *napi, int budget)
>> +static int stmmac_rx_poll(struct napi_struct *napi, int budget)
>>  {
>>  	struct stmmac_rx_queue *rx_q =
>>  		container_of(napi, struct stmmac_rx_queue, napi);
>>  	struct stmmac_priv *priv = rx_q->priv_data;
>> -	u32 tx_count = priv->plat->tx_queues_to_use;
>>  	u32 chan = rx_q->queue_index;
>>  	int work_done = 0;
>> -	u32 queue;
>>  
>>  	priv->xstats.napi_poll++;
>>  
>> -	/* check all the queues */
>> -	for (queue = 0; queue < tx_count; queue++)
>> -		stmmac_tx_clean(priv, queue);
>> -
>>  	work_done = stmmac_rx(priv, budget, rx_q->queue_index);
>> +	if (work_done < budget && napi_complete_done(napi, work_done))
>> +		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
>> +
>> +	return work_done;
>> +}
>> +
>> +static int stmmac_tx_poll(struct napi_struct *napi, int budget)
>> +{
>> +	struct stmmac_tx_queue *tx_q =
>> +		container_of(napi, struct stmmac_tx_queue, napi);
>> +	struct stmmac_priv *priv = tx_q->priv_data;
>> +	u32 chan = tx_q->queue_index;
>> +	int work_done = 0;
>> +	bool more;
>> +
>> +	priv->xstats.napi_poll++;
>> +
>> +	work_done = stmmac_tx_clean(priv, budget, chan, &more);
>>  	if (work_done < budget) {
>>  		napi_complete_done(napi, work_done);
>> -		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
>> +		if (more)
>> +			napi_reschedule(napi);
>>  	}
>> +
>>  	return work_done;
>>  }
>>  
>> @@ -4325,10 +4364,17 @@ int stmmac_dvr_probe(struct device *device,
>>  	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
>>  		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
>>  
>> -		netif_napi_add(ndev, &rx_q->napi, stmmac_poll,
>> +		netif_napi_add(ndev, &rx_q->napi, stmmac_rx_poll,
>>  			       (8 * priv->plat->rx_queues_to_use));
>>  	}
>>  
>> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		netif_napi_add(ndev, &tx_q->napi, stmmac_tx_poll,
>> +			       (8 * priv->plat->tx_queues_to_use));
>> +	}
>> +
>>  	mutex_init(&priv->lock);
>>  
>>  	/* If a specific clk_csr value is passed from the platform
>> @@ -4377,6 +4423,11 @@ int stmmac_dvr_probe(struct device *device,
>>  
>>  		netif_napi_del(&rx_q->napi);
>>  	}
>> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		netif_napi_del(&tx_q->napi);
>> +	}
>>  error_hw_init:
>>  	destroy_workqueue(priv->wq);
>>  error_wq:
>>


[-- Attachment #2: 0001-fixup_coalesce.patch --]
[-- Type: text/x-patch, Size: 3982 bytes --]

>From 35a5e33f4edcc663511d615e61a1ea119dfc77ee Mon Sep 17 00:00:00 2001
Message-Id: <35a5e33f4edcc663511d615e61a1ea119dfc77ee.1536583587.git.joabreu@synopsys.com>
From: Jose Abreu <joabreu@synopsys.com>
Date: Mon, 10 Sep 2018 14:46:09 +0200
Subject: [PATCH] fixup_coalesce

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 39 +++++++++++++----------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 97268769186e..7875e81966fb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1872,7 +1872,7 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue,
 	priv->xstats.tx_clean++;
 
 	entry = tx_q->dirty_tx;
-	while ((entry != tx_q->cur_tx) && (pkts_compl < limit)) {
+	while (entry != tx_q->cur_tx) {
 		struct sk_buff *skb = tx_q->tx_skbuff[entry];
 		struct dma_desc *p;
 		int status;
@@ -2241,6 +2241,17 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
 	return ret;
 }
 
+static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
+{
+	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
+
+	if (tx_q->tx_timer_active)
+		return;
+
+	mod_timer(&tx_q->txtimer, STMMAC_COAL_TIMER(priv->tx_coal_timer));
+	tx_q->tx_timer_active = true;
+}
+
 /**
  * stmmac_tx_timer - mitigation sw timer for tx.
  * @data: data pointer
@@ -2250,10 +2261,10 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
 static void stmmac_tx_timer(struct timer_list *t)
 {
 	struct stmmac_tx_queue *tx_q = from_timer(tx_q, t, txtimer);
+	struct stmmac_priv *priv = tx_q->priv_data;
 
-	if (likely(napi_schedule_prep(&tx_q->napi)))
-		__napi_schedule(&tx_q->napi);
-	tx_q->tx_timer_active = 0;
+	stmmac_tx_clean(priv, ~0, tx_q->queue_index, NULL);
+	tx_q->tx_timer_active = false;
 }
 
 /**
@@ -2852,6 +2863,9 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Compute header lengths */
 	proto_hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
 
+	/* Start coalesce timer earlier in case TX Queue is stopped */
+	stmmac_tx_timer_arm(priv, queue);
+
 	/* Desc availability based on threshold should be enough safe */
 	if (unlikely(stmmac_tx_avail(priv, queue) <
 		(((skb->len - proto_hdr_len) / TSO_MAX_BUFF_SIZE + 1)))) {
@@ -3009,12 +3023,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	tx_q->tx_tail_addr = tx_q->dma_tx_phy + (tx_q->cur_tx * sizeof(*desc));
 	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
 
-	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
-		tx_q->tx_timer_active = 1;
-		mod_timer(&tx_q->txtimer,
-				STMMAC_COAL_TIMER(priv->tx_coal_timer));
-	}
-
 	return NETDEV_TX_OK;
 
 dma_map_err:
@@ -3054,6 +3062,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 			return stmmac_tso_xmit(skb, dev);
 	}
 
+	/* Start coalesce timer earlier in case TX Queue is stopped */
+	stmmac_tx_timer_arm(priv, queue);
+
 	if (unlikely(stmmac_tx_avail(priv, queue) < nfrags + 1)) {
 		if (!netif_tx_queue_stopped(netdev_get_tx_queue(dev, queue))) {
 			netif_tx_stop_queue(netdev_get_tx_queue(priv->dev,
@@ -3221,12 +3232,6 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	tx_q->tx_tail_addr = tx_q->dma_tx_phy + (tx_q->cur_tx * sizeof(*desc));
 	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
 
-	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
-		tx_q->tx_timer_active = 1;
-		mod_timer(&tx_q->txtimer,
-				STMMAC_COAL_TIMER(priv->tx_coal_timer));
-	}
-
 	return NETDEV_TX_OK;
 
 dma_map_err:
@@ -3575,7 +3580,7 @@ static int stmmac_tx_poll(struct napi_struct *napi, int budget)
 			napi_reschedule(napi);
 	}
 
-	return work_done;
+	return min(work_done, budget);
 }
 
 /**
-- 
2.7.4

Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Jose Abreu]

On 10-09-2018 13:52, Jose Abreu wrote:
>
> Can you please try attached follow-up patch ? 

Oh, please apply the whole series otherwise this will not apply
cleanly.

Thanks and Best Regards,
Jose Miguel Abreu

Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Neil Armstrong]

hi Jose,

On 10/09/2018 14:55, Jose Abreu wrote:
> On 10-09-2018 13:52, Jose Abreu wrote:
>>
>> Can you please try attached follow-up patch ? 
> 
> Oh, please apply the whole series otherwise this will not apply
> cleanly.

Indeed, it helps!

With the fixups, it fails later, around 15s instead of 3, in RX and TX.

Neil

> 
> Thanks and Best Regards,
> Jose Miguel Abreu
> 
> 

Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Jose Abreu]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

On 10-09-2018 14:46, Neil Armstrong wrote:
> hi Jose,
>
> On 10/09/2018 14:55, Jose Abreu wrote:
>> On 10-09-2018 13:52, Jose Abreu wrote:
>>> Can you please try attached follow-up patch ? 
>> Oh, please apply the whole series otherwise this will not apply
>> cleanly.
> Indeed, it helps!
>
> With the fixups, it fails later, around 15s instead of 3, in RX and TX.

Thanks for testing Neil. What if we keep rearming the timer
whilst there are pending packets ? Something like in the attach.
(applies on top of previous one).

Thanks and Best Regards,
Jose Miguel Abreu

[-- Attachment #2: 0001-fixup_coalesce_2.patch --]
[-- Type: text/x-patch, Size: 1130 bytes --]

>From 5d5a6bd882006f14c59b351f4324160115f818c0 Mon Sep 17 00:00:00 2001
Message-Id: <5d5a6bd882006f14c59b351f4324160115f818c0.1536590220.git.joabreu@synopsys.com>
From: Jose Abreu <joabreu@synopsys.com>
Date: Mon, 10 Sep 2018 16:36:37 +0200
Subject: [PATCH] fixup_coalesce_2

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7875e81966fb..76a6196b3263 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2262,9 +2262,12 @@ static void stmmac_tx_timer(struct timer_list *t)
 {
 	struct stmmac_tx_queue *tx_q = from_timer(tx_q, t, txtimer);
 	struct stmmac_priv *priv = tx_q->priv_data;
+	bool more;
 
-	stmmac_tx_clean(priv, ~0, tx_q->queue_index, NULL);
 	tx_q->tx_timer_active = false;
+	stmmac_tx_clean(priv, ~0, tx_q->queue_index, &more);
+	if (more)
+		stmmac_tx_timer_arm(priv, tx_q->queue_index);
 }
 
 /**
-- 
2.7.4

Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Neil Armstrong]

Hi Jose,

On 10/09/2018 16:44, Jose Abreu wrote:
> On 10-09-2018 14:46, Neil Armstrong wrote:
>> hi Jose,
>>
>> On 10/09/2018 14:55, Jose Abreu wrote:
>>> On 10-09-2018 13:52, Jose Abreu wrote:
>>>> Can you please try attached follow-up patch ? 
>>> Oh, please apply the whole series otherwise this will not apply
>>> cleanly.
>> Indeed, it helps!
>>
>> With the fixups, it fails later, around 15s instead of 3, in RX and TX.
> 
> Thanks for testing Neil. What if we keep rearming the timer
> whilst there are pending packets ? Something like in the attach.
> (applies on top of previous one).

It fixes RX, but TX fails after ~13s.

Neil

> 
> Thanks and Best Regards,
> Jose Miguel Abreu
> 

Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Jose Abreu]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On 10-09-2018 16:49, Neil Armstrong wrote:
> Hi Jose,
>
> On 10/09/2018 16:44, Jose Abreu wrote:
>> On 10-09-2018 14:46, Neil Armstrong wrote:
>>> hi Jose,
>>>
>>> On 10/09/2018 14:55, Jose Abreu wrote:
>>>> On 10-09-2018 13:52, Jose Abreu wrote:
>>>>> Can you please try attached follow-up patch ? 
>>>> Oh, please apply the whole series otherwise this will not apply
>>>> cleanly.
>>> Indeed, it helps!
>>>
>>> With the fixups, it fails later, around 15s instead of 3, in RX and TX.
>> Thanks for testing Neil. What if we keep rearming the timer
>> whilst there are pending packets ? Something like in the attach.
>> (applies on top of previous one).
> It fixes RX, but TX fails after ~13s.

Ok :(

Can you please try attached follow-up patch ?

I'm so sorry about this back and forth and I appreciate all your
help .

Thanks and Best Regards,
Jose Miguel Abreu


>
> Neil
>
>> Thanks and Best Regards,
>> Jose Miguel Abreu
>>


[-- Attachment #2: 0001-fixup_coalesce_3.patch --]
[-- Type: text/x-patch, Size: 3315 bytes --]

>From 4f2ba5fca6c8858cfe640f3d466fd01904c451e3 Mon Sep 17 00:00:00 2001
Message-Id: <4f2ba5fca6c8858cfe640f3d466fd01904c451e3.1536596296.git.joabreu@synopsys.com>
From: Jose Abreu <joabreu@synopsys.com>
Date: Mon, 10 Sep 2018 18:18:10 +0200
Subject: [PATCH] fixup_coalesce_3

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 ++++++-----------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 76a6196b3263..f6587ee372ab 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2245,11 +2245,7 @@ static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
 {
 	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
 
-	if (tx_q->tx_timer_active)
-		return;
-
 	mod_timer(&tx_q->txtimer, STMMAC_COAL_TIMER(priv->tx_coal_timer));
-	tx_q->tx_timer_active = true;
 }
 
 /**
@@ -2264,10 +2260,7 @@ static void stmmac_tx_timer(struct timer_list *t)
 	struct stmmac_priv *priv = tx_q->priv_data;
 	bool more;
 
-	tx_q->tx_timer_active = false;
 	stmmac_tx_clean(priv, ~0, tx_q->queue_index, &more);
-	if (more)
-		stmmac_tx_timer_arm(priv, tx_q->queue_index);
 }
 
 /**
@@ -2866,9 +2859,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Compute header lengths */
 	proto_hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
 
-	/* Start coalesce timer earlier in case TX Queue is stopped */
-	stmmac_tx_timer_arm(priv, queue);
-
 	/* Desc availability based on threshold should be enough safe */
 	if (unlikely(stmmac_tx_avail(priv, queue) <
 		(((skb->len - proto_hdr_len) / TSO_MAX_BUFF_SIZE + 1)))) {
@@ -2975,6 +2965,8 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 		stmmac_set_tx_ic(priv, desc);
 		priv->xstats.tx_set_ic_bit++;
 		tx_q->tx_count_frames = 0;
+	} else {
+		stmmac_tx_timer_arm(priv, queue);
 	}
 
 	skb_tx_timestamp(skb);
@@ -3065,9 +3057,6 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 			return stmmac_tso_xmit(skb, dev);
 	}
 
-	/* Start coalesce timer earlier in case TX Queue is stopped */
-	stmmac_tx_timer_arm(priv, queue);
-
 	if (unlikely(stmmac_tx_avail(priv, queue) < nfrags + 1)) {
 		if (!netif_tx_queue_stopped(netdev_get_tx_queue(dev, queue))) {
 			netif_tx_stop_queue(netdev_get_tx_queue(priv->dev,
@@ -3186,6 +3175,8 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 		stmmac_set_tx_ic(priv, desc);
 		priv->xstats.tx_set_ic_bit++;
 		tx_q->tx_count_frames = 0;
+	} else {
+		stmmac_tx_timer_arm(priv, queue);
 	}
 
 	skb_tx_timestamp(skb);
@@ -3572,16 +3563,12 @@ static int stmmac_tx_poll(struct napi_struct *napi, int budget)
 	struct stmmac_priv *priv = tx_q->priv_data;
 	u32 chan = tx_q->queue_index;
 	int work_done = 0;
-	bool more;
 
 	priv->xstats.napi_poll++;
 
-	work_done = stmmac_tx_clean(priv, budget, chan, &more);
-	if (work_done < budget) {
+	work_done = stmmac_tx_clean(priv, budget, chan, NULL);
+	if (work_done < budget)
 		napi_complete_done(napi, work_done);
-		if (more)
-			napi_reschedule(napi);
-	}
 
 	return min(work_done, budget);
 }
-- 
2.7.4

Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Neil Armstrong]

Hi Jose,

On 10/09/2018 18:21, Jose Abreu wrote:
> On 10-09-2018 16:49, Neil Armstrong wrote:
>> Hi Jose,
>>
>> On 10/09/2018 16:44, Jose Abreu wrote:
>>> On 10-09-2018 14:46, Neil Armstrong wrote:
>>>> hi Jose,
>>>>
>>>> On 10/09/2018 14:55, Jose Abreu wrote:
>>>>> On 10-09-2018 13:52, Jose Abreu wrote:
>>>>>> Can you please try attached follow-up patch ? 
>>>>> Oh, please apply the whole series otherwise this will not apply
>>>>> cleanly.
>>>> Indeed, it helps!
>>>>
>>>> With the fixups, it fails later, around 15s instead of 3, in RX and TX.
>>> Thanks for testing Neil. What if we keep rearming the timer
>>> whilst there are pending packets ? Something like in the attach.
>>> (applies on top of previous one).
>> It fixes RX, but TX fails after ~13s.
> 
> Ok :(
> 
> Can you please try attached follow-up patch ?

RX is still ok but now TX fails almost immediately...

With 100ms report :

$ iperf3 -c 192.168.1.47 -t 0 -p 5202 -R -i 0.1
Connecting to host 192.168.1.47, port 5202
Reverse mode, remote host 192.168.1.47 is sending
[  4] local 192.168.1.45 port 45900 connected to 192.168.1.47 port 5202
[ ID] Interval           Transfer     Bandwidth
[  4]   0.00-0.10   sec  10.9 MBytes   913 Mbits/sec
[  4]   0.10-0.20   sec  11.0 MBytes   923 Mbits/sec
[  4]   0.20-0.30   sec  6.34 MBytes   532 Mbits/sec
[  4]   0.30-0.40   sec  0.00 Bytes  0.00 bits/sec
[  4]   0.40-0.50   sec  0.00 Bytes  0.00 bits/sec
[  4]   0.50-0.60   sec  0.00 Bytes  0.00 bits/sec
[  4]   0.60-0.70   sec  0.00 Bytes  0.00 bits/sec
[  4]   0.70-0.80   sec  0.00 Bytes  0.00 bits/sec
[  4]   0.80-0.90   sec  0.00 Bytes  0.00 bits/sec
[  4]   0.90-1.00   sec  0.00 Bytes  0.00 bits/sec
[  4]   1.00-1.10   sec  0.00 Bytes  0.00 bits/sec
^C[  4]   1.10-1.10   sec  0.00 Bytes  0.00 bits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bandwidth
[  4]   0.00-1.10   sec  0.00 Bytes  0.00 bits/sec                  sender
[  4]   0.00-1.10   sec  28.2 MBytes   214 Mbits/sec                  receiver
iperf3: interrupt - the client has terminated

Neil

> 
> I'm so sorry about this back and forth and I appreciate all your
> help .
> 
> Thanks and Best Regards,
> Jose Miguel Abreu
> 
> 
>>
>> Neil
>>
>>> Thanks and Best Regards,
>>> Jose Miguel Abreu
>>>
> 

Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Jose Abreu]

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]

On 10-09-2018 19:15, Neil Armstrong wrote:
>
> RX is still ok but now TX fails almost immediately...
>
> With 100ms report :
>
> $ iperf3 -c 192.168.1.47 -t 0 -p 5202 -R -i 0.1
> Connecting to host 192.168.1.47, port 5202
> Reverse mode, remote host 192.168.1.47 is sending
> [  4] local 192.168.1.45 port 45900 connected to 192.168.1.47 port 5202
> [ ID] Interval           Transfer     Bandwidth
> [  4]   0.00-0.10   sec  10.9 MBytes   913 Mbits/sec
> [  4]   0.10-0.20   sec  11.0 MBytes   923 Mbits/sec
> [  4]   0.20-0.30   sec  6.34 MBytes   532 Mbits/sec
> [  4]   0.30-0.40   sec  0.00 Bytes  0.00 bits/sec
> [  4]   0.40-0.50   sec  0.00 Bytes  0.00 bits/sec
> [  4]   0.50-0.60   sec  0.00 Bytes  0.00 bits/sec
> [  4]   0.60-0.70   sec  0.00 Bytes  0.00 bits/sec
> [  4]   0.70-0.80   sec  0.00 Bytes  0.00 bits/sec
> [  4]   0.80-0.90   sec  0.00 Bytes  0.00 bits/sec
> [  4]   0.90-1.00   sec  0.00 Bytes  0.00 bits/sec
> [  4]   1.00-1.10   sec  0.00 Bytes  0.00 bits/sec
> ^C[  4]   1.10-1.10   sec  0.00 Bytes  0.00 bits/sec
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bandwidth
> [  4]   0.00-1.10   sec  0.00 Bytes  0.00 bits/sec                  sender
> [  4]   0.00-1.10   sec  28.2 MBytes   214 Mbits/sec                  receiver
> iperf3: interrupt - the client has terminated
>
> Neil

Ok, here goes another incremental patch. If this doesn't work can
you please send me a link to the spec of the board you are using ?

Thanks and Best Regards,
Jose Miguel Abreu


[-- Attachment #2: 0001-fixup_coalesce_4.patch --]
[-- Type: text/x-patch, Size: 2739 bytes --]

>From d6c3bc9c282eadfa754bd78e7c7447a200dd1737 Mon Sep 17 00:00:00 2001
Message-Id: <d6c3bc9c282eadfa754bd78e7c7447a200dd1737.1536653739.git.joabreu@synopsys.com>
From: Jose Abreu <joabreu@synopsys.com>
Date: Tue, 11 Sep 2018 10:15:31 +0200
Subject: [PATCH] fixup_coalesce_4

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 ++++++++++-------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f6587ee372ab..b6d661f17bd7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1857,17 +1857,14 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
  * @queue: TX queue index
  * Description: it reclaims the transmit resources after transmission completes.
  */
-static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue,
-			   bool *more)
+static bool stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
 {
 	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
 	unsigned int bytes_compl = 0, pkts_compl = 0;
+	bool has_more = false;
 	unsigned int entry;
 
-	netif_tx_lock(priv->dev);
-
-	if (more)
-		*more = false;
+	netif_tx_lock_bh(priv->dev);
 
 	priv->xstats.tx_clean++;
 
@@ -1956,12 +1953,12 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue,
 		mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(eee_timer));
 	}
 
-	if (more && (tx_q->dirty_tx != tx_q->cur_tx))
-		*more = true;
+	if (tx_q->dirty_tx != tx_q->cur_tx)
+		has_more = true;
 
-	netif_tx_unlock(priv->dev);
+	netif_tx_unlock_bh(priv->dev);
 
-	return pkts_compl;
+	return has_more;
 }
 
 /**
@@ -2257,10 +2254,9 @@ static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
 static void stmmac_tx_timer(struct timer_list *t)
 {
 	struct stmmac_tx_queue *tx_q = from_timer(tx_q, t, txtimer);
-	struct stmmac_priv *priv = tx_q->priv_data;
-	bool more;
 
-	stmmac_tx_clean(priv, ~0, tx_q->queue_index, &more);
+	if (likely(napi_schedule_prep(&tx_q->napi)))
+		__napi_schedule(&tx_q->napi);
 }
 
 /**
@@ -3562,15 +3558,14 @@ static int stmmac_tx_poll(struct napi_struct *napi, int budget)
 		container_of(napi, struct stmmac_tx_queue, napi);
 	struct stmmac_priv *priv = tx_q->priv_data;
 	u32 chan = tx_q->queue_index;
-	int work_done = 0;
 
 	priv->xstats.napi_poll++;
 
-	work_done = stmmac_tx_clean(priv, budget, chan, NULL);
-	if (work_done < budget)
-		napi_complete_done(napi, work_done);
+	if (stmmac_tx_clean(priv, chan))
+		return budget;
 
-	return min(work_done, budget);
+	napi_complete_done(napi, 0);
+	return 0;
 }
 
 /**
-- 
2.7.4

Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Neil Armstrong]

Hi Jose,

On 11/09/2018 10:17, Jose Abreu wrote:
> On 10-09-2018 19:15, Neil Armstrong wrote:
>>
>> RX is still ok but now TX fails almost immediately...
>>
>> With 100ms report :
>>
>> $ iperf3 -c 192.168.1.47 -t 0 -p 5202 -R -i 0.1
>> Connecting to host 192.168.1.47, port 5202
>> Reverse mode, remote host 192.168.1.47 is sending
>> [  4] local 192.168.1.45 port 45900 connected to 192.168.1.47 port 5202
>> [ ID] Interval           Transfer     Bandwidth
>> [  4]   0.00-0.10   sec  10.9 MBytes   913 Mbits/sec
>> [  4]   0.10-0.20   sec  11.0 MBytes   923 Mbits/sec
>> [  4]   0.20-0.30   sec  6.34 MBytes   532 Mbits/sec
>> [  4]   0.30-0.40   sec  0.00 Bytes  0.00 bits/sec
>> [  4]   0.40-0.50   sec  0.00 Bytes  0.00 bits/sec
>> [  4]   0.50-0.60   sec  0.00 Bytes  0.00 bits/sec
>> [  4]   0.60-0.70   sec  0.00 Bytes  0.00 bits/sec
>> [  4]   0.70-0.80   sec  0.00 Bytes  0.00 bits/sec
>> [  4]   0.80-0.90   sec  0.00 Bytes  0.00 bits/sec
>> [  4]   0.90-1.00   sec  0.00 Bytes  0.00 bits/sec
>> [  4]   1.00-1.10   sec  0.00 Bytes  0.00 bits/sec
>> ^C[  4]   1.10-1.10   sec  0.00 Bytes  0.00 bits/sec
>> - - - - - - - - - - - - - - - - - - - - - - - - -
>> [ ID] Interval           Transfer     Bandwidth
>> [  4]   0.00-1.10   sec  0.00 Bytes  0.00 bits/sec                  sender
>> [  4]   0.00-1.10   sec  28.2 MBytes   214 Mbits/sec                  receiver
>> iperf3: interrupt - the client has terminated
>>
>> Neil
> 
> Ok, here goes another incremental patch. If this doesn't work can
> you please send me a link to the spec of the board you are using ?

Sorry for the delay...

Not better, sorry.

$ iperf3 -c 10.1.3.201 -p 5202 -R -t 0
Connecting to host 10.1.3.201, port 5202
Reverse mode, remote host 10.1.3.201 is sending
[  4] local 10.1.2.12 port 60612 connected to 10.1.3.201 port 5202
[ ID] Interval           Transfer     Bandwidth
[  4]   0.00-1.00   sec   110 MBytes   920 Mbits/sec
[  4]   1.00-2.00   sec   110 MBytes   926 Mbits/sec
[  4]   2.00-3.00   sec  1.94 MBytes  16.3 Mbits/sec
[  4]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec
^C[  4]   4.00-4.76   sec  0.00 Bytes  0.00 bits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bandwidth
[  4]   0.00-4.76   sec  0.00 Bytes  0.00 bits/sec                  sender
[  4]   0.00-4.76   sec   222 MBytes   391 Mbits/sec                  receiver
iperf3: interrupt - the client has terminated

The board is the Amlogic S400 with the A113D SoC, sorry there is no public spec for this board and for this SoC.

Neil

> 
> Thanks and Best Regards,
> Jose Miguel Abreu
> 

Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Jose Abreu]

Hi Neil,

On 12-09-2018 14:50, Neil Armstrong wrote:
> Hi Jose,
>
> On 11/09/2018 10:17, Jose Abreu wrote:
>> On 10-09-2018 19:15, Neil Armstrong wrote:
>>> RX is still ok but now TX fails almost immediately...
>>>
>>> With 100ms report :
>>>
>>> $ iperf3 -c 192.168.1.47 -t 0 -p 5202 -R -i 0.1
>>> Connecting to host 192.168.1.47, port 5202
>>> Reverse mode, remote host 192.168.1.47 is sending
>>> [  4] local 192.168.1.45 port 45900 connected to 192.168.1.47 port 5202
>>> [ ID] Interval           Transfer     Bandwidth
>>> [  4]   0.00-0.10   sec  10.9 MBytes   913 Mbits/sec
>>> [  4]   0.10-0.20   sec  11.0 MBytes   923 Mbits/sec
>>> [  4]   0.20-0.30   sec  6.34 MBytes   532 Mbits/sec
>>> [  4]   0.30-0.40   sec  0.00 Bytes  0.00 bits/sec
>>> [  4]   0.40-0.50   sec  0.00 Bytes  0.00 bits/sec
>>> [  4]   0.50-0.60   sec  0.00 Bytes  0.00 bits/sec
>>> [  4]   0.60-0.70   sec  0.00 Bytes  0.00 bits/sec
>>> [  4]   0.70-0.80   sec  0.00 Bytes  0.00 bits/sec
>>> [  4]   0.80-0.90   sec  0.00 Bytes  0.00 bits/sec
>>> [  4]   0.90-1.00   sec  0.00 Bytes  0.00 bits/sec
>>> [  4]   1.00-1.10   sec  0.00 Bytes  0.00 bits/sec
>>> ^C[  4]   1.10-1.10   sec  0.00 Bytes  0.00 bits/sec
>>> - - - - - - - - - - - - - - - - - - - - - - - - -
>>> [ ID] Interval           Transfer     Bandwidth
>>> [  4]   0.00-1.10   sec  0.00 Bytes  0.00 bits/sec                  sender
>>> [  4]   0.00-1.10   sec  28.2 MBytes   214 Mbits/sec                  receiver
>>> iperf3: interrupt - the client has terminated
>>>
>>> Neil
>> Ok, here goes another incremental patch. If this doesn't work can
>> you please send me a link to the spec of the board you are using ?
> Sorry for the delay...
>
> Not better, sorry.
>
> $ iperf3 -c 10.1.3.201 -p 5202 -R -t 0
> Connecting to host 10.1.3.201, port 5202
> Reverse mode, remote host 10.1.3.201 is sending
> [  4] local 10.1.2.12 port 60612 connected to 10.1.3.201 port 5202
> [ ID] Interval           Transfer     Bandwidth
> [  4]   0.00-1.00   sec   110 MBytes   920 Mbits/sec
> [  4]   1.00-2.00   sec   110 MBytes   926 Mbits/sec
> [  4]   2.00-3.00   sec  1.94 MBytes  16.3 Mbits/sec
> [  4]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec
> ^C[  4]   4.00-4.76   sec  0.00 Bytes  0.00 bits/sec
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bandwidth
> [  4]   0.00-4.76   sec  0.00 Bytes  0.00 bits/sec                  sender
> [  4]   0.00-4.76   sec   222 MBytes   391 Mbits/sec                  receiver
> iperf3: interrupt - the client has terminated
>
> The board is the Amlogic S400 with the A113D SoC, sorry there is no public spec for this board and for this SoC.

Thanks for testing. I will send a new version with major
differences, if you could validate it it would be great.

Thanks and Best Regards,
Jose Miguel Abreu

>
> Neil
>
>> Thanks and Best Regards,
>> Jose Miguel Abreu
>>

Re: [PATCH net-next v2 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Florian Fainelli]

On 09/10/2018 02:14 AM, Jose Abreu wrote:
> This follows David Miller advice and tries to fix coalesce timer in
> multi-queue scenarios.
> 
> We are now using per-queue coalesce values and per-queue TX timer.
> 
> Coalesce timer default values was changed to 1ms and the coalesce frames
> to 25.
> 
> Tested in B2B setup between XGMAC2 and GMAC5.

Why not revert the entire features for this merge window and work on
getting it to work over the next weeks/merge windows?

The idea of using a timer to coalesce TX path when there is not a HW
timer is a good idea and if this is made robust enough, you could even
promote that as being a network stack library/feature that could be used
by other drivers. In fact, this could be a great addition to the net DIM
library (Tal, what do you think?)

Here's a quick drive by review of things that appear wrong in the
current driver (without your patches):

- in stmmac_xmit(), in case we hit the !is_jumbo branch and we fail the
DMA mapping, there is no timer cancellation, don't we want to abort the
whole transmission?

- stmmac_tx_clean() should probably use netif_lock_bh() to guard against
the timer (soft IRQ context) and the the NAPI context (also soft IRQ)
running in parallel on two different CPUs. This may not explain all
problems, but these two things are fundamentally exclusive, because the
timer is meant to emulate the interrupt after N packets, while NAPI
executes when such a thing did actually occur

- stmmac_poll() should cancel pending timer(s) if it was able to reclaim
packets, likewise stmmac_tx_timer() should re-enable TX interrupts if it
reclaimed packets, since TX interrupts could have been left disabled
from a prior NAPI run. These could be considered optimizations, since
you could leave the TX timer running all the time, just adjust the
deadline (based on line rate, MTU, IPG, number of fragments and their
respective length), worst case, both NAPI and the timer clean up your TX
ring, so you should always have room to push more packets

> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> ---
> Jerome,
> 
> Can you please test if this one is okay ?
> 
> Thanks and Best Regards,
> Jose Miguel Abreu
> ---
>  drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   6 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 207 ++++++++++++++--------
>  3 files changed, 135 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> index 1854f270ad66..b1b305f8f414 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -258,10 +258,10 @@ struct stmmac_safety_stats {
>  #define MAX_DMA_RIWT		0xff
>  #define MIN_DMA_RIWT		0x20
>  /* Tx coalesce parameters */
> -#define STMMAC_COAL_TX_TIMER	40000
> +#define STMMAC_COAL_TX_TIMER	1000
>  #define STMMAC_MAX_COAL_TX_TICK	100000
>  #define STMMAC_TX_MAX_FRAMES	256
> -#define STMMAC_TX_FRAMES	64
> +#define STMMAC_TX_FRAMES	25
>  
>  /* Packets types */
>  enum packets_types {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index c0a855b7ab3b..957030cfb833 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -48,6 +48,9 @@ struct stmmac_tx_info {
>  
>  /* Frequently used values are kept adjacent for cache effect */
>  struct stmmac_tx_queue {
> +	u32 tx_count_frames;
> +	int tx_timer_active;
> +	struct timer_list txtimer;
>  	u32 queue_index;
>  	struct stmmac_priv *priv_data;
>  	struct dma_extended_desc *dma_etx ____cacheline_aligned_in_smp;
> @@ -59,6 +62,7 @@ struct stmmac_tx_queue {
>  	dma_addr_t dma_tx_phy;
>  	u32 tx_tail_addr;
>  	u32 mss;
> +	struct napi_struct napi ____cacheline_aligned_in_smp;
>  };
>  
>  struct stmmac_rx_queue {
> @@ -109,14 +113,12 @@ struct stmmac_pps_cfg {
>  
>  struct stmmac_priv {
>  	/* Frequently used values are kept adjacent for cache effect */
> -	u32 tx_count_frames;
>  	u32 tx_coal_frames;
>  	u32 tx_coal_timer;
>  
>  	int tx_coalesce;
>  	int hwts_tx_en;
>  	bool tx_path_in_lpi_mode;
> -	struct timer_list txtimer;
>  	bool tso;
>  
>  	unsigned int dma_buf_sz;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9f458bb16f2a..9809c2b319fe 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -148,6 +148,7 @@ static void stmmac_verify_args(void)
>  static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>  {
>  	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>  	u32 queue;
>  
>  	for (queue = 0; queue < rx_queues_cnt; queue++) {
> @@ -155,6 +156,12 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>  
>  		napi_disable(&rx_q->napi);
>  	}
> +
> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		napi_disable(&tx_q->napi);
> +	}
>  }
>  
>  /**
> @@ -164,6 +171,7 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>  static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>  {
>  	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>  	u32 queue;
>  
>  	for (queue = 0; queue < rx_queues_cnt; queue++) {
> @@ -171,6 +179,12 @@ static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>  
>  		napi_enable(&rx_q->napi);
>  	}
> +
> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		napi_enable(&tx_q->napi);
> +	}
>  }
>  
>  /**
> @@ -1843,7 +1857,8 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
>   * @queue: TX queue index
>   * Description: it reclaims the transmit resources after transmission completes.
>   */
> -static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
> +static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue,
> +			   bool *more)
>  {
>  	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>  	unsigned int bytes_compl = 0, pkts_compl = 0;
> @@ -1851,10 +1866,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>  
>  	netif_tx_lock(priv->dev);
>  
> +	if (more)
> +		*more = false;
> +
>  	priv->xstats.tx_clean++;
>  
>  	entry = tx_q->dirty_tx;
> -	while (entry != tx_q->cur_tx) {
> +	while ((entry != tx_q->cur_tx) && (pkts_compl < limit)) {
>  		struct sk_buff *skb = tx_q->tx_skbuff[entry];
>  		struct dma_desc *p;
>  		int status;
> @@ -1937,7 +1955,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>  		stmmac_enable_eee_mode(priv);
>  		mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(eee_timer));
>  	}
> +
> +	if (more && (tx_q->dirty_tx != tx_q->cur_tx))
> +		*more = true;
> +
>  	netif_tx_unlock(priv->dev);
> +
> +	return pkts_compl;
>  }
>  
>  /**
> @@ -2020,6 +2044,34 @@ static bool stmmac_safety_feat_interrupt(struct stmmac_priv *priv)
>  	return false;
>  }
>  
> +static int stmmac_napi_check(struct stmmac_priv *priv, u32 chan)
> +{
> +	int status = stmmac_dma_interrupt_status(priv, priv->ioaddr,
> +						 &priv->xstats, chan);
> +
> +	if ((status & handle_rx) && (chan < priv->plat->rx_queues_to_use)) {
> +		struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
> +
> +		if (likely(napi_schedule_prep(&rx_q->napi))) {
> +			stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
> +			__napi_schedule(&rx_q->napi);
> +		}
> +	} else {
> +		status &= ~handle_rx;
> +	}
> +
> +	if ((status & handle_tx) && (chan < priv->plat->tx_queues_to_use)) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
> +
> +		if (likely(napi_schedule_prep(&tx_q->napi)))
> +			__napi_schedule(&tx_q->napi);
> +	} else {
> +		status &= ~handle_tx;
> +	}
> +
> +	return status;
> +}
> +
>  /**
>   * stmmac_dma_interrupt - DMA ISR
>   * @priv: driver private structure
> @@ -2034,57 +2086,14 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
>  	u32 channels_to_check = tx_channel_count > rx_channel_count ?
>  				tx_channel_count : rx_channel_count;
>  	u32 chan;
> -	bool poll_scheduled = false;
>  	int status[max_t(u32, MTL_MAX_TX_QUEUES, MTL_MAX_RX_QUEUES)];
>  
>  	/* Make sure we never check beyond our status buffer. */
>  	if (WARN_ON_ONCE(channels_to_check > ARRAY_SIZE(status)))
>  		channels_to_check = ARRAY_SIZE(status);
>  
> -	/* Each DMA channel can be used for rx and tx simultaneously, yet
> -	 * napi_struct is embedded in struct stmmac_rx_queue rather than in a
> -	 * stmmac_channel struct.
> -	 * Because of this, stmmac_poll currently checks (and possibly wakes)
> -	 * all tx queues rather than just a single tx queue.
> -	 */
>  	for (chan = 0; chan < channels_to_check; chan++)
> -		status[chan] = stmmac_dma_interrupt_status(priv, priv->ioaddr,
> -				&priv->xstats, chan);
> -
> -	for (chan = 0; chan < rx_channel_count; chan++) {
> -		if (likely(status[chan] & handle_rx)) {
> -			struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
> -
> -			if (likely(napi_schedule_prep(&rx_q->napi))) {
> -				stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
> -				__napi_schedule(&rx_q->napi);
> -				poll_scheduled = true;
> -			}
> -		}
> -	}
> -
> -	/* If we scheduled poll, we already know that tx queues will be checked.
> -	 * If we didn't schedule poll, see if any DMA channel (used by tx) has a
> -	 * completed transmission, if so, call stmmac_poll (once).
> -	 */
> -	if (!poll_scheduled) {
> -		for (chan = 0; chan < tx_channel_count; chan++) {
> -			if (status[chan] & handle_tx) {
> -				/* It doesn't matter what rx queue we choose
> -				 * here. We use 0 since it always exists.
> -				 */
> -				struct stmmac_rx_queue *rx_q =
> -					&priv->rx_queue[0];
> -
> -				if (likely(napi_schedule_prep(&rx_q->napi))) {
> -					stmmac_disable_dma_irq(priv,
> -							priv->ioaddr, chan);
> -					__napi_schedule(&rx_q->napi);
> -				}
> -				break;
> -			}
> -		}
> -	}
> +		status[chan] = stmmac_napi_check(priv, chan);
>  
>  	for (chan = 0; chan < tx_channel_count; chan++) {
>  		if (unlikely(status[chan] & tx_hard_error_bump_tc)) {
> @@ -2241,13 +2250,11 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
>   */
>  static void stmmac_tx_timer(struct timer_list *t)
>  {
> -	struct stmmac_priv *priv = from_timer(priv, t, txtimer);
> -	u32 tx_queues_count = priv->plat->tx_queues_to_use;
> -	u32 queue;
> +	struct stmmac_tx_queue *tx_q = from_timer(tx_q, t, txtimer);
>  
> -	/* let's scan all the tx queues */
> -	for (queue = 0; queue < tx_queues_count; queue++)
> -		stmmac_tx_clean(priv, queue);
> +	if (likely(napi_schedule_prep(&tx_q->napi)))
> +		__napi_schedule(&tx_q->napi);
> +	tx_q->tx_timer_active = 0;
>  }
>  
>  /**
> @@ -2260,11 +2267,17 @@ static void stmmac_tx_timer(struct timer_list *t)
>   */
>  static void stmmac_init_tx_coalesce(struct stmmac_priv *priv)
>  {
> +	u32 tx_channel_count = priv->plat->tx_queues_to_use;
> +	u32 chan;
> +
>  	priv->tx_coal_frames = STMMAC_TX_FRAMES;
>  	priv->tx_coal_timer = STMMAC_COAL_TX_TIMER;
> -	timer_setup(&priv->txtimer, stmmac_tx_timer, 0);
> -	priv->txtimer.expires = STMMAC_COAL_TIMER(priv->tx_coal_timer);
> -	add_timer(&priv->txtimer);
> +
> +	for (chan = 0; chan < tx_channel_count; chan++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
> +
> +		timer_setup(&tx_q->txtimer, stmmac_tx_timer, 0);
> +	}
>  }
>  
>  static void stmmac_set_rings_length(struct stmmac_priv *priv)
> @@ -2592,6 +2605,7 @@ static void stmmac_hw_teardown(struct net_device *dev)
>  static int stmmac_open(struct net_device *dev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> +	u32 chan;
>  	int ret;
>  
>  	stmmac_check_ether_addr(priv);
> @@ -2688,7 +2702,9 @@ static int stmmac_open(struct net_device *dev)
>  	if (dev->phydev)
>  		phy_stop(dev->phydev);
>  
> -	del_timer_sync(&priv->txtimer);
> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
> +
>  	stmmac_hw_teardown(dev);
>  init_error:
>  	free_dma_desc_resources(priv);
> @@ -2708,6 +2724,7 @@ static int stmmac_open(struct net_device *dev)
>  static int stmmac_release(struct net_device *dev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> +	u32 chan;
>  
>  	if (priv->eee_enabled)
>  		del_timer_sync(&priv->eee_ctrl_timer);
> @@ -2722,7 +2739,8 @@ static int stmmac_release(struct net_device *dev)
>  
>  	stmmac_disable_all_queues(priv);
>  
> -	del_timer_sync(&priv->txtimer);
> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
>  
>  	/* Free the IRQ lines */
>  	free_irq(dev->irq, dev);
> @@ -2936,14 +2954,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  	priv->xstats.tx_tso_nfrags += nfrags;
>  
>  	/* Manage tx mitigation */
> -	priv->tx_count_frames += nfrags + 1;
> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> -		mod_timer(&priv->txtimer,
> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> -	} else {
> -		priv->tx_count_frames = 0;
> +	tx_q->tx_count_frames += nfrags + 1;
> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>  		stmmac_set_tx_ic(priv, desc);
>  		priv->xstats.tx_set_ic_bit++;
> +		tx_q->tx_count_frames = 0;
>  	}
>  
>  	skb_tx_timestamp(skb);
> @@ -2994,6 +3009,12 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>  
> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
> +		tx_q->tx_timer_active = 1;
> +		mod_timer(&tx_q->txtimer,
> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
> +	}
> +
>  	return NETDEV_TX_OK;
>  
>  dma_map_err:
> @@ -3146,14 +3167,11 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	 * This approach takes care about the fragments: desc is the first
>  	 * element in case of no SG.
>  	 */
> -	priv->tx_count_frames += nfrags + 1;
> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> -		mod_timer(&priv->txtimer,
> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> -	} else {
> -		priv->tx_count_frames = 0;
> +	tx_q->tx_count_frames += nfrags + 1;
> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>  		stmmac_set_tx_ic(priv, desc);
>  		priv->xstats.tx_set_ic_bit++;
> +		tx_q->tx_count_frames = 0;
>  	}
>  
>  	skb_tx_timestamp(skb);
> @@ -3199,8 +3217,15 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	netdev_tx_sent_queue(netdev_get_tx_queue(dev, queue), skb->len);
>  
>  	stmmac_enable_dma_transmission(priv, priv->ioaddr);
> +
>  	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>  
> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
> +		tx_q->tx_timer_active = 1;
> +		mod_timer(&tx_q->txtimer,
> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
> +	}
> +
>  	return NETDEV_TX_OK;
>  
>  dma_map_err:
> @@ -3514,27 +3539,41 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>   *  Description :
>   *  To look at the incoming frames and clear the tx resources.
>   */
> -static int stmmac_poll(struct napi_struct *napi, int budget)
> +static int stmmac_rx_poll(struct napi_struct *napi, int budget)
>  {
>  	struct stmmac_rx_queue *rx_q =
>  		container_of(napi, struct stmmac_rx_queue, napi);
>  	struct stmmac_priv *priv = rx_q->priv_data;
> -	u32 tx_count = priv->plat->tx_queues_to_use;
>  	u32 chan = rx_q->queue_index;
>  	int work_done = 0;
> -	u32 queue;
>  
>  	priv->xstats.napi_poll++;
>  
> -	/* check all the queues */
> -	for (queue = 0; queue < tx_count; queue++)
> -		stmmac_tx_clean(priv, queue);
> -
>  	work_done = stmmac_rx(priv, budget, rx_q->queue_index);
> +	if (work_done < budget && napi_complete_done(napi, work_done))
> +		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
> +
> +	return work_done;
> +}
> +
> +static int stmmac_tx_poll(struct napi_struct *napi, int budget)
> +{
> +	struct stmmac_tx_queue *tx_q =
> +		container_of(napi, struct stmmac_tx_queue, napi);
> +	struct stmmac_priv *priv = tx_q->priv_data;
> +	u32 chan = tx_q->queue_index;
> +	int work_done = 0;
> +	bool more;
> +
> +	priv->xstats.napi_poll++;
> +
> +	work_done = stmmac_tx_clean(priv, budget, chan, &more);
>  	if (work_done < budget) {
>  		napi_complete_done(napi, work_done);
> -		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
> +		if (more)
> +			napi_reschedule(napi);
>  	}
> +
>  	return work_done;
>  }
>  
> @@ -4325,10 +4364,17 @@ int stmmac_dvr_probe(struct device *device,
>  	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
>  		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
>  
> -		netif_napi_add(ndev, &rx_q->napi, stmmac_poll,
> +		netif_napi_add(ndev, &rx_q->napi, stmmac_rx_poll,
>  			       (8 * priv->plat->rx_queues_to_use));
>  	}
>  
> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		netif_napi_add(ndev, &tx_q->napi, stmmac_tx_poll,
> +			       (8 * priv->plat->tx_queues_to_use));
> +	}
> +
>  	mutex_init(&priv->lock);
>  
>  	/* If a specific clk_csr value is passed from the platform
> @@ -4377,6 +4423,11 @@ int stmmac_dvr_probe(struct device *device,
>  
>  		netif_napi_del(&rx_q->napi);
>  	}
> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		netif_napi_del(&tx_q->napi);
> +	}
>  error_hw_init:
>  	destroy_workqueue(priv->wq);
>  error_wq:
> 


-- 
Florian

Re: [PATCH net-next v2 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Jose Abreu]

Hi Florian,

Thanks for your input.

On 10-09-2018 20:22, Florian Fainelli wrote:
> On 09/10/2018 02:14 AM, Jose Abreu wrote:
>> This follows David Miller advice and tries to fix coalesce timer in
>> multi-queue scenarios.
>>
>> We are now using per-queue coalesce values and per-queue TX timer.
>>
>> Coalesce timer default values was changed to 1ms and the coalesce frames
>> to 25.
>>
>> Tested in B2B setup between XGMAC2 and GMAC5.
> Why not revert the entire features for this merge window and work on
> getting it to work over the next weeks/merge windows?

It was already reverted but the performance drops a little bit
(not that much but I'm trying to fix it).

>
> The idea of using a timer to coalesce TX path when there is not a HW
> timer is a good idea and if this is made robust enough, you could even
> promote that as being a network stack library/feature that could be used
> by other drivers. In fact, this could be a great addition to the net DIM
> library (Tal, what do you think?)
>
> Here's a quick drive by review of things that appear wrong in the
> current driver (without your patches):
>
> - in stmmac_xmit(), in case we hit the !is_jumbo branch and we fail the
> DMA mapping, there is no timer cancellation, don't we want to abort the
> whole transmission?

I don't think this is needed because then tx pointer will not
advance and in stmmac_tx_clean we just won't perform any work.
Besides, we can have a pending timer from previous packets
running so canceling it can cause some problems.

>
> - stmmac_tx_clean() should probably use netif_lock_bh() to guard against
> the timer (soft IRQ context) and the the NAPI context (also soft IRQ)
> running in parallel on two different CPUs. This may not explain all
> problems, but these two things are fundamentally exclusive, because the
> timer is meant to emulate the interrupt after N packets, while NAPI
> executes when such a thing did actually occur

Ok, and now I'm also using __netif_tx_lock_bh(queue) to just lock
per queue instead of the whole TX.

>
> - stmmac_poll() should cancel pending timer(s) if it was able to reclaim
> packets, likewise stmmac_tx_timer() should re-enable TX interrupts if it
> reclaimed packets, since TX interrupts could have been left disabled
> from a prior NAPI run. These could be considered optimizations, since
> you could leave the TX timer running all the time, just adjust the
> deadline (based on line rate, MTU, IPG, number of fragments and their
> respective length), worst case, both NAPI and the timer clean up your TX
> ring, so you should always have room to push more packets

In next version I'm dropping the direct call to stmmac_tx_clean()
in the timer function and just scheduling NAPI instead.

Thanks and Best Regards,
Jose Miguel Abreu

Re: [PATCH net-next v2 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Tal Gilboa]

On 9/10/2018 10:22 PM, Florian Fainelli wrote:
> On 09/10/2018 02:14 AM, Jose Abreu wrote:
>> This follows David Miller advice and tries to fix coalesce timer in
>> multi-queue scenarios.
>>
>> We are now using per-queue coalesce values and per-queue TX timer.
>>
>> Coalesce timer default values was changed to 1ms and the coalesce frames
>> to 25.
>>
>> Tested in B2B setup between XGMAC2 and GMAC5.
> 
> Why not revert the entire features for this merge window and work on
> getting it to work over the next weeks/merge windows?
> 
> The idea of using a timer to coalesce TX path when there is not a HW
> timer is a good idea and if this is made robust enough, you could even
> promote that as being a network stack library/feature that could be used
> by other drivers. In fact, this could be a great addition to the net DIM
> library (Tal, what do you think?)

Not sure it would be a natural fit. DIM doesn't know/care which type of 
timer is used. Maybe the two mechanisms can work together with DIM 
optimizes the number of frames to use for optimal coalescing. As the 
timer is set to 1ms, DIM would have issues suggesting meaningful values 
as it is designed for typical values of 10s on us.

> 
> Here's a quick drive by review of things that appear wrong in the
> current driver (without your patches):
> 
> - in stmmac_xmit(), in case we hit the !is_jumbo branch and we fail the
> DMA mapping, there is no timer cancellation, don't we want to abort the
> whole transmission?
> 
> - stmmac_tx_clean() should probably use netif_lock_bh() to guard against
> the timer (soft IRQ context) and the the NAPI context (also soft IRQ)
> running in parallel on two different CPUs. This may not explain all
> problems, but these two things are fundamentally exclusive, because the
> timer is meant to emulate the interrupt after N packets, while NAPI
> executes when such a thing did actually occur
> 
> - stmmac_poll() should cancel pending timer(s) if it was able to reclaim
> packets, likewise stmmac_tx_timer() should re-enable TX interrupts if it
> reclaimed packets, since TX interrupts could have been left disabled
> from a prior NAPI run. These could be considered optimizations, since
> you could leave the TX timer running all the time, just adjust the
> deadline (based on line rate, MTU, IPG, number of fragments and their
> respective length), worst case, both NAPI and the timer clean up your TX
> ring, so you should always have room to push more packets
> 
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Jerome Brunet <jbrunet@baylibre.com>
>> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Cc: Alexandre Torgue <alexandre.torgue@st.com>
>> ---
>> Jerome,
>>
>> Can you please test if this one is okay ?
>>
>> Thanks and Best Regards,
>> Jose Miguel Abreu
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
>>   drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   6 +-
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 207 ++++++++++++++--------
>>   3 files changed, 135 insertions(+), 82 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
>> index 1854f270ad66..b1b305f8f414 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
>> @@ -258,10 +258,10 @@ struct stmmac_safety_stats {
>>   #define MAX_DMA_RIWT		0xff
>>   #define MIN_DMA_RIWT		0x20
>>   /* Tx coalesce parameters */
>> -#define STMMAC_COAL_TX_TIMER	40000
>> +#define STMMAC_COAL_TX_TIMER	1000
>>   #define STMMAC_MAX_COAL_TX_TICK	100000
>>   #define STMMAC_TX_MAX_FRAMES	256
>> -#define STMMAC_TX_FRAMES	64
>> +#define STMMAC_TX_FRAMES	25
>>   
>>   /* Packets types */
>>   enum packets_types {
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> index c0a855b7ab3b..957030cfb833 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> @@ -48,6 +48,9 @@ struct stmmac_tx_info {
>>   
>>   /* Frequently used values are kept adjacent for cache effect */
>>   struct stmmac_tx_queue {
>> +	u32 tx_count_frames;
>> +	int tx_timer_active;
>> +	struct timer_list txtimer;
>>   	u32 queue_index;
>>   	struct stmmac_priv *priv_data;
>>   	struct dma_extended_desc *dma_etx ____cacheline_aligned_in_smp;
>> @@ -59,6 +62,7 @@ struct stmmac_tx_queue {
>>   	dma_addr_t dma_tx_phy;
>>   	u32 tx_tail_addr;
>>   	u32 mss;
>> +	struct napi_struct napi ____cacheline_aligned_in_smp;
>>   };
>>   
>>   struct stmmac_rx_queue {
>> @@ -109,14 +113,12 @@ struct stmmac_pps_cfg {
>>   
>>   struct stmmac_priv {
>>   	/* Frequently used values are kept adjacent for cache effect */
>> -	u32 tx_count_frames;
>>   	u32 tx_coal_frames;
>>   	u32 tx_coal_timer;
>>   
>>   	int tx_coalesce;
>>   	int hwts_tx_en;
>>   	bool tx_path_in_lpi_mode;
>> -	struct timer_list txtimer;
>>   	bool tso;
>>   
>>   	unsigned int dma_buf_sz;
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 9f458bb16f2a..9809c2b319fe 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -148,6 +148,7 @@ static void stmmac_verify_args(void)
>>   static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>>   {
>>   	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
>> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>>   	u32 queue;
>>   
>>   	for (queue = 0; queue < rx_queues_cnt; queue++) {
>> @@ -155,6 +156,12 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>>   
>>   		napi_disable(&rx_q->napi);
>>   	}
>> +
>> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		napi_disable(&tx_q->napi);
>> +	}
>>   }
>>   
>>   /**
>> @@ -164,6 +171,7 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>>   static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>>   {
>>   	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
>> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>>   	u32 queue;
>>   
>>   	for (queue = 0; queue < rx_queues_cnt; queue++) {
>> @@ -171,6 +179,12 @@ static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>>   
>>   		napi_enable(&rx_q->napi);
>>   	}
>> +
>> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		napi_enable(&tx_q->napi);
>> +	}
>>   }
>>   
>>   /**
>> @@ -1843,7 +1857,8 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
>>    * @queue: TX queue index
>>    * Description: it reclaims the transmit resources after transmission completes.
>>    */
>> -static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>> +static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue,
>> +			   bool *more)
>>   {
>>   	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>>   	unsigned int bytes_compl = 0, pkts_compl = 0;
>> @@ -1851,10 +1866,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>>   
>>   	netif_tx_lock(priv->dev);
>>   
>> +	if (more)
>> +		*more = false;
>> +
>>   	priv->xstats.tx_clean++;
>>   
>>   	entry = tx_q->dirty_tx;
>> -	while (entry != tx_q->cur_tx) {
>> +	while ((entry != tx_q->cur_tx) && (pkts_compl < limit)) {
>>   		struct sk_buff *skb = tx_q->tx_skbuff[entry];
>>   		struct dma_desc *p;
>>   		int status;
>> @@ -1937,7 +1955,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>>   		stmmac_enable_eee_mode(priv);
>>   		mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(eee_timer));
>>   	}
>> +
>> +	if (more && (tx_q->dirty_tx != tx_q->cur_tx))
>> +		*more = true;
>> +
>>   	netif_tx_unlock(priv->dev);
>> +
>> +	return pkts_compl;
>>   }
>>   
>>   /**
>> @@ -2020,6 +2044,34 @@ static bool stmmac_safety_feat_interrupt(struct stmmac_priv *priv)
>>   	return false;
>>   }
>>   
>> +static int stmmac_napi_check(struct stmmac_priv *priv, u32 chan)
>> +{
>> +	int status = stmmac_dma_interrupt_status(priv, priv->ioaddr,
>> +						 &priv->xstats, chan);
>> +
>> +	if ((status & handle_rx) && (chan < priv->plat->rx_queues_to_use)) {
>> +		struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
>> +
>> +		if (likely(napi_schedule_prep(&rx_q->napi))) {
>> +			stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
>> +			__napi_schedule(&rx_q->napi);
>> +		}
>> +	} else {
>> +		status &= ~handle_rx;
>> +	}
>> +
>> +	if ((status & handle_tx) && (chan < priv->plat->tx_queues_to_use)) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
>> +
>> +		if (likely(napi_schedule_prep(&tx_q->napi)))
>> +			__napi_schedule(&tx_q->napi);
>> +	} else {
>> +		status &= ~handle_tx;
>> +	}
>> +
>> +	return status;
>> +}
>> +
>>   /**
>>    * stmmac_dma_interrupt - DMA ISR
>>    * @priv: driver private structure
>> @@ -2034,57 +2086,14 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
>>   	u32 channels_to_check = tx_channel_count > rx_channel_count ?
>>   				tx_channel_count : rx_channel_count;
>>   	u32 chan;
>> -	bool poll_scheduled = false;
>>   	int status[max_t(u32, MTL_MAX_TX_QUEUES, MTL_MAX_RX_QUEUES)];
>>   
>>   	/* Make sure we never check beyond our status buffer. */
>>   	if (WARN_ON_ONCE(channels_to_check > ARRAY_SIZE(status)))
>>   		channels_to_check = ARRAY_SIZE(status);
>>   
>> -	/* Each DMA channel can be used for rx and tx simultaneously, yet
>> -	 * napi_struct is embedded in struct stmmac_rx_queue rather than in a
>> -	 * stmmac_channel struct.
>> -	 * Because of this, stmmac_poll currently checks (and possibly wakes)
>> -	 * all tx queues rather than just a single tx queue.
>> -	 */
>>   	for (chan = 0; chan < channels_to_check; chan++)
>> -		status[chan] = stmmac_dma_interrupt_status(priv, priv->ioaddr,
>> -				&priv->xstats, chan);
>> -
>> -	for (chan = 0; chan < rx_channel_count; chan++) {
>> -		if (likely(status[chan] & handle_rx)) {
>> -			struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
>> -
>> -			if (likely(napi_schedule_prep(&rx_q->napi))) {
>> -				stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
>> -				__napi_schedule(&rx_q->napi);
>> -				poll_scheduled = true;
>> -			}
>> -		}
>> -	}
>> -
>> -	/* If we scheduled poll, we already know that tx queues will be checked.
>> -	 * If we didn't schedule poll, see if any DMA channel (used by tx) has a
>> -	 * completed transmission, if so, call stmmac_poll (once).
>> -	 */
>> -	if (!poll_scheduled) {
>> -		for (chan = 0; chan < tx_channel_count; chan++) {
>> -			if (status[chan] & handle_tx) {
>> -				/* It doesn't matter what rx queue we choose
>> -				 * here. We use 0 since it always exists.
>> -				 */
>> -				struct stmmac_rx_queue *rx_q =
>> -					&priv->rx_queue[0];
>> -
>> -				if (likely(napi_schedule_prep(&rx_q->napi))) {
>> -					stmmac_disable_dma_irq(priv,
>> -							priv->ioaddr, chan);
>> -					__napi_schedule(&rx_q->napi);
>> -				}
>> -				break;
>> -			}
>> -		}
>> -	}
>> +		status[chan] = stmmac_napi_check(priv, chan);
>>   
>>   	for (chan = 0; chan < tx_channel_count; chan++) {
>>   		if (unlikely(status[chan] & tx_hard_error_bump_tc)) {
>> @@ -2241,13 +2250,11 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
>>    */
>>   static void stmmac_tx_timer(struct timer_list *t)
>>   {
>> -	struct stmmac_priv *priv = from_timer(priv, t, txtimer);
>> -	u32 tx_queues_count = priv->plat->tx_queues_to_use;
>> -	u32 queue;
>> +	struct stmmac_tx_queue *tx_q = from_timer(tx_q, t, txtimer);
>>   
>> -	/* let's scan all the tx queues */
>> -	for (queue = 0; queue < tx_queues_count; queue++)
>> -		stmmac_tx_clean(priv, queue);
>> +	if (likely(napi_schedule_prep(&tx_q->napi)))
>> +		__napi_schedule(&tx_q->napi);
>> +	tx_q->tx_timer_active = 0;
>>   }
>>   
>>   /**
>> @@ -2260,11 +2267,17 @@ static void stmmac_tx_timer(struct timer_list *t)
>>    */
>>   static void stmmac_init_tx_coalesce(struct stmmac_priv *priv)
>>   {
>> +	u32 tx_channel_count = priv->plat->tx_queues_to_use;
>> +	u32 chan;
>> +
>>   	priv->tx_coal_frames = STMMAC_TX_FRAMES;
>>   	priv->tx_coal_timer = STMMAC_COAL_TX_TIMER;
>> -	timer_setup(&priv->txtimer, stmmac_tx_timer, 0);
>> -	priv->txtimer.expires = STMMAC_COAL_TIMER(priv->tx_coal_timer);
>> -	add_timer(&priv->txtimer);
>> +
>> +	for (chan = 0; chan < tx_channel_count; chan++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
>> +
>> +		timer_setup(&tx_q->txtimer, stmmac_tx_timer, 0);
>> +	}
>>   }
>>   
>>   static void stmmac_set_rings_length(struct stmmac_priv *priv)
>> @@ -2592,6 +2605,7 @@ static void stmmac_hw_teardown(struct net_device *dev)
>>   static int stmmac_open(struct net_device *dev)
>>   {
>>   	struct stmmac_priv *priv = netdev_priv(dev);
>> +	u32 chan;
>>   	int ret;
>>   
>>   	stmmac_check_ether_addr(priv);
>> @@ -2688,7 +2702,9 @@ static int stmmac_open(struct net_device *dev)
>>   	if (dev->phydev)
>>   		phy_stop(dev->phydev);
>>   
>> -	del_timer_sync(&priv->txtimer);
>> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
>> +
>>   	stmmac_hw_teardown(dev);
>>   init_error:
>>   	free_dma_desc_resources(priv);
>> @@ -2708,6 +2724,7 @@ static int stmmac_open(struct net_device *dev)
>>   static int stmmac_release(struct net_device *dev)
>>   {
>>   	struct stmmac_priv *priv = netdev_priv(dev);
>> +	u32 chan;
>>   
>>   	if (priv->eee_enabled)
>>   		del_timer_sync(&priv->eee_ctrl_timer);
>> @@ -2722,7 +2739,8 @@ static int stmmac_release(struct net_device *dev)
>>   
>>   	stmmac_disable_all_queues(priv);
>>   
>> -	del_timer_sync(&priv->txtimer);
>> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
>>   
>>   	/* Free the IRQ lines */
>>   	free_irq(dev->irq, dev);
>> @@ -2936,14 +2954,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>>   	priv->xstats.tx_tso_nfrags += nfrags;
>>   
>>   	/* Manage tx mitigation */
>> -	priv->tx_count_frames += nfrags + 1;
>> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
>> -		mod_timer(&priv->txtimer,
>> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> -	} else {
>> -		priv->tx_count_frames = 0;
>> +	tx_q->tx_count_frames += nfrags + 1;
>> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>>   		stmmac_set_tx_ic(priv, desc);
>>   		priv->xstats.tx_set_ic_bit++;
>> +		tx_q->tx_count_frames = 0;
>>   	}
>>   
>>   	skb_tx_timestamp(skb);
>> @@ -2994,6 +3009,12 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>>   
>>   	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>>   
>> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
>> +		tx_q->tx_timer_active = 1;
>> +		mod_timer(&tx_q->txtimer,
>> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> +	}
>> +
>>   	return NETDEV_TX_OK;
>>   
>>   dma_map_err:
>> @@ -3146,14 +3167,11 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>   	 * This approach takes care about the fragments: desc is the first
>>   	 * element in case of no SG.
>>   	 */
>> -	priv->tx_count_frames += nfrags + 1;
>> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
>> -		mod_timer(&priv->txtimer,
>> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> -	} else {
>> -		priv->tx_count_frames = 0;
>> +	tx_q->tx_count_frames += nfrags + 1;
>> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>>   		stmmac_set_tx_ic(priv, desc);
>>   		priv->xstats.tx_set_ic_bit++;
>> +		tx_q->tx_count_frames = 0;
>>   	}
>>   
>>   	skb_tx_timestamp(skb);
>> @@ -3199,8 +3217,15 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>   	netdev_tx_sent_queue(netdev_get_tx_queue(dev, queue), skb->len);
>>   
>>   	stmmac_enable_dma_transmission(priv, priv->ioaddr);
>> +
>>   	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>>   
>> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
>> +		tx_q->tx_timer_active = 1;
>> +		mod_timer(&tx_q->txtimer,
>> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> +	}
>> +
>>   	return NETDEV_TX_OK;
>>   
>>   dma_map_err:
>> @@ -3514,27 +3539,41 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>>    *  Description :
>>    *  To look at the incoming frames and clear the tx resources.
>>    */
>> -static int stmmac_poll(struct napi_struct *napi, int budget)
>> +static int stmmac_rx_poll(struct napi_struct *napi, int budget)
>>   {
>>   	struct stmmac_rx_queue *rx_q =
>>   		container_of(napi, struct stmmac_rx_queue, napi);
>>   	struct stmmac_priv *priv = rx_q->priv_data;
>> -	u32 tx_count = priv->plat->tx_queues_to_use;
>>   	u32 chan = rx_q->queue_index;
>>   	int work_done = 0;
>> -	u32 queue;
>>   
>>   	priv->xstats.napi_poll++;
>>   
>> -	/* check all the queues */
>> -	for (queue = 0; queue < tx_count; queue++)
>> -		stmmac_tx_clean(priv, queue);
>> -
>>   	work_done = stmmac_rx(priv, budget, rx_q->queue_index);
>> +	if (work_done < budget && napi_complete_done(napi, work_done))
>> +		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
>> +
>> +	return work_done;
>> +}
>> +
>> +static int stmmac_tx_poll(struct napi_struct *napi, int budget)
>> +{
>> +	struct stmmac_tx_queue *tx_q =
>> +		container_of(napi, struct stmmac_tx_queue, napi);
>> +	struct stmmac_priv *priv = tx_q->priv_data;
>> +	u32 chan = tx_q->queue_index;
>> +	int work_done = 0;
>> +	bool more;
>> +
>> +	priv->xstats.napi_poll++;
>> +
>> +	work_done = stmmac_tx_clean(priv, budget, chan, &more);
>>   	if (work_done < budget) {
>>   		napi_complete_done(napi, work_done);
>> -		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
>> +		if (more)
>> +			napi_reschedule(napi);
>>   	}
>> +
>>   	return work_done;
>>   }
>>   
>> @@ -4325,10 +4364,17 @@ int stmmac_dvr_probe(struct device *device,
>>   	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
>>   		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
>>   
>> -		netif_napi_add(ndev, &rx_q->napi, stmmac_poll,
>> +		netif_napi_add(ndev, &rx_q->napi, stmmac_rx_poll,
>>   			       (8 * priv->plat->rx_queues_to_use));
>>   	}
>>   
>> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		netif_napi_add(ndev, &tx_q->napi, stmmac_tx_poll,
>> +			       (8 * priv->plat->tx_queues_to_use));
>> +	}
>> +
>>   	mutex_init(&priv->lock);
>>   
>>   	/* If a specific clk_csr value is passed from the platform
>> @@ -4377,6 +4423,11 @@ int stmmac_dvr_probe(struct device *device,
>>   
>>   		netif_napi_del(&rx_q->napi);
>>   	}
>> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		netif_napi_del(&tx_q->napi);
>> +	}
>>   error_hw_init:
>>   	destroy_workqueue(priv->wq);
>>   error_wq:
>>
> 
> 

[PATCH net-next v2 2/2] net: stmmac: Fixup the tail addr setting in xmit path

[Jose Abreu]

Currently we are always setting the tail address of descriptor list to
the end of the pre-allocated list.

According to databook this is not correct. Tail address should point to
the last available descriptor + 1, which means we have to update the
tail address everytime we call the xmit function.

This should make no impact in older versions of MAC but in newer
versions there are some DMA features which allows the IP to fetch
descriptors in advance and in a non sequential order so its critical
that we set the tail address correctly.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Joao Pinto <jpinto@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9809c2b319fe..97268769186e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2229,8 +2229,7 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
 		stmmac_init_tx_chan(priv, priv->ioaddr, priv->plat->dma_cfg,
 				    tx_q->dma_tx_phy, chan);
 
-		tx_q->tx_tail_addr = tx_q->dma_tx_phy +
-			    (DMA_TX_SIZE * sizeof(struct dma_desc));
+		tx_q->tx_tail_addr = tx_q->dma_tx_phy;
 		stmmac_set_tx_tail_ptr(priv, priv->ioaddr,
 				       tx_q->tx_tail_addr, chan);
 	}
@@ -3007,6 +3006,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	netdev_tx_sent_queue(netdev_get_tx_queue(dev, queue), skb->len);
 
+	tx_q->tx_tail_addr = tx_q->dma_tx_phy + (tx_q->cur_tx * sizeof(*desc));
 	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
 
 	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
@@ -3218,6 +3218,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	stmmac_enable_dma_transmission(priv, priv->ioaddr);
 
+	tx_q->tx_tail_addr = tx_q->dma_tx_phy + (tx_q->cur_tx * sizeof(*desc));
 	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
 
 	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
-- 
2.7.4

Re: [PATCH net-next v2 2/2] net: stmmac: Fixup the tail addr setting in xmit path

[Florian Fainelli]

On 09/10/2018 02:14 AM, Jose Abreu wrote:
> Currently we are always setting the tail address of descriptor list to
> the end of the pre-allocated list.
> 
> According to databook this is not correct. Tail address should point to
> the last available descriptor + 1, which means we have to update the
> tail address everytime we call the xmit function.
> 
> This should make no impact in older versions of MAC but in newer
> versions there are some DMA features which allows the IP to fetch
> descriptors in advance and in a non sequential order so its critical
> that we set the tail address correctly.


Can you include the appropriate Fixes tag here so this can easily be
backported to relevant stable branches?
-- 
Florian

Re: [PATCH net-next v2 2/2] net: stmmac: Fixup the tail addr setting in xmit path

[Jose Abreu]

Hi Florian,

On 10-09-2018 19:46, Florian Fainelli wrote:
>
> Can you include the appropriate Fixes tag here so this can easily be
> backported to relevant stable branches?

Well I guess it goes since forever but it can only cause a major
impact in xgmac2 operation, remaining shall be okay.

I didn't add a Fixes tag because xgmac2 was merged quite recently
... Will add in next version.

Thanks and Best Regards,
Jose Miguel Abreu