Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1528307 > unrolled thread

stmmac ethernet in kernel 4.4: coalescing related pauses?

Back to article view | Back to linux.kernel

Contents

  stmmac ethernet in kernel 4.4: coalescing related pauses? Pavel Machek <pavel@ucw.cz> - 2016-11-23 12:00 +0100
    stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-11-24 10:00 +0100
      Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-11-24 11:30 +0100
        Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@denx.de> - 2016-11-24 11:40 +0100
          [PATCH] stmmac ethernet: unify locking  Pavel Machek <pavel@ucw.cz> - 2016-11-24 11:50 +0100
            [PATCH] stmmac ethernet: remove cut & paste code Pavel Machek <pavel@ucw.cz> - 2016-11-24 12:10 +0100
              Re: [PATCH] stmmac ethernet: remove cut & paste code Joe Perches <joe@perches.com> - 2016-11-24 21:10 +0100
                Re: [PATCH] stmmac ethernet: remove cut & paste code Pavel Machek <pavel@ucw.cz> - 2016-11-24 23:00 +0100
                  Re: [PATCH] stmmac ethernet: remove cut & paste code Joe Perches <joe@perches.com> - 2016-11-24 23:30 +0100
                    Re: [PATCH] stmmac ethernet: remove cut & paste code Pavel Machek <pavel@ucw.cz> - 2016-11-28 13:00 +0100
                      Re: [PATCH] stmmac ethernet: remove cut & paste code Joe Perches <joe@perches.com> - 2016-11-28 15:30 +0100
                        Re: [PATCH] stmmac ethernet: remove cut & paste code Pavel Machek <pavel@ucw.cz> - 2016-11-28 15:40 +0100
                          Re: [PATCH] stmmac ethernet: remove cut & paste code Joe Perches <joe@perches.com> - 2016-11-28 17:10 +0100
      Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. David Miller <davem@davemloft.net> - 2016-11-24 17:20 +0100
        Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-11-24 22:30 +0100
          Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 09:30 +0100
            Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-02 09:50 +0100
              Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 10:50 +0100
                Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-02 13:40 +0100
                  Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 15:00 +0100
                    Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Alexandre Torgue <alexandre.torgue@st.com> - 2016-12-02 15:30 +0100
                      Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 17:10 +0100
                        Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 13:40 +0100
                    Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 13:10 +0100
                Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 11:40 +0100
                  Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. "Lino Sanfilippo" <LinoSanfilippo@gmx.de> - 2016-12-05 12:50 +0100
                    Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 23:20 +0100
                      Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-05 23:40 +0100
                        Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 23:50 +0100
                          Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-06 00:00 +0100
                            Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-06 00:20 +0100
              Aw: Re: stmmac ethernet in kernel 4.9-rc6: coalescing related  pauses. "Lino Sanfilippo" <LinoSanfilippo@gmx.de> - 2016-12-02 15:10 +0100
                [RFC] Re: Re: stmmac ethernet in kernel 4.9-rc6: coalescing related  pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-07 13:40 +0100
                  Re: [RFC] Re: Re: stmmac ethernet in kernel 4.9-rc6: coalescing  related pauses. Lino Sanfilippo <lsanfil@marvell.com> - 2016-12-07 14:20 +0100
            Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 09:50 +0100
            Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 13:00 +0100
        [PATCH] stmmac: fix comments, make debug output consistent Pavel Machek <pavel@ucw.cz> - 2016-11-28 13:00 +0100
          Re: [PATCH] stmmac: fix comments, make debug output consistent David Miller <davem@davemloft.net> - 2016-11-30 02:00 +0100
        Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-11-28 13:20 +0100
        [PATCH] stmmac: reduce code duplication getting basic descriptors Pavel Machek <pavel@ucw.cz> - 2016-11-28 13:20 +0100
          Re: [PATCH] stmmac: reduce code duplication getting basic descriptors Alexandre Torgue <alexandre.torgue@st.com> - 2016-12-02 15:10 +0100
        [PATCH] stmmac: simplify flag assignment Pavel Machek <pavel@ucw.cz> - 2016-11-30 12:50 +0100
          Re: [PATCH] stmmac: simplify flag assignment David Miller <davem@davemloft.net> - 2016-12-01 21:30 +0100
            stmmac: turn coalescing / NAPI off in stmmac Pavel Machek <pavel@ucw.cz> - 2016-12-01 23:50 +0100
              Re: stmmac: turn coalescing / NAPI off in stmmac Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 09:50 +0100
                Re: stmmac: turn coalescing / NAPI off in stmmac Pavel Machek <pavel@ucw.cz> - 2016-12-02 11:50 +0100
                  Re: stmmac: turn coalescing / NAPI off in stmmac Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 17:10 +0100
                    Re: stmmac: turn coalescing / NAPI off in stmmac Pavel Machek <pavel@ucw.cz> - 2016-12-05 12:50 +0100
          Re: [PATCH] stmmac: simplify flag assignment Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 09:30 +0100
        [PATCH] stmmac: cleanup documenation, make it match reality Pavel Machek <pavel@ucw.cz> - 2016-12-01 11:40 +0100
          Re: [PATCH] stmmac: cleanup documenation, make it match reality David Miller <davem@davemloft.net> - 2016-12-03 21:10 +0100
        [PATCH] stmmac: disable tx coalescing Pavel Machek <pavel@ucw.cz> - 2016-12-05 13:30 +0100
    Re: stmmac ethernet in kernel 4.4: coalescing related pauses? Lino Sanfilippo <lsanfil@marvell.com> - 2016-11-28 14:10 +0100
      Re: stmmac ethernet in kernel 4.4: coalescing related pauses? David Miller <davem@davemloft.net> - 2016-11-28 16:00 +0100
        Re: stmmac ethernet in kernel 4.4: coalescing related pauses? Lino Sanfilippo <lsanfil@marvell.com> - 2016-11-28 16:40 +0100
        Re: stmmac ethernet in kernel 4.4: coalescing related pauses? Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-28 16:40 +0100
          Re: stmmac ethernet in kernel 4.4: coalescing related pauses? Lino Sanfilippo <lsanfil@marvell.com> - 2016-11-28 17:00 +0100
            Re: stmmac ethernet in kernel 4.4: coalescing related pauses? David Miller <davem@davemloft.net> - 2016-11-28 17:40 +0100
              Re: stmmac ethernet in kernel 4.4: coalescing related pauses? Lino Sanfilippo <lsanfil@marvell.com> - 2016-11-28 18:10 +0100
          Re: stmmac ethernet in kernel 4.4: coalescing related pauses? Pavel Machek <pavel@ucw.cz> - 2016-11-30 11:30 +0100

Page 1 of 3  [1] 2 3  Next page →

#1528307 — stmmac ethernet in kernel 4.4: coalescing related pauses?

[Multipart message — attachments visible in raw view] — view raw

Hi!

I'm debugging strange delays during transmit in stmmac driver. They
seem to be present in 4.4 kernel (and older kernels, too). Workload is
burst of udp packets being sent, pause, burst of udp packets, ...

Test code is attached, I use these parameters for testing:

./udp-test raw 10.0.0.6 1234 1000 100 30

The delays seem to be related to coalescing:

drivers/net/ethernet/stmicro/stmmac/common.h
#define STMMAC_COAL_TX_TIMER    40000
#define STMMAC_MAX_COAL_TX_TICK 100000
#define STMMAC_TX_MAX_FRAMES    256

If I lower the parameters, delays are gone, but I get netdev watchdog
backtrace followed by broken driver.

Any ideas what is going on there?

[I'm currently trying to get newer kernels working on affected
hardware.]

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1529076 — stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

[Multipart message — attachments visible in raw view] — view raw

Hi!

> I'm debugging strange delays during transmit in stmmac driver. They
> seem to be present in 4.4 kernel (and older kernels, too). Workload is
> burst of udp packets being sent, pause, burst of udp packets, ...
> 
> Test code is attached, I use these parameters for testing:
> 
> ./udp-test raw 10.0.0.6 1234 1000 100 30
> 
> The delays seem to be related to coalescing:
> 
> drivers/net/ethernet/stmicro/stmmac/common.h
> #define STMMAC_COAL_TX_TIMER    40000
> #define STMMAC_MAX_COAL_TX_TICK 100000
> #define STMMAC_TX_MAX_FRAMES    256
> 
> If I lower the parameters, delays are gone, but I get netdev watchdog
> backtrace followed by broken driver.
> 
> Any ideas what is going on there?

4.9-rc6 still has the delays. With the

#define STMMAC_COAL_TX_TIMER 1000
#define STMMAC_TX_MAX_FRAMES 2

settings, delays go away, and driver still works. (It fails fairly
fast in 4.4). Good news. But the question still is: what is going on
there?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1529142 — Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

[Multipart message — attachments visible in raw view] — view raw

Hi!

What is going on with stmmac_tso_xmit() vs. stmmac_xmit()? One seems
to be copy of another, with subtle differences -- like calling
netif_queue_stopped() under spin_lock(&priv->tx_lock), or not.

What is going on with all these likely()s? Likely new hardware owners
will not be happy... or anyone running a lot of jumbo frames. (Perhaps
CPU's branch prediction can do better job here, without explicit hints?)

     if (unlikely(is_jumbo) && likely(priv->synopsys_id <
                                              DWMAC_CORE_4_00)) {

Are you sure this is okay?

        if (unlikely(netif_queue_stopped(priv->dev) &&
                     stmmac_tx_avail(priv) > STMMAC_TX_THRESH)) {
		netif_tx_lock(priv->dev);
                if (netif_queue_stopped(priv->dev) &&
                    stmmac_tx_avail(priv) > STMMAC_TX_THRESH) {

---

Fix english in comments.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 1f9ec02..e5a5a05 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1747,11 +1747,11 @@ static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
 	if (priv->hw->pcs && priv->hw->mac->pcs_ctrl_ane)
 		priv->hw->mac->pcs_ctrl_ane(priv->hw, 1, priv->hw->ps, 0);
 
-	/*  set TX ring length */
+	/* Set TX ring length */
 	if (priv->hw->dma->set_tx_ring_len)
 		priv->hw->dma->set_tx_ring_len(priv->ioaddr,
 					       (DMA_TX_SIZE - 1));
-	/*  set RX ring length */
+	/* Set RX ring length */
 	if (priv->hw->dma->set_rx_ring_len)
 		priv->hw->dma->set_rx_ring_len(priv->ioaddr,
 					       (DMA_RX_SIZE - 1));
@@ -2212,7 +2212,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	priv->tx_skbuff[first_entry] = skb;
 
 	enh_desc = priv->plat->enh_desc;
-	/* To program the descriptors according to the size of the frame */
+	/* Program the descriptors according to the size of the frame */
 	if (enh_desc)
 		is_jumbo = priv->hw->mode->is_jumbo_frm(skb->len, enh_desc);
 
@@ -2665,7 +2665,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
  *  @budget : maximum number of packets that the current CPU can receive from
  *	      all interfaces.
  *  Description :
- *  To look at the incoming frames and clear the tx resources.
+ *  Look at the incoming frames and clear the tx resources.
  */
 static int stmmac_poll(struct napi_struct *napi, int budget)
 {
@@ -2828,7 +2828,7 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
 		return IRQ_NONE;
 	}
 
-	/* To handle GMAC own interrupts */
+	/* Handle GMAC own interrupts */
 	if ((priv->plat->has_gmac) || (priv->plat->has_gmac4)) {
 		int status = priv->hw->mac->host_irq_status(priv->hw,
 							    &priv->xstats);
@@ -2853,7 +2853,7 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
 		}
 	}
 
-	/* To handle DMA interrupts */
+	/* Handle DMA interrupts */
 	stmmac_dma_interrupt(priv);
 
 	return IRQ_HANDLED;
@@ -3145,7 +3145,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 
 	priv->hw = mac;
 
-	/* To use the chained or ring mode */
+	/* Use the chained or ring mode */
 	if (priv->synopsys_id >= DWMAC_CORE_4_00) {
 		priv->hw->mode = &dwmac4_ring_mode_ops;
 	} else {
@@ -3191,7 +3191,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 	} else
 		pr_info(" No HW DMA feature register supported");
 
-	/* To use alternate (extended), normal or GMAC4 descriptor structures */
+	/* Use alternate (extended), normal or GMAC4 descriptor structures */
 	if (priv->synopsys_id >= DWMAC_CORE_4_00)
 		priv->hw->desc = &dwmac4_desc_ops;
 	else



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1529148 — Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

[Multipart message — attachments visible in raw view] — view raw

Hi!

> What is going on with all these likely()s? Likely new hardware owners
> will not be happy... or anyone running a lot of jumbo frames. (Perhaps
> CPU's branch prediction can do better job here, without explicit hints?)
> 
>      if (unlikely(is_jumbo) && likely(priv->synopsys_id <
>                                               DWMAC_CORE_4_00)) {

    Fix english, remove misleading unlikely's.

Signed-off-by: Pavel Machek <pavel@denx.de>

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e5a5a05..0363db3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2003,7 +2003,7 @@ 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);
 
-	/* Desc availability based on threshold should be enough safe */
+	/* Desc availability based on threshold should be safe enough */
 	if (unlikely(stmmac_tx_avail(priv) <
 		(((skb->len - proto_hdr_len) / TSO_MAX_BUFF_SIZE + 1)))) {
 		if (!netif_queue_stopped(dev)) {
@@ -2216,8 +2216,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (enh_desc)
 		is_jumbo = priv->hw->mode->is_jumbo_frm(skb->len, enh_desc);
 
-	if (unlikely(is_jumbo) && likely(priv->synopsys_id <
-					 DWMAC_CORE_4_00)) {
+	if (unlikely(is_jumbo) && priv->synopsys_id < DWMAC_CORE_4_00) {
 		entry = priv->hw->mode->jumbo_frm(priv, skb, csum_insertion);
 		if (unlikely(entry < 0))
 			goto dma_map_err;
@@ -2242,7 +2241,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
 		priv->tx_skbuff[entry] = NULL;
 
-		if (unlikely(priv->synopsys_id >= DWMAC_CORE_4_00)) {
+		if (priv->synopsys_id >= DWMAC_CORE_4_00) {
 			desc->des0 = des;
 			priv->tx_skbuff_dma[entry].buf = desc->des0;
 		} else {
@@ -2319,7 +2318,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 		if (dma_mapping_error(priv->device, des))
 			goto dma_map_err;
 
-		if (unlikely(priv->synopsys_id >= DWMAC_CORE_4_00)) {
+		if (priv->synopsys_id >= DWMAC_CORE_4_00) {
 			first->des0 = des;
 			priv->tx_skbuff_dma[first_entry].buf = first->des0;
 		} else {
@@ -2438,7 +2437,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv)
 				break;
 			}
 
-			if (unlikely(priv->synopsys_id >= DWMAC_CORE_4_00)) {
+			if (priv->synopsys_id >= DWMAC_CORE_4_00) {
 				p->des0 = priv->rx_skbuff_dma[entry];
 				p->des1 = 0;
 			} else {
@@ -2455,7 +2454,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv)
 		}
 		wmb();
 
-		if (unlikely(priv->synopsys_id >= DWMAC_CORE_4_00))
+		if (priv->synopsys_id >= DWMAC_CORE_4_00)
 			priv->hw->desc->init_rx_desc(p, priv->use_riwt, 0, 0);
 		else
 			priv->hw->desc->set_rx_owner(p);
@@ -2545,7 +2544,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
 			int frame_len;
 			unsigned int des;
 
-			if (unlikely(priv->synopsys_id >= DWMAC_CORE_4_00))
+			if (priv->synopsys_id >= DWMAC_CORE_4_00)
 				des = p->des0;
 			else
 				des = p->des2;


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1529152 — [PATCH] stmmac ethernet: unify locking

[Multipart message — attachments visible in raw view] — view raw

Make locking match in both _xmit functions.
   
Signed-off-by: Pavel Machek <pavel@denx.de>

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 0363db3..1cff258 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2185,12 +2185,12 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	spin_lock(&priv->tx_lock);
 
 	if (unlikely(stmmac_tx_avail(priv) < nfrags + 1)) {
-		spin_unlock(&priv->tx_lock);
 		if (!netif_queue_stopped(dev)) {
 			netif_stop_queue(dev);
 			/* This is a hard error, log it. */
 			pr_err("%s: Tx Ring full when queue awake\n", __func__);
 		}
+		spin_unlock(&priv->tx_lock);
 		return NETDEV_TX_BUSY;
 	}
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1529176 — [PATCH] stmmac ethernet: remove cut & paste code

[Multipart message — attachments visible in raw view] — view raw

Remove duplicate code from _tx routines.
    
Signed-off-by: Pavel Machek <pavel@denx.de>

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 1cff258..5cf9cef 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1960,6 +1960,38 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
 	}
 }
 
+static void stmmac_xmit_common(struct sk_buff *skb, struct net_device *dev, int nfrags, struct dma_desc *desc)
+{
+	struct stmmac_priv *priv = netdev_priv(dev);
+
+	if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
+		if (netif_msg_hw(priv))
+			pr_debug("%s: stop transmitted packets\n", __func__);
+		netif_stop_queue(dev);
+	}
+
+	dev->stats.tx_bytes += skb->len;
+
+	/* According to the coalesce parameter the IC bit for the latest
+	 * segment is reset and the timer re-started to clean the tx status.
+	 * 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;
+		priv->hw->desc->set_tx_ic(desc);
+		priv->xstats.tx_set_ic_bit++;
+	}
+
+	if (!priv->hwts_tx_en)
+		skb_tx_timestamp(skb);
+}
+
+
 /**
  *  stmmac_tso_xmit - Tx entry point of the driver for oversized frames (TSO)
  *  @skb : the socket buffer
@@ -2081,30 +2113,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	priv->cur_tx = STMMAC_GET_ENTRY(priv->cur_tx, DMA_TX_SIZE);
 
-	if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
-		if (netif_msg_hw(priv))
-			pr_debug("%s: stop transmitted packets\n", __func__);
-		netif_stop_queue(dev);
-	}
-
-	dev->stats.tx_bytes += skb->len;
+	stmmac_xmit_common(skb, dev, nfrags, desc);
+	
 	priv->xstats.tx_tso_frames++;
 	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;
-		priv->hw->desc->set_tx_ic(desc);
-		priv->xstats.tx_set_ic_bit++;
-	}
-
-	if (!priv->hwts_tx_en)
-		skb_tx_timestamp(skb);
-
 	if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
 		     priv->hwts_tx_en)) {
 		/* declare that device is doing timestamping */
@@ -2280,31 +2293,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 		print_pkt(skb->data, skb->len);
 	}
 
-	if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
-		if (netif_msg_hw(priv))
-			pr_debug("%s: stop transmitted packets\n", __func__);
-		netif_stop_queue(dev);
-	}
-
-	dev->stats.tx_bytes += skb->len;
-
-	/* According to the coalesce parameter the IC bit for the latest
-	 * segment is reset and the timer re-started to clean the tx status.
-	 * 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;
-		priv->hw->desc->set_tx_ic(desc);
-		priv->xstats.tx_set_ic_bit++;
-	}
-
-	if (!priv->hwts_tx_en)
-		skb_tx_timestamp(skb);
+	stmmac_xmit_common(skb, dev, nfrags, desc);
 
 	/* Ready to fill the first descriptor and set the OWN bit w/o any
 	 * problems because all the descriptors are actually ready to be


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1529671 — Re: [PATCH] stmmac ethernet: remove cut & paste code

On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote:
> Remove duplicate code from _tx routines.

trivia:

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[]
> @@ -1960,6 +1960,38 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
>  	}
>  }
>  
> +static void stmmac_xmit_common(struct sk_buff *skb, struct net_device *dev, int nfrags, struct dma_desc *desc)
> +{
> +	struct stmmac_priv *priv = netdev_priv(dev);
> +
> +	if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
> +		if (netif_msg_hw(priv))
> +			pr_debug("%s: stop transmitted packets\n", __func__);

		netif_dbg(priv, hw, dev, "%s: stop transmitted packets\n",
			  __func__);

[toc] | [prev] | [next] | [standalone]

#1529716 — Re: [PATCH] stmmac ethernet: remove cut & paste code

[Multipart message — attachments visible in raw view] — view raw

On Thu 2016-11-24 12:05:25, Joe Perches wrote:
> On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote:
> > Remove duplicate code from _tx routines.
> 
> trivia:
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> []
> > @@ -1960,6 +1960,38 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
> >  	}
> >  }
> >  
> > +static void stmmac_xmit_common(struct sk_buff *skb, struct net_device *dev, int nfrags, struct dma_desc *desc)
> > +{
> > +	struct stmmac_priv *priv = netdev_priv(dev);
> > +
> > +	if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
> > +		if (netif_msg_hw(priv))
> > +			pr_debug("%s: stop transmitted packets\n", __func__);
> 
> 		netif_dbg(priv, hw, dev, "%s: stop transmitted packets\n",
> 			  __func__);

Not now. Modifying the code while de-duplicating would be bad idea.

(And it looks like the driver has rather more serious problems than
printk style...)

Thanks,									
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1529722 — Re: [PATCH] stmmac ethernet: remove cut & paste code

On Thu, 2016-11-24 at 22:44 +0100, Pavel Machek wrote:
> On Thu 2016-11-24 12:05:25, Joe Perches wrote:
> > On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote:
> > > Remove duplicate code from _tx routines.
> > 
> > trivia:
> > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > 
> > []
> > > @@ -1960,6 +1960,38 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
> > >  	}
> > >  }
> > >  
> > > +static void stmmac_xmit_common(struct sk_buff *skb, struct net_device *dev, int nfrags, struct dma_desc *desc)
> > > +{
> > > +	struct stmmac_priv *priv = netdev_priv(dev);
> > > +
> > > +	if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
> > > +		if (netif_msg_hw(priv))
> > > +			pr_debug("%s: stop transmitted packets\n", __func__);
> > 
> > 		netif_dbg(priv, hw, dev, "%s: stop transmitted packets\n",
> > 			  __func__);
> 
> Not now. Modifying the code while de-duplicating would be bad idea.

Too many people think overly granular patches are the
best and only way to make changes.

Deduplication and consolidation can happen simultaneously.

> (And it looks like the driver has rather more serious problems than
> printk style...)

Probably so.

cheers, Joe

[toc] | [prev] | [next] | [standalone]

#1531244 — Re: [PATCH] stmmac ethernet: remove cut & paste code

[Multipart message — attachments visible in raw view] — view raw

On Thu 2016-11-24 14:27:13, Joe Perches wrote:
> On Thu, 2016-11-24 at 22:44 +0100, Pavel Machek wrote:
> > On Thu 2016-11-24 12:05:25, Joe Perches wrote:
> > > On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote:
> > > > Remove duplicate code from _tx routines.
> > > 
> > > trivia:
> > > 
> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > 
> > > []
> > > > @@ -1960,6 +1960,38 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
> > > >  	}
> > > >  }
> > > >  
> > > > +static void stmmac_xmit_common(struct sk_buff *skb, struct net_device *dev, int nfrags, struct dma_desc *desc)
> > > > +{
> > > > +	struct stmmac_priv *priv = netdev_priv(dev);
> > > > +
> > > > +	if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
> > > > +		if (netif_msg_hw(priv))
> > > > +			pr_debug("%s: stop transmitted packets\n", __func__);
> > > 
> > > 		netif_dbg(priv, hw, dev, "%s: stop transmitted packets\n",
> > > 			  __func__);
> > 
> > Not now. Modifying the code while de-duplicating would be bad idea.
> 
> Too many people think overly granular patches are the
> best and only way to make changes.

> Deduplication and consolidation can happen simultaneously.

Can, but should not at this point. Please take a look at the driver in
question before commenting on trivial printk style.

Feel free to do your favourite cleanup on whole tree, or per-driver
basis. Doing it on per-message basis would be wrong thing to do.

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1531352 — Re: [PATCH] stmmac ethernet: remove cut & paste code

On Mon, 2016-11-28 at 12:50 +0100, Pavel Machek wrote:
> On Thu 2016-11-24 14:27:13, Joe Perches wrote:
> > On Thu, 2016-11-24 at 22:44 +0100, Pavel Machek wrote:
> > > On Thu 2016-11-24 12:05:25, Joe Perches wrote:
> > > > On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote:
> > > > > Remove duplicate code from _tx routines.
> > > > 
> > > > trivia:
> > > > 
> > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > 
> > > > []
> > > > > @@ -1960,6 +1960,38 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +static void stmmac_xmit_common(struct sk_buff *skb, struct net_device *dev, int nfrags, struct dma_desc *desc)
> > > > > +{
> > > > > +	struct stmmac_priv *priv = netdev_priv(dev);
> > > > > +
> > > > > +	if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
> > > > > +		if (netif_msg_hw(priv))
> > > > > +			pr_debug("%s: stop transmitted packets\n", __func__);
> > > > 
> > > > 		netif_dbg(priv, hw, dev, "%s: stop transmitted packets\n",
> > > > 			  __func__);
> > > 
> > > Not now. Modifying the code while de-duplicating would be bad idea.
> > 
> > Too many people think overly granular patches are the
> > best and only way to make changes.
> > Deduplication and consolidation can happen simultaneously.
> 
> Can, but should not at this point. Please take a look at the driver in
> question before commenting on trivial printk style.

I had.

It's perfectly acceptable and already uses netif_<level> properly.

This consolidation now introduces the _only_ instance where it is
now improperly using a netif_msg_<type> then single pr_<level>
function sequence that should be consolidated into netif_dbg.

Every other use of netif_msg_<level> then either emits multiple
lines or is used in an if/else.

cheers, Joe

[toc] | [prev] | [next] | [standalone]

#1531363 — Re: [PATCH] stmmac ethernet: remove cut & paste code

[Multipart message — attachments visible in raw view] — view raw

On Mon 2016-11-28 06:24:28, Joe Perches wrote:
> On Mon, 2016-11-28 at 12:50 +0100, Pavel Machek wrote:
> > On Thu 2016-11-24 14:27:13, Joe Perches wrote:
> > > On Thu, 2016-11-24 at 22:44 +0100, Pavel Machek wrote:
> > > > On Thu 2016-11-24 12:05:25, Joe Perches wrote:
> > > > > On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote:
> > > > > > Remove duplicate code from _tx routines.
> > > > > 
> > > > > trivia:
> > > > > 
> > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > 
> > > > > []
> > > > > > @@ -1960,6 +1960,38 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
> > > > > >  	}
> > > > > >  }
> > > > > >  
> > > > > > +static void stmmac_xmit_common(struct sk_buff *skb, struct net_device *dev, int nfrags, struct dma_desc *desc)
> > > > > > +{
> > > > > > +	struct stmmac_priv *priv = netdev_priv(dev);
> > > > > > +
> > > > > > +	if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
> > > > > > +		if (netif_msg_hw(priv))
> > > > > > +			pr_debug("%s: stop transmitted packets\n", __func__);
> > > > > 
> > > > > 		netif_dbg(priv, hw, dev, "%s: stop transmitted packets\n",
> > > > > 			  __func__);
> > > > 
> > > > Not now. Modifying the code while de-duplicating would be bad idea.
> > > 
> > > Too many people think overly granular patches are the
> > > best and only way to make changes.
> > > Deduplication and consolidation can happen simultaneously.
> > 
> > Can, but should not at this point. Please take a look at the driver in
> > question before commenting on trivial printk style.
> 
> I had.
> 
> It's perfectly acceptable and already uses netif_<level> properly.
> 
> This consolidation now introduces the _only_ instance where it is
> now improperly using a netif_msg_<type> then single pr_<level>
> function sequence that should be consolidated into netif_dbg.

> Every other use of netif_msg_<level> then either emits multiple
> lines or is used in an if/else.

Are you looking at right driver? I don't see single use of
netif_msg_<level>, but see this at stmmac_main.c:756. Code is actually
pretty consistent using pr_*.

                                if (netif_msg_link(priv))
                                        pr_warn("%s: Speed (%d) not 10/100\n",
                                                dev->name, phydev->speed);

Anyway, I'm moving code around, if you want to do trivial cleanups, do
them yourself.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1531417 — Re: [PATCH] stmmac ethernet: remove cut & paste code

On Mon, 2016-11-28 at 15:35 +0100, Pavel Machek wrote:
> On Mon 2016-11-28 06:24:28, Joe Perches wrote:
> > On Mon, 2016-11-28 at 12:50 +0100, Pavel Machek wrote:
> > > On Thu 2016-11-24 14:27:13, Joe Perches wrote:
> > > > On Thu, 2016-11-24 at 22:44 +0100, Pavel Machek wrote:
> > > > > On Thu 2016-11-24 12:05:25, Joe Perches wrote:
> > > > > > On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote:
> > > > > > > Remove duplicate code from _tx routines.
> > > > > > 
> > > > > > trivia:
> > > > > > 
> > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > > 
> > > > > > []
> > > > > > > @@ -1960,6 +1960,38 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
> > > > > > >  	}
> > > > > > >  }
> > > > > > >  
> > > > > > > +static void stmmac_xmit_common(struct sk_buff *skb, struct net_device *dev, int nfrags, struct dma_desc *desc)
> > > > > > > +{
> > > > > > > +	struct stmmac_priv *priv = netdev_priv(dev);
> > > > > > > +
> > > > > > > +	if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
> > > > > > > +		if (netif_msg_hw(priv))
> > > > > > > +			pr_debug("%s: stop transmitted packets\n", __func__);
> > > > > > 
> > > > > > 		netif_dbg(priv, hw, dev, "%s: stop transmitted packets\n",
> > > > > > 			  __func__);
> > > > > 
> > > > > Not now. Modifying the code while de-duplicating would be bad idea.
> > > > 
> > > > Too many people think overly granular patches are the
> > > > best and only way to make changes.
> > > > Deduplication and consolidation can happen simultaneously.
> > > 
> > > Can, but should not at this point. Please take a look at the driver in
> > > question before commenting on trivial printk style.
> > 
> > I had.
> > 
> > It's perfectly acceptable and already uses netif_<level> properly.
> > 
> > This consolidation now introduces the _only_ instance where it is
> > now improperly using a netif_msg_<type> then single pr_<level>
> > function sequence that should be consolidated into netif_dbg.
> > Every other use of netif_msg_<level> then either emits multiple
> > lines or is used in an if/else.
> 
> Are you looking at right driver?

Yes and I think you should make changes against -next
and not Linus' where this is:

b3e51069627e2 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c (LABBE Corentin            2016-11-16 20:09:41 +0100  755)                              netif_warn(priv, link, priv->dev,
b3e51069627e2 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c (LABBE Corentin            2016-11-16 20:09:41 +0100  756)                                         "Speed (%d) not 10/100\n",
b3e51069627e2 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c (LABBE Corentin            2016-11-16 20:09:41 +0100  757)                                         phydev->speed);

>  I don't see single use of
> netif_msg_<level>, but see this at stmmac_main.c:756. Code is actually
> pretty consistent using pr_*.
> 
>                                 if (netif_msg_link(priv))
>                                         pr_warn("%s: Speed (%d) not 10/100\n",
>                                                 dev->name, phydev->speed);
> 
> Anyway, I'm moving code around, if you want to do trivial cleanups, do
> them yourself.

cheers, Joe

[toc] | [prev] | [next] | [standalone]

#1529537 — Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

From: Pavel Machek <pavel@ucw.cz>
Date: Thu, 24 Nov 2016 09:55:06 +0100

> Hi!
> 
>> I'm debugging strange delays during transmit in stmmac driver. They
>> seem to be present in 4.4 kernel (and older kernels, too). Workload is
>> burst of udp packets being sent, pause, burst of udp packets, ...
>> 
>> Test code is attached, I use these parameters for testing:
>> 
>> ./udp-test raw 10.0.0.6 1234 1000 100 30
>> 
>> The delays seem to be related to coalescing:
>> 
>> drivers/net/ethernet/stmicro/stmmac/common.h
>> #define STMMAC_COAL_TX_TIMER    40000
>> #define STMMAC_MAX_COAL_TX_TICK 100000
>> #define STMMAC_TX_MAX_FRAMES    256
>> 
>> If I lower the parameters, delays are gone, but I get netdev watchdog
>> backtrace followed by broken driver.
>> 
>> Any ideas what is going on there?
> 
> 4.9-rc6 still has the delays. With the
> 
> #define STMMAC_COAL_TX_TIMER 1000
> #define STMMAC_TX_MAX_FRAMES 2
> 
> settings, delays go away, and driver still works. (It fails fairly
> fast in 4.4). Good news. But the question still is: what is going on
> there?

256 packets looks way too large for being a trigger for aborting the
TX coalescing timer.

Looking more deeply into this, the driver is using non-highres timers
to implement the TX coalescing.  This simply cannot work.

1 HZ, which is the lowest granularity of non-highres timers in the
kernel, is variable as well as already too large of a delay for
effective TX coalescing.

I seriously think that the TX coalescing support should be ripped out
or disabled entirely until it is implemented properly in this driver.

[toc] | [prev] | [next] | [standalone]

#1529711 — Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >> I'm debugging strange delays during transmit in stmmac driver. They
> >> seem to be present in 4.4 kernel (and older kernels, too). Workload is
> >> burst of udp packets being sent, pause, burst of udp packets, ...
...
> > 4.9-rc6 still has the delays. With the
> > 
> > #define STMMAC_COAL_TX_TIMER 1000
> > #define STMMAC_TX_MAX_FRAMES 2
> > 
> > settings, delays go away, and driver still works. (It fails fairly
> > fast in 4.4). Good news. But the question still is: what is going on
> > there?
> 
> 256 packets looks way too large for being a trigger for aborting the
> TX coalescing timer.
> 
> Looking more deeply into this, the driver is using non-highres timers
> to implement the TX coalescing.  This simply cannot work.
> 
> 1 HZ, which is the lowest granularity of non-highres timers in the
> kernel, is variable as well as already too large of a delay for
> effective TX coalescing.
> 
> I seriously think that the TX coalescing support should be ripped out
> or disabled entirely until it is implemented properly in this
> driver.

Ok, I'd disable coalescing, but could not figure it out till. What is
generic way to do that?

It seems only thing stmmac_tx_timer() does is calling
stmmac_tx_clean(), which reclaims tx_skbuff[] entries. It should be
possible to do that explicitely, without delay, but it stops working
completely if I attempt to do that.

On a side note, stmmac_poll() does stmmac_enable_dma_irq() while
stmmac_dma_interrupt() disables interrupts. But I don't see any
protection between the two, so IMO it could race and we'd end up
without polling or interrupts...

Thanks and best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1534736 — Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

Hello


On 11/24/2016 10:25 PM, Pavel Machek wrote:
> Hi!
>
>>>> I'm debugging strange delays during transmit in stmmac driver. They
>>>> seem to be present in 4.4 kernel (and older kernels, too). Workload is
>>>> burst of udp packets being sent, pause, burst of udp packets, ...
> ...
>>> 4.9-rc6 still has the delays. With the
>>>
>>> #define STMMAC_COAL_TX_TIMER 1000
>>> #define STMMAC_TX_MAX_FRAMES 2
>>>
>>> settings, delays go away, and driver still works. (It fails fairly
>>> fast in 4.4). Good news. But the question still is: what is going on
>>> there?
>>
>> 256 packets looks way too large for being a trigger for aborting the
>> TX coalescing timer.
>>
>> Looking more deeply into this, the driver is using non-highres timers
>> to implement the TX coalescing.  This simply cannot work.
>>
>> 1 HZ, which is the lowest granularity of non-highres timers in the
>> kernel, is variable as well as already too large of a delay for
>> effective TX coalescing.
>>
>> I seriously think that the TX coalescing support should be ripped out
>> or disabled entirely until it is implemented properly in this
>> driver.
>
> Ok, I'd disable coalescing, but could not figure it out till. What is
> generic way to do that?
>
> It seems only thing stmmac_tx_timer() does is calling
> stmmac_tx_clean(), which reclaims tx_skbuff[] entries. It should be
> possible to do that explicitely, without delay, but it stops working
> completely if I attempt to do that.
>
> On a side note, stmmac_poll() does stmmac_enable_dma_irq() while
> stmmac_dma_interrupt() disables interrupts. But I don't see any
> protection between the two, so IMO it could race and we'd end up
> without polling or interrupts...


the idea behind the TX mitigation is to mix the interrupt and
timer and this approach gave us real benefit in terms
of performances and CPU usage (especially on SH4-200/SH4-300 platforms
based).
In the ring, some descriptors can raise the irq (according to a
threshold) and set the IC bit. In this path, the NAPI  poll will be
scheduled.
But there is a timer that can run (and we experimented that no high
resolution is needed) to clear the tx resources.
Concerning the lock protection, we had reviewed long time ago and
IIRC, no raise condition should be present. Open to review it, again!

So, welcome any other schema and testing on platforms supported.


Hoping this summary can help.

Peppe


>
> Thanks and best regards,
> 									Pavel
>

[toc] | [prev] | [next] | [standalone]

#1534741 — Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >>1 HZ, which is the lowest granularity of non-highres timers in the
> >>kernel, is variable as well as already too large of a delay for
> >>effective TX coalescing.
> >>
> >>I seriously think that the TX coalescing support should be ripped out
> >>or disabled entirely until it is implemented properly in this
> >>driver.
> >
> >Ok, I'd disable coalescing, but could not figure it out till. What is
> >generic way to do that?
> >
> >It seems only thing stmmac_tx_timer() does is calling
> >stmmac_tx_clean(), which reclaims tx_skbuff[] entries. It should be
> >possible to do that explicitely, without delay, but it stops working
> >completely if I attempt to do that.
> >
> >On a side note, stmmac_poll() does stmmac_enable_dma_irq() while
> >stmmac_dma_interrupt() disables interrupts. But I don't see any
> >protection between the two, so IMO it could race and we'd end up
> >without polling or interrupts...
> 
> 
> the idea behind the TX mitigation is to mix the interrupt and
> timer and this approach gave us real benefit in terms
> of performances and CPU usage (especially on SH4-200/SH4-300 platforms
> based).

Well, if you have a workload that sends and receive packets, it tends
to work ok, as you do tx_clean() in stmmac_poll(). My workload is not
like that -- it is "sending packets at 3MB/sec, receiving none". So
the stmmac_tx_timer() is rescheduled and rescheduled and rescheduled,
and then we run out of transmit descriptors, and then 40msec passes,
and then we clean them. Bad.

And that's why low-res timers do not cut it.

> In the ring, some descriptors can raise the irq (according to a
> threshold) and set the IC bit. In this path, the NAPI  poll will be
> scheduled.

Not NAPI poll but stmmac_tx_timer(), right? 

> But there is a timer that can run (and we experimented that no high
> resolution is needed) to clear the tx resources.
> Concerning the lock protection, we had reviewed long time ago and
> IIRC, no raise condition should be present. Open to review it,
> again!

Well, I certainly like the fact that we are talking :-).

And yes, I have some questions.

There's nothing that protect stmmac_poll() from running concurently
with stmmac_dma_interrupt(), right?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1534775 — Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

Hi Pavel

On 12/2/2016 9:45 AM, Pavel Machek wrote:
> Hi!
>
>>>> 1 HZ, which is the lowest granularity of non-highres timers in the
>>>> kernel, is variable as well as already too large of a delay for
>>>> effective TX coalescing.
>>>>
>>>> I seriously think that the TX coalescing support should be ripped out
>>>> or disabled entirely until it is implemented properly in this
>>>> driver.
>>>
>>> Ok, I'd disable coalescing, but could not figure it out till. What is
>>> generic way to do that?
>>>
>>> It seems only thing stmmac_tx_timer() does is calling
>>> stmmac_tx_clean(), which reclaims tx_skbuff[] entries. It should be
>>> possible to do that explicitely, without delay, but it stops working
>>> completely if I attempt to do that.
>>>
>>> On a side note, stmmac_poll() does stmmac_enable_dma_irq() while
>>> stmmac_dma_interrupt() disables interrupts. But I don't see any
>>> protection between the two, so IMO it could race and we'd end up
>>> without polling or interrupts...
>>
>>
>> the idea behind the TX mitigation is to mix the interrupt and
>> timer and this approach gave us real benefit in terms
>> of performances and CPU usage (especially on SH4-200/SH4-300 platforms
>> based).
>
> Well, if you have a workload that sends and receive packets, it tends
> to work ok, as you do tx_clean() in stmmac_poll(). My workload is not
> like that -- it is "sending packets at 3MB/sec, receiving none". So
> the stmmac_tx_timer() is rescheduled and rescheduled and rescheduled,
> and then we run out of transmit descriptors, and then 40msec passes,
> and then we clean them. Bad.
>
> And that's why low-res timers do not cut it.

in that case, I expect that the tuning of the driver could help you.
I mean, by using ethtool, it could be enough to set the IC bit on all
the descriptors. You should touch the tx_coal_frames.

Then you can use ethtool -S to monitor the status.

We had experimented this tuning on STB IP where just datagrams
had to send externally. To be honest, although we had seen
better results w/o any timer, we kept this approach enabled
because the timer was fast enough to cover our tests on SH4 boxes.

FYI, stmmac doesn't implement adaptive algo.

>
>> In the ring, some descriptors can raise the irq (according to a
>> threshold) and set the IC bit. In this path, the NAPI  poll will be
>> scheduled.
>
> Not NAPI poll but stmmac_tx_timer(), right?

in the xmit according the the threshold the timer is started or the
interrupt is set inside the descriptor.
Then stmmac_tx_clean will be always called and, if you see the flow,
no irqlock protection is needed!

>
>> But there is a timer that can run (and we experimented that no high
>> resolution is needed) to clear the tx resources.
>> Concerning the lock protection, we had reviewed long time ago and
>> IIRC, no raise condition should be present. Open to review it,
>> again!
>
> Well, I certainly like the fact that we are talking :-).
>
> And yes, I have some questions.
>
> There's nothing that protect stmmac_poll() from running concurently
> with stmmac_dma_interrupt(), right?

This is not necessary.

Best Regards
peppe

>
> Best regards,
> 									Pavel
>

[toc] | [prev] | [next] | [standalone]

#1534883 — Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >Well, if you have a workload that sends and receive packets, it tends
> >to work ok, as you do tx_clean() in stmmac_poll(). My workload is not
> >like that -- it is "sending packets at 3MB/sec, receiving none". So
> >the stmmac_tx_timer() is rescheduled and rescheduled and rescheduled,
> >and then we run out of transmit descriptors, and then 40msec passes,
> >and then we clean them. Bad.
> >
> >And that's why low-res timers do not cut it.
> 
> in that case, I expect that the tuning of the driver could help you.
> I mean, by using ethtool, it could be enough to set the IC bit on all
> the descriptors. You should touch the tx_coal_frames.
> 
> Then you can use ethtool -S to monitor the status.

Yes, I did something similar. Unfortnunately that meant crash within
minutes, at least with 4.4 kernel. (If you know what was fixed between
4.4 and 4.9, that would be helpful).

> We had experimented this tuning on STB IP where just datagrams
> had to send externally. To be honest, although we had seen
> better results w/o any timer, we kept this approach enabled
> because the timer was fast enough to cover our tests on SH4 boxes.

Please reply to David, and explain how it is supposed to
work... because right now it does not. 40 msec delays are not
acceptable in default configuration.

> >>In the ring, some descriptors can raise the irq (according to a
> >>threshold) and set the IC bit. In this path, the NAPI  poll will be
> >>scheduled.
> >
> >Not NAPI poll but stmmac_tx_timer(), right?
> 
> in the xmit according the the threshold the timer is started or the
> interrupt is set inside the descriptor.
> Then stmmac_tx_clean will be always called and, if you see the flow,
> no irqlock protection is needed!

Agreed that no irqlock protection is needed if we rely on napi and timers.

> >>Concerning the lock protection, we had reviewed long time ago and
> >>IIRC, no raise condition should be present. Open to review it,
> >>again!
...
> >There's nothing that protect stmmac_poll() from running concurently
> >with stmmac_dma_interrupt(), right?
> 
> This is not necessary.

dma_interrupt accesses shared priv->xstats; variables are of type
unsigned long (not atomic_t), yet they are accesssed from interrupt
context and from stmmac_ethtool without any locking. That can result
in broken statistics AFAICT.

Please take another look. As far as I can tell, you can have two cpus
at #1 and #2 in the code, at the same time. It looks like napi_... has
some atomic opertions inside so that looks safe at the first look. But
I'm not sure if they also include enough memory barriers to make it
safe...?


static void stmmac_dma_interrupt(struct stmmac_priv *priv)
{
...
        status = priv->hw->dma->dma_interrupt(priv->ioaddr, &priv->xstats);
        if (likely((status & handle_rx)) || (status & handle_tx)) {
                if (likely(napi_schedule_prep(&priv->napi))) {
#1
                        stmmac_disable_dma_irq(priv);
                        __napi_schedule(&priv->napi);
                }
        }


static int stmmac_poll(struct napi_struct *napi, int budget)
{
        struct stmmac_priv *priv = container_of(napi, struct stmmac_priv, napi);
        int work_done = 0;

        priv->xstats.napi_poll++;
        stmmac_tx_clean(priv);

        work_done = stmmac_rx(priv, budget);
	if (work_done < budget) {
                napi_complete(napi);
#2
	        stmmac_enable_dma_irq(priv);
        }
        return work_done;
}


Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1534930 — Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

On 12/2/2016 1:32 PM, Pavel Machek wrote:
> Hi!
>
>>> Well, if you have a workload that sends and receive packets, it tends
>>> to work ok, as you do tx_clean() in stmmac_poll(). My workload is not
>>> like that -- it is "sending packets at 3MB/sec, receiving none". So
>>> the stmmac_tx_timer() is rescheduled and rescheduled and rescheduled,
>>> and then we run out of transmit descriptors, and then 40msec passes,
>>> and then we clean them. Bad.
>>>
>>> And that's why low-res timers do not cut it.
>>
>> in that case, I expect that the tuning of the driver could help you.
>> I mean, by using ethtool, it could be enough to set the IC bit on all
>> the descriptors. You should touch the tx_coal_frames.
>>
>> Then you can use ethtool -S to monitor the status.
>
> Yes, I did something similar. Unfortnunately that meant crash within
> minutes, at least with 4.4 kernel. (If you know what was fixed between
> 4.4 and 4.9, that would be helpful).

4.4 has no GMAC4 support.
Alex, do you remember any patches to fix that?

>> We had experimented this tuning on STB IP where just datagrams
>> had to send externally. To be honest, although we had seen
>> better results w/o any timer, we kept this approach enabled
>> because the timer was fast enough to cover our tests on SH4 boxes.
>
> Please reply to David, and explain how it is supposed to
> work... because right now it does not. 40 msec delays are not
> acceptable in default configuration.

I mean, that on UP and SMP system this schema helped
to improve the performance saving CPU on my side and this has been
tested since a long time (~4 years).
I tested something similar to yours where unidirectional traffic
with limited throughput was needed and I can confirm you that
tuning/removing coalesce parameters this helped. The tuning I decided
to keep in the driver was suitable in several user cases and if now
you have problems or you want to review it I can just confirm that
there are no problems on my side. If you want to simply the logic
around the tx process and remove timer on official driver I can accept
that. I will just ask you uto double check if the throughput and
CPU usage when request max throughput (better if on GiGa setup) has
no regressions.
Otherwise we could start thinking about adaptive schema if feasible.

>>>> In the ring, some descriptors can raise the irq (according to a
>>>> threshold) and set the IC bit. In this path, the NAPI  poll will be
>>>> scheduled.
>>>
>>> Not NAPI poll but stmmac_tx_timer(), right?
>>
>> in the xmit according the the threshold the timer is started or the
>> interrupt is set inside the descriptor.
>> Then stmmac_tx_clean will be always called and, if you see the flow,
>> no irqlock protection is needed!
>
> Agreed that no irqlock protection is needed if we rely on napi and timers.

ok

>
>>>> Concerning the lock protection, we had reviewed long time ago and
>>>> IIRC, no raise condition should be present. Open to review it,
>>>> again!
> ...
>>> There's nothing that protect stmmac_poll() from running concurently
>>> with stmmac_dma_interrupt(), right?
>>
>> This is not necessary.
>
> dma_interrupt accesses shared priv->xstats; variables are of type
> unsigned long (not atomic_t), yet they are accesssed from interrupt
> context and from stmmac_ethtool without any locking. That can result
> in broken statistics AFAICT.

ok we can check this and welcome patches and I'd prefer to
remove xstats from critical part of the code like ISR (that
comes from old story of the driver).

>
> Please take another look. As far as I can tell, you can have two cpus
> at #1 and #2 in the code, at the same time. It looks like napi_... has
> some atomic opertions inside so that looks safe at the first look. But
> I'm not sure if they also include enough memory barriers to make it
> safe...?

Although I have never reproduced related issues on SMP platforms
due to reordering of memory operations but, as said above, welcome
review on this especially if you are seeing problems when manage NAPI.

FYI, the only memory barrier you will see in the driver are about the
OWN_BIT setting till now.

> static void stmmac_dma_interrupt(struct stmmac_priv *priv)
> {
> ...
>         status = priv->hw->dma->dma_interrupt(priv->ioaddr, &priv->xstats);
>         if (likely((status & handle_rx)) || (status & handle_tx)) {
>                 if (likely(napi_schedule_prep(&priv->napi))) {
> #1
>                         stmmac_disable_dma_irq(priv);
>                         __napi_schedule(&priv->napi);
>                 }
>         }
>
>
> static int stmmac_poll(struct napi_struct *napi, int budget)
> {
>         struct stmmac_priv *priv = container_of(napi, struct stmmac_priv, napi);
>         int work_done = 0;
>
>         priv->xstats.napi_poll++;
>         stmmac_tx_clean(priv);
>
>         work_done = stmmac_rx(priv, budget);
> 	if (work_done < budget) {
>                 napi_complete(napi);
> #2
> 	        stmmac_enable_dma_irq(priv);
>         }

hmm, I have to check (and refresh my memory) but the driver
uses the napi_schedule_prep.

Regards

Peppe

>         return work_done;
> }
>
>
> Best regards,
> 									Pavel
>

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web