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

[PATCH net-next 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      |   7 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 182 +++++++++++++++-------
 3 files changed, 129 insertions(+), 64 deletions(-)

-- 
2.7.4

[PATCH net-next 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 I have your Tested-by in this patch?

Thanks and Best Regards,
Jose Miguel Abreu
---
 drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   7 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 177 +++++++++++++++-------
 3 files changed, 126 insertions(+), 62 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 76649adf8fb0..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,15 +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;
-	bool tx_timer_armed;
 
 	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 ff1ffb46198a..1fca66ad6b17 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,18 +1857,18 @@ 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)
 {
 	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
 	unsigned int bytes_compl = 0, pkts_compl = 0;
-	unsigned int entry;
+	unsigned int entry, count = 0;
 
 	netif_tx_lock(priv->dev);
 
 	priv->xstats.tx_clean++;
 
 	entry = tx_q->dirty_tx;
-	while (entry != tx_q->cur_tx) {
+	while ((entry != tx_q->cur_tx) && (count < limit)) {
 		struct sk_buff *skb = tx_q->tx_skbuff[entry];
 		struct dma_desc *p;
 		int status;
@@ -1870,6 +1884,8 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
 		if (unlikely(status & tx_dma_own))
 			break;
 
+		count++;
+
 		/* Make sure descriptor fields are read after reading
 		 * the own bit.
 		 */
@@ -1938,6 +1954,8 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
 		mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(eee_timer));
 	}
 	netif_tx_unlock(priv->dev);
+
+	return count;
 }
 
 /**
@@ -2034,7 +2052,6 @@ 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. */
@@ -2055,11 +2072,8 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
 		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);
+			if (likely(napi_schedule_prep(&rx_q->napi)))
 				__napi_schedule(&rx_q->napi);
-				poll_scheduled = true;
-			}
 		}
 	}
 
@@ -2067,22 +2081,12 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
 	 * 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];
+	for (chan = 0; chan < tx_channel_count; chan++) {
+		if (status[chan] & handle_tx) {
+			struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
 
-				if (likely(napi_schedule_prep(&rx_q->napi))) {
-					stmmac_disable_dma_irq(priv,
-							priv->ioaddr, chan);
-					__napi_schedule(&rx_q->napi);
-				}
-				break;
-			}
+			if (likely(napi_schedule_prep(&tx_q->napi)))
+				__napi_schedule(&tx_q->napi);
 		}
 	}
 
@@ -2241,13 +2245,15 @@ 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);
+	struct stmmac_priv *priv = tx_q->priv_data;
 
-	/* let's scan all the tx queues */
-	for (queue = 0; queue < tx_queues_count; queue++)
-		stmmac_tx_clean(priv, queue);
+	if (napi_schedule_prep(&tx_q->napi)) {
+		stmmac_disable_dma_irq(priv, priv->ioaddr, tx_q->queue_index);
+		__napi_schedule(&tx_q->napi);
+	}
+
+	tx_q->tx_timer_active = 0;
 }
 
 /**
@@ -2260,11 +2266,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 +2604,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 +2701,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 +2723,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 +2738,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);
@@ -2828,6 +2845,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	int tmp_pay_len = 0;
 	u32 pay_len, mss;
 	u8 proto_hdr_len;
+	bool tx_ic;
 	int i;
 
 	tx_q = &priv->tx_queue[queue];
@@ -2936,12 +2954,17 @@ 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_ic = false;
+	else if ((nfrags + 1) > priv->tx_coal_frames)
+		tx_ic = true;
+	else if ((tx_q->tx_count_frames % priv->tx_coal_frames) < (nfrags + 1))
+		tx_ic = true;
+	else
+		tx_ic = false;
+
+	if (tx_ic) {
 		stmmac_set_tx_ic(priv, desc);
 		priv->xstats.tx_set_ic_bit++;
 	}
@@ -2994,6 +3017,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:
@@ -3024,6 +3053,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct stmmac_tx_queue *tx_q;
 	unsigned int enh_desc;
 	unsigned int des;
+	bool tx_ic;
 
 	tx_q = &priv->tx_queue[queue];
 
@@ -3146,17 +3176,19 @@ 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) &&
-	    !priv->tx_timer_armed) {
-		mod_timer(&priv->txtimer,
-			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
-		priv->tx_timer_armed = true;
-	} else {
-		priv->tx_count_frames = 0;
+	tx_q->tx_count_frames += nfrags + 1;
+	if (!priv->tx_coal_frames)
+		tx_ic = false;
+	else if ((nfrags + 1) > priv->tx_coal_frames)
+		tx_ic = true;
+	else if ((tx_q->tx_count_frames % priv->tx_coal_frames) < (nfrags + 1))
+		tx_ic = true;
+	else
+		tx_ic = false;
+
+	if (tx_ic) {
 		stmmac_set_tx_ic(priv, desc);
 		priv->xstats.tx_set_ic_bit++;
-		priv->tx_timer_armed = false;
 	}
 
 	skb_tx_timestamp(skb);
@@ -3202,8 +3234,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:
@@ -3517,22 +3556,16 @@ 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);
@@ -3541,6 +3574,24 @@ static int stmmac_poll(struct napi_struct *napi, int budget)
 	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;
+
+	priv->xstats.napi_poll++;
+
+	work_done = stmmac_tx_clean(priv, budget, chan);
+	if (work_done < budget) {
+		napi_complete_done(napi, work_done);
+		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
+	}
+	return work_done;
+}
+
 /**
  *  stmmac_tx_timeout
  *  @dev : Pointer to net device structure
@@ -4328,10 +4379,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
@@ -4380,6 +4438,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: [PATCH net-next 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races

[Jerome Brunet]

On Mon, 2018-09-03 at 14:35 +0100, 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,
> 
> Can I have your Tested-by in this patch?

You had it on what you sent in the RFT, but this different.

Like with the RFT, the network breakdown we had is no longer reproduced.
However this patch wreck the Rx throughput (680MBps -> 35MBps)

BTW, this patch and the RFT assume that 4ae0169fd1b3 ("net: stmmac: Do not keep
rearming the coalesce timer in stmmac_xmit") is still applied but I believe
David reverted the patch.

If you still need this change, you should include it back in your changeset.

> 
> Thanks and Best Regards,
> Jose Miguel Abreu
> ---
>  drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   7 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 177 +++++++++++++++-------
>  3 files changed, 126 insertions(+), 62 deletions(-)

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

[Jose Abreu]

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

On 03-09-2018 15:07, Jerome Brunet wrote:
>
> You had it on what you sent in the RFT, but this different.

Yeah, I had to fix the logic where tx queues != rx queues...

>
> Like with the RFT, the network breakdown we had is no longer reproduced.
> However this patch wreck the Rx throughput (680MBps -> 35MBps)

Damn, thats low. And I cant reproduce it here :/

Strange because I barely messed around with RX path...

Can you try attached patch in top of this one please?

>
> BTW, this patch and the RFT assume that 4ae0169fd1b3 ("net: stmmac: Do not keep
> rearming the coalesce timer in stmmac_xmit") is still applied but I believe
> David reverted the patch.
>
> If you still need this change, you should include it back in your changeset.

Yes I know it was reverted but -net was not merged into -next yet...

Thanks and Best Regards,
Jose Miguel Abreu

>
>> Thanks and Best Regards,
>> Jose Miguel Abreu
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
>>  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   7 +-
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 177 +++++++++++++++-------
>>  3 files changed, 126 insertions(+), 62 deletions(-)
>


[-- Attachment #2: fixup.patch --]
[-- Type: text/x-patch, Size: 668 bytes --]

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 14f890f2a970..3c7cfda80433 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2247,10 +2247,8 @@ 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 (napi_schedule_prep(&tx_q->napi)) {
-		stmmac_disable_dma_irq(priv, priv->ioaddr, tx_q->queue_index);
+	if (napi_schedule_prep(&tx_q->napi))
 		__napi_schedule(&tx_q->napi);
-	}
 
 	tx_q->tx_timer_active = 0;
 }

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

[Jerome Brunet]

On Mon, 2018-09-03 at 16:19 +0100, Jose Abreu wrote:
> On 03-09-2018 15:07, Jerome Brunet wrote:
> > 
> > You had it on what you sent in the RFT, but this different.
> 
> Yeah, I had to fix the logic where tx queues != rx queues...
> 
> > 
> > Like with the RFT, the network breakdown we had is no longer reproduced.
> > However this patch wreck the Rx throughput (680MBps -> 35MBps)
> 
> Damn, thats low. And I cant reproduce it here :/
> 
> Strange because I barely messed around with RX path...
> 
> Can you try attached patch in top of this one please?

Situation is even worse with this.
I'm using an NFS root filesystem. With your fixup, I'm not reaching the prompt
anymore. Looks like a the same kind of network breakdown we had previously

> 
> > 
> > BTW, this patch and the RFT assume that 4ae0169fd1b3 ("net: stmmac: Do not keep
> > rearming the coalesce timer in stmmac_xmit") is still applied but I believe
> > David reverted the patch.
> > 
> > If you still need this change, you should include it back in your changeset.
> 
> Yes I know it was reverted but -net was not merged into -next yet...
> 
> Thanks and Best Regards,
> Jose Miguel Abreu
> 
> > 
> > > Thanks and Best Regards,
> > > Jose Miguel Abreu
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   7 +-
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 177 +++++++++++++++-------
> > >  3 files changed, 126 insertions(+), 62 deletions(-)
> 
> 

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

[Jose Abreu]

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

Hi Jerome,

On 03-09-2018 17:22, Jerome Brunet wrote:
>
> Situation is even worse with this.
> I'm using an NFS root filesystem. With your fixup, I'm not reaching the prompt
> anymore. Looks like a the same kind of network breakdown we had previously
>

I was able to reproduce your problem and the attached fixup patch
fixed it up for me. Can you please try?

Thanks and Best Regards,
Jose Miguel Abreu

[-- Attachment #2: fixup2.patch --]
[-- Type: text/x-patch, Size: 2485 bytes --]

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 14f890f2a970..2cf927ccb409 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1861,14 +1861,14 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue)
 {
 	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
 	unsigned int bytes_compl = 0, pkts_compl = 0;
-	unsigned int entry, count = 0;
+	unsigned int entry;
 
 	netif_tx_lock(priv->dev);
 
 	priv->xstats.tx_clean++;
 
 	entry = tx_q->dirty_tx;
-	while ((entry != tx_q->cur_tx) && (count < limit)) {
+	while ((entry != tx_q->cur_tx) && (pkts_compl < limit)) {
 		struct sk_buff *skb = tx_q->tx_skbuff[entry];
 		struct dma_desc *p;
 		int status;
@@ -1884,8 +1884,6 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue)
 		if (unlikely(status & tx_dma_own))
 			break;
 
-		count++;
-
 		/* Make sure descriptor fields are read after reading
 		 * the own bit.
 		 */
@@ -1955,7 +1953,7 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue)
 	}
 	netif_tx_unlock(priv->dev);
 
-	return count;
+	return pkts_compl;
 }
 
 /**
@@ -2072,8 +2070,11 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
 		if (likely(status[chan] & handle_rx)) {
 			struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
 
-			if (likely(napi_schedule_prep(&rx_q->napi)))
+			if (likely(napi_schedule_prep(&rx_q->napi))) {
+				stmmac_disable_dma_irq(priv, priv->ioaddr,
+						       rx_q->queue_index);
 				__napi_schedule(&rx_q->napi);
+			}
 		}
 	}
 
@@ -2085,8 +2086,11 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
 		if (status[chan] & handle_tx) {
 			struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
 
-			if (likely(napi_schedule_prep(&tx_q->napi)))
+			if (likely(napi_schedule_prep(&tx_q->napi))) {
+				stmmac_disable_dma_irq(priv, priv->ioaddr,
+						       tx_q->queue_index);
 				__napi_schedule(&tx_q->napi);
+			}
 		}
 	}
 
@@ -2247,11 +2251,7 @@ 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 (napi_schedule_prep(&tx_q->napi)) {
-		stmmac_disable_dma_irq(priv, priv->ioaddr, tx_q->queue_index);
-		__napi_schedule(&tx_q->napi);
-	}
-
+	stmmac_tx_clean(priv, ~0, tx_q->queue_index);
 	tx_q->tx_timer_active = 0;
 }
 

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

[Jerome Brunet]

On Tue, 2018-09-04 at 10:57 +0100, Jose Abreu wrote:
> Hi Jerome,
> 
> On 03-09-2018 17:22, Jerome Brunet wrote:
> > 
> > Situation is even worse with this.
> > I'm using an NFS root filesystem. With your fixup, I'm not reaching the prompt
> > anymore. Looks like a the same kind of network breakdown we had previously
> > 
> 
> I was able to reproduce your problem and the attached fixup patch
> fixed it up for me. Can you please try?

I suppose this applies on top the initial patch, not the previous fixup
(judging from the rejection) Could you details the baseline for each
patch you send, its not easy to follow.

BTW, there something weird (at least for me) with the patch you attach.
git always refuse to apply them and even patch complains:

git apply fixup2.patch
error: patch failed: drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:1861
error: drivers/net/ethernet/stmicro/stmmac/stmmac_main.c: patch does not apply

patch -p1 < fixup2.patch
(Stripping trailing CRs from patch; use --binary to disable.)
patching file drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
Hunk #6 succeeded at 2252 (offset 1 line).                               
patch unexpectedly ends in middle of line
patch unexpectedly ends in middle of line

Anyway, with this second fixup, I'm back to square one:
I can boot but iperf3 won't hold for long


# iperf3 -c 10.1.2.124 -p 12345 -t 600
Connecting to host 10.1.2.124, port 12345
[  4] local 10.1.4.59 port 38650 connected to 10.1.2.124 port 12345
[ ID] Interval           Transfer     Bandwidth       Retr  Cwnd
[  4]   0.00-1.00   sec  80.8 MBytes   678 Mbits/sec    1    300 KBytes
[  4]   1.00-2.00   sec  81.1 MBytes   680 Mbits/sec    0    329 KBytes
[  4]   2.00-3.00   sec  80.7 MBytes   677 Mbits/sec    0    335 KBytes
[  4]   3.00-4.00   sec  81.7 MBytes   685 Mbits/sec    0    337 KBytes
[  4]   4.00-5.00   sec  81.0 MBytes   680 Mbits/sec    0    341 KBytes
[  4]   5.00-6.00   sec  81.0 MBytes   680 Mbits/sec    0    344 KBytes
[  4]   6.00-7.00   sec  80.7 MBytes   677 Mbits/sec    0    345 KBytes
[  4]   7.00-8.00   sec  81.5 MBytes   684 Mbits/sec    0    346 KBytes
[  4]   8.00-9.00   sec  81.2 MBytes   680 Mbits/sec    0    348 KBytes
[  4]   9.00-10.00  sec  5.59 MBytes  46.9 Mbits/sec    2   1.41 KBytes
[  4]  10.00-11.00  sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
[  4]  11.00-12.00  sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
[  4]  12.00-13.00  sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
[  4]  13.00-14.00  sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes


> 
> Thanks and Best Regards,
> Jose Miguel Abreu

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

[Jose Abreu]

Hi Jerome,

On 04-09-2018 13:27, Jerome Brunet wrote:
> On Tue, 2018-09-04 at 10:57 +0100, Jose Abreu wrote:
>> Hi Jerome,
>>
>> On 03-09-2018 17:22, Jerome Brunet wrote:
>>> Situation is even worse with this.
>>> I'm using an NFS root filesystem. With your fixup, I'm not reaching the prompt
>>> anymore. Looks like a the same kind of network breakdown we had previously
>>>
>> I was able to reproduce your problem and the attached fixup patch
>> fixed it up for me. Can you please try?
> I suppose this applies on top the initial patch, not the previous fixup
> (judging from the rejection) Could you details the baseline for each
> patch you send, its not easy to follow.
>
> BTW, there something weird (at least for me) with the patch you attach.
> git always refuse to apply them and even patch complains:
>
> git apply fixup2.patch
> error: patch failed: drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:1861
> error: drivers/net/ethernet/stmicro/stmmac/stmmac_main.c: patch does not apply
>
> patch -p1 < fixup2.patch
> (Stripping trailing CRs from patch; use --binary to disable.)
> patching file drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> Hunk #6 succeeded at 2252 (offset 1 line).                               
> patch unexpectedly ends in middle of line
> patch unexpectedly ends in middle of line

I just do "git diff <file> > out.patch". Maybe not the best thing
to do then.

>
> Anyway, with this second fixup, I'm back to square one:
> I can boot but iperf3 won't hold for long
>
>
> # iperf3 -c 10.1.2.124 -p 12345 -t 600
> Connecting to host 10.1.2.124, port 12345
> [  4] local 10.1.4.59 port 38650 connected to 10.1.2.124 port 12345
> [ ID] Interval           Transfer     Bandwidth       Retr  Cwnd
> [  4]   0.00-1.00   sec  80.8 MBytes   678 Mbits/sec    1    300 KBytes
> [  4]   1.00-2.00   sec  81.1 MBytes   680 Mbits/sec    0    329 KBytes
> [  4]   2.00-3.00   sec  80.7 MBytes   677 Mbits/sec    0    335 KBytes
> [  4]   3.00-4.00   sec  81.7 MBytes   685 Mbits/sec    0    337 KBytes
> [  4]   4.00-5.00   sec  81.0 MBytes   680 Mbits/sec    0    341 KBytes
> [  4]   5.00-6.00   sec  81.0 MBytes   680 Mbits/sec    0    344 KBytes
> [  4]   6.00-7.00   sec  80.7 MBytes   677 Mbits/sec    0    345 KBytes
> [  4]   7.00-8.00   sec  81.5 MBytes   684 Mbits/sec    0    346 KBytes
> [  4]   8.00-9.00   sec  81.2 MBytes   680 Mbits/sec    0    348 KBytes
> [  4]   9.00-10.00  sec  5.59 MBytes  46.9 Mbits/sec    2   1.41 KBytes
> [  4]  10.00-11.00  sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
> [  4]  11.00-12.00  sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
> [  4]  12.00-13.00  sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
> [  4]  13.00-14.00  sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes

Okay, so this is odd because I now have a similar setup as yours
and is working perfectly fine:

---
# dmesg | grep -i stmmac
stmmaceth f0008000.ethernet: PTP uses main clock
stmmaceth f0008000.ethernet: User ID: 0x10, Synopsys ID: 0x37
stmmaceth f0008000.ethernet:    DWMAC1000
stmmaceth f0008000.ethernet: DMA HW capability register supported
stmmaceth f0008000.ethernet: RX Checksum Offload Engine supported
stmmaceth f0008000.ethernet: COE Type 2
stmmaceth f0008000.ethernet: TX Checksum insertion supported
stmmaceth f0008000.ethernet: Normal descriptors
stmmaceth f0008000.ethernet: Ring mode enabled
stmmaceth f0008000.ethernet: Enable RX Mitigation via HW Watchdog
Timer
libphy: stmmac: probed
stmmaceth f0008000.ethernet eth0: device MAC address
0e:67:f6:6c:59:c6
Micrel KSZ9031 Gigabit PHY stmmac-0:00: attached PHY driver
[Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=stmmac-0:00, irq=POLL)
stmmaceth f0008000.ethernet eth0: No Safety Features support found
stmmaceth f0008000.ethernet eth0: PTP not supported by HW
stmmaceth f0008000.ethernet eth0: Link is Up - 1Gbps/Full - flow
control off
stmmaceth f0008000.ethernet eth0: Link is Down
stmmaceth f0008000.ethernet eth0: Link is Up - 1Gbps/Full - flow
control off

---
# iperf3 -c 192.168.0.3 -t 600
Connecting to host 192.168.0.3, port 5201
[  4] local 192.168.0.1 port 46796 connected to 192.168.0.3 port 5201
[ ID] Interval           Transfer     Bandwidth       Retr  Cwnd
[  4]   0.00-1.01   sec   101 MBytes   841 Mbits/sec    1    467
KBytes
[  4]   1.01-2.00   sec   112 MBytes   945 Mbits/sec    0    475
KBytes
[  4]   2.00-3.01   sec   114 MBytes   947 Mbits/sec    0    481
KBytes
[  4]   3.01-4.00   sec   112 MBytes   945 Mbits/sec    0    486
KBytes
[  4]   4.00-5.00   sec   113 MBytes   947 Mbits/sec    0    506
KBytes
[  4]   5.00-6.01   sec   113 MBytes   947 Mbits/sec    0    520
KBytes
[  4]   6.01-7.00   sec   112 MBytes   950 Mbits/sec    0    625
KBytes
[  4]   7.00-8.01   sec   114 MBytes   948 Mbits/sec    0    625
KBytes
[  4]   8.01-9.00   sec   112 MBytes   948 Mbits/sec    0    625
KBytes
[  4]   9.00-10.00  sec   114 MBytes   955 Mbits/sec    0    998
KBytes
[  4]  10.00-11.01  sec   114 MBytes   949 Mbits/sec    0    998
KBytes
[  4]  11.01-12.00  sec   113 MBytes   949 Mbits/sec    0    998
KBytes
[  4]  12.00-13.00  sec   114 MBytes   954 Mbits/sec    0    998
KBytes
[  4]  13.00-14.00  sec   114 MBytes   950 Mbits/sec    0    998
KBytes
[  4]  14.00-15.01  sec   114 MBytes   951 Mbits/sec    0    998
KBytes
[  4]  15.01-16.00  sec   113 MBytes   951 Mbits/sec    0    998
KBytes
[  4]  16.00-17.00  sec   114 MBytes   951 Mbits/sec    0    998
KBytes
[  4]  17.00-18.01  sec   114 MBytes   951 Mbits/sec    0    998
KBytes
[  4]  18.01-19.00  sec   113 MBytes   951 Mbits/sec    0    998
KBytes
[  4]  19.00-20.00  sec   114 MBytes   951 Mbits/sec    0    998
KBytes
[  4]  20.00-21.00  sec   112 MBytes   947 Mbits/sec    0    998
KBytes

[ And just keeps going on ]

---

Note that although the User ID is not the same (0x10 vs 0x11) the
callbacks and the sw flow is the same, there are no differences
regarding User ID in stmmac driver, only the Synopsys ID is used.

Maybe some feature is not enabled in your HW ?

Thanks and Best Regards,
Jose Miguel Abreu

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

[PATCH net-next 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 1fca66ad6b17..14f890f2a970 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2224,8 +2224,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);
 	}
@@ -3015,6 +3014,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) {
@@ -3235,6 +3235,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