stmmac ethernet in kernel 4.4: coalescing related pauses?

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?

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)) {

Remove duplicate code from _tx routines.

Quoted 1 line · Pavel Machek

Remove duplicate code from _tx routines.

trivia:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

Quoted 1 line

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -0

[]

Quoted 4 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -0

@@ -1960,6 +1960,38 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
 	}
 }
 

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des, · line 1969

Quoted 7 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +7 -0

+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__);

Quoted 12 lines · Joe Perches

Quoted 1 line · Pavel Machek

Remove duplicate code from _tx routines.

trivia:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

Quoted 1 line

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -0

[]

Quoted 4 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -0

@@ -1960,6 +1960,38 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
 	}
 }
 

Quoted 10 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des, · line 1969

Quoted 7 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +7 -0

+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...)

Quoted 24 lines · Pavel Machek

Quoted 22 lines · Joe Perches

Quoted 1 line · Pavel Machek

Remove duplicate code from _tx routines.

trivia:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

Quoted 1 line

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -0

[]

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des, · line 1969

Quoted 11 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +7 -0

@@ -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__);

Quoted 1 line

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.

Quoted 25 lines · Joe Perches

Quoted 24 lines · Pavel Machek

Quoted 22 lines · Joe Perches

Quoted 1 line · Pavel Machek

Remove duplicate code from _tx routines.

trivia:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

Quoted 1 line

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -0

[]

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des, · line 1969

Quoted 11 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +7 -0

@@ -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__);

Quoted 5 lines

Quoted 1 line

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.

Quoted 26 lines · Pavel Machek

Quoted 25 lines · Joe Perches

Quoted 24 lines · Pavel Machek

Quoted 22 lines · Joe Perches

Quoted 1 line · Pavel Machek

Remove duplicate code from _tx routines.

trivia:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

Quoted 1 line

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -0

[]

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des, · line 1969

Quoted 11 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +7 -0

@@ -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__);

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 40 lines · Joe Perches

Quoted 26 lines · Pavel Machek

Quoted 25 lines · Joe Perches

Quoted 24 lines · Pavel Machek

Quoted 22 lines · Joe Perches

Quoted 1 line · Pavel Machek

Remove duplicate code from _tx routines.

trivia:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

Quoted 1 line

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -0

[]

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des, · line 1969

Quoted 11 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +7 -0

@@ -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__);

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 5 lines

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.

Hi!

Quoted 15 lines

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

Quoted 4 lines

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?

Quoted 3 lines

Quoted 3 lines

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, ...

Quoted 8 lines

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.

Hi!

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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, ...

...

Quoted 15 lines

Quoted 8 lines

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.

Quoted 7 lines

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

Hello

Quoted 37 lines · Pavel Machek

Hi!

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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, ...

...

Quoted 15 lines

Quoted 8 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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

Quoted 4 lines

Thanks and best regards,
Pavel

Quoted 16 lines

Quoted 7 lines

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.

Quoted 4 lines

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!

Hi!

Quoted 22 lines

Quoted 16 lines

Quoted 7 lines

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.

Quoted 4 lines

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...

Quoted 4 lines

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.

Quoted 4 lines

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?

Quoted 6 lines

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

Quoted 7 lines

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.

Quoted 1 line

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.

Quoted 5 lines

Quoted 3 lines

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!

Quoted 3 lines

Quoted 3 lines

Concerning the lock protection, we had reviewed long time ago and IIRC, no raise condition should be present. Open to review it, again!

Quoted 2 lines

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

This is not necessary.

Hi!

Quoted 13 lines

Quoted 8 lines

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.

Quoted 1 line

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).

Quoted 4 lines

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.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 1 line

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.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Concerning the lock protection, we had reviewed long time ago and IIRC, no raise condition should be present. Open to review it, again!

...

Quoted 3 lines

Quoted 2 lines

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

Quoted 1 line

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

Quoted 15 lines · Pavel Machek

Hi!

Quoted 13 lines

Quoted 8 lines

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.

Quoted 5 lines

Quoted 1 line

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?

Quoted 9 lines

Quoted 5 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

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?

Quoted 7 lines

Quoted 4 lines

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

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Concerning the lock protection, we had reviewed long time ago and IIRC, no raise condition should be present. Open to review it, again!

...

Quoted 3 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 1 line

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).

Quoted 6 lines

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.

Quoted 17 lines

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

Quoted 12 lines

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

Quoted 7 lines

        return work_done;
}

Best regards,
Pavel

Quoted 2 lines

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

No sorry Peppe.

Pavel,

Sorry but I'm a little bit confused. I'm dropped in some mails without historic. I see cleanup, coalescence issue and TSO question. What is your main issue? Are you working on gmac4 or 3.x ? Can you refresh a little bit the story please ?

Quoted 4 lines

Sorry but I'm a little bit confused. I'm dropped in some mails without historic. I see cleanup, coalescence issue and TSO question. What is your main issue? Are you working on gmac4 or 3.x ? Can you refresh a little bit the story please ?

let me try to do a sum, please Pavel feel free to correct me.

There are some open points about the tx mitigation schema that we are trying to detail and eventually tune or change (but keeping the same performance on other user-case).

In particular, the test case that is raising problem is an unicast tx bench.
I suggested Pavel to tune coalesce (IC bit settings) via ethtool and monitor stats but he is getting problems (maybe due to lock).

IIUC problems are mainly on new kernel and not on 4.4 where the gmac4 is missing. Please Pavel, could you confirm?

Also there are some other discussion about the lock protection on NAPI still under discussion. I have not clear if in this case Pavel is getting strange behavior.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 1 line

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

ok

Quoted 5 lines

Quoted 3 lines

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!

Actually, I was wrong. irqlock protection is needed, since stmmac_tx_clean() is called from timer, and that's interrupt context, as you can confirm using BUG_ON(in_interrupt());

Quoted 5 lines

Actually, I was wrong. irqlock protection is needed, since stmmac_tx_clean() is called from timer, and that's interrupt context, as you can confirm using BUG_ON(in_interrupt());

in_interrupt() can mean both softirq and hardirq context. In this case it means softirq. So I guess you were right before, and no irq locking is needed.

we need spin_lock_bh at minimum, as we are locking user context against timer.

Best regards,
Pavel

Hi Pavel,

Quoted 4 lines · Pavel Machek

we need spin_lock_bh at minimum, as we are locking user context against timer.

Quoted 3 lines

Best regards,
Pavel

I was referring to stmmac_tx_clean() which AFAICS is only called from softirq context, (one time in the timer handler and one time in napi poll handler) so a spin_lock() should be sufficient. I cant see how this is called from userspace. If it were, a spin_lock_bh() had to be used, of course.

Quoted 7 lines · Lino Sanfilippo

Hi Pavel,

Quoted 4 lines · Pavel Machek

we need spin_lock_bh at minimum, as we are locking user context against timer.

Quoted 8 lines

Quoted 3 lines

Best regards,
Pavel

I was referring to stmmac_tx_clean() which AFAICS is only called from softirq context, (one time in the timer handler and one time in napi poll handler) so a spin_lock() should be sufficient. I cant see how this is called from userspace. If it were, a spin_lock_bh() had to be used, of course.

stmmac_tx_clean() shares lock with stmmac_tx() -- and that's process context as far as I can tell. So... spin_lock_bh() at minimum... right?

Pavel

You mean stmmac_xmit()? Thats also softirq AFAICT, its the TX softirq....

Regards,
Lino

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

Hi,

Quoted 4 lines

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

could it be that there is also another issue concerned locking?:
The tx completion handler takes the xmit_lock in case that the netif_queue is stopped. This is AFAICS unnecessary, since both xmit and completion handler are already synchronized by the private tx lock. But it is IMHO also dangerous:

In the xmit handler we have the locking order
1. xmit_lock
2. private tx lock

while in the completion handler its the reverse:

1. private tx lock
2. xmit lock.

Do I miss something?

Quoted 23 lines · Lino Sanfilippo

Hi,

Quoted 4 lines

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

could it be that there is also another issue concerned locking?:
The tx completion handler takes the xmit_lock in case that the netif_queue is stopped. This is AFAICS unnecessary, since both xmit and completion handler are already synchronized by the private tx lock. But it is IMHO also dangerous:

In the xmit handler we have the locking order
1. xmit_lock
2. private tx lock

while in the completion handler its the reverse:

1. private tx lock
2. xmit lock.

Quoted 1 line

Do I miss something?

No, it seems you are right. Something like this?

Hmm. And can priv->tx_lock be removed, as we already rely on netif_tx_lock?

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.

Fix comments, add some new, and make debugfs output consistent.

Signed-off-by Pavel Machek <[email protected]>

Quoted 10 lines

Quoted 9 lines

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?

Quoted 8 lines

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.

Remove code duplication getting basic descriptors.

Signed-off-by Pavel Machek <[email protected]>

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +16 -26

index f7133d0..ed20668 100644
@@ -929,6 +929,17 @@ static int stmmac_set_bfsize(int mtu, int bufsize)
 	return ret;
 }
 
+static inline struct dma_desc *stmmac_tx_desc(struct stmmac_priv *priv, int i)
+{
+	struct dma_desc *p;
+	if (priv->extend_desc)
+		p = &((priv->dma_etx + i)->basic);
+	else
+		p = priv->dma_tx + i;
+	return p;
+}
+
+
 /**
  * stmmac_clear_descriptors - clear descriptors
  * @priv: driver private structure

@@ -1078,11 +1089,7 @@ static int init_dma_desc_rings(struct net_device *dev, gfp_t flags)
 
 	/* TX INITIALIZATION */
 	for (i = 0; i < DMA_TX_SIZE; i++) {
-		struct dma_desc *p;
-		if (priv->extend_desc)
-			p = &((priv->dma_etx + i)->basic);
-		else
-			p = priv->dma_tx + i;
+		struct dma_desc *p = stmmac_tx_desc(priv, i);
 
 		if (priv->synopsys_id >= DWMAC_CORE_4_00) {
 			p->des0 = 0;

@@ -1129,12 +1136,7 @@ static void dma_free_tx_skbufs(struct stmmac_priv *priv)
 	int i;
 
 	for (i = 0; i < DMA_TX_SIZE; i++) {
-		struct dma_desc *p;
-
-		if (priv->extend_desc)
-			p = &((priv->dma_etx + i)->basic);
-		else
-			p = priv->dma_tx + i;
+		struct dma_desc *p = stmmac_tx_desc(priv, i);
 
 		if (priv->tx_skbuff_dma[i].buf) {
 			if (priv->tx_skbuff_dma[i].map_as_page)

@@ -1314,14 +1316,9 @@ static void __stmmac_tx_clean(struct stmmac_priv *priv)
 
 	while (entry != priv->cur_tx) {
 		struct sk_buff *skb = priv->tx_skbuff[entry];
-		struct dma_desc *p;
+		struct dma_desc *p = stmmac_tx_desc(priv, entry);
 		int status;
 
-		if (priv->extend_desc)
-			p = (struct dma_desc *)(priv->dma_etx + entry);
-		else
-			p = priv->dma_tx + entry;
-
 		status = priv->hw->desc->tx_status(&priv->dev->stats,
 						      &priv->xstats, p,
 						      priv->ioaddr);

@@ -2227,11 +2224,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	csum_insertion = (skb->ip_summed == CHECKSUM_PARTIAL);
 
-	if (likely(priv->extend_desc))
-		desc = (struct dma_desc *)(priv->dma_etx + entry);
-	else
-		desc = priv->dma_tx + entry;
-
+	desc = stmmac_tx_desc(priv, entry);
 	first = desc;
 
 	priv->tx_skbuff[first_entry] = skb;

@@ -2254,10 +2247,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
 		entry = STMMAC_GET_ENTRY(entry, DMA_TX_SIZE);
 
-		if (likely(priv->extend_desc))
-			desc = (struct dma_desc *)(priv->dma_etx + entry);
-		else
-			desc = priv->dma_tx + entry;
+		desc = stmmac_tx_desc(priv, entry);
 
 		des = skb_frag_dma_map(priv->device, frag, 0, len,
 				       DMA_TO_DEVICE);

Simplify flag assignment.

Signed-off-by Pavel Machek <[email protected]>

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -0

index ed20668..0b706a7 100644
@@ -2771,12 +2771,8 @@ static netdev_features_t stmmac_fix_features(struct net_device *dev,
 		features &= ~NETIF_F_CSUM_MASK;
 

Quoted 4 lines

(quoted diff) +0 -0

@@ -2771,12 +2771,8 @@ static netdev_features_t stmmac_fix_features(struct net_device *dev,
 		features &= ~NETIF_F_CSUM_MASK;
 

Review static netdev_features_t stmmac_fix_features(struct net_device *dev, · line 2776

Quoted 10 lines

(quoted diff) +2 -6

 	/* Disable tso if asked by ethtool */
-	if ((priv->plat->tso_en) && (priv->dma_cap.tsoen)) {
-		if (features & NETIF_F_TSO)
-			priv->tso = true;
-		else
-			priv->tso = false;
-	}
+	if ((priv->plat->tso_en) && (priv->dma_cap.tsoen))
+		priv->tso = !!(features & NETIF_F_TSO);
 

Pavel, this really seems arbitrary.

Whilst I really appreciate you're looking into this driver a bit because of some issues you are trying to resolve, I'd like to ask that you not start bombarding me with nit-pick cleanups here and there and instead concentrate on the real bug or issue.

Quoted 17 lines

Quoted 4 lines

(quoted diff) +0 -0

@@ -2771,12 +2771,8 @@ static netdev_features_t stmmac_fix_features(struct net_device *dev,
 		features &= ~NETIF_F_CSUM_MASK;
 

Review static netdev_features_t stmmac_fix_features(struct net_device *dev, · line 2776

Quoted 10 lines

(quoted diff) +2 -6

 	/* Disable tso if asked by ethtool */
-	if ((priv->plat->tso_en) && (priv->dma_cap.tsoen)) {
-		if (features & NETIF_F_TSO)
-			priv->tso = true;
-		else
-			priv->tso = false;
-	}
+	if ((priv->plat->tso_en) && (priv->dma_cap.tsoen))
+		priv->tso = !!(features & NETIF_F_TSO);
 

Pavel, this really seems arbitrary.

Quoted 4 lines

Whilst I really appreciate you're looking into this driver a bit because of some issues you are trying to resolve, I'd like to ask that you not start bombarding me with nit-pick cleanups here and there and instead concentrate on the real bug or issue.

Well, fixing clean code is easier than fixing strange code... Plus I was hoping to make the mainainers to talk. The driver is listed as supported after all.

Anyway... since you asked. I belive I have way to disable NAPI / tx coalescing in the driver. Unfortunately, locking is missing on the rx path, and needs to be extended to _irqsave variant on tx path.

So patch currently looks like this (hand edited, can't be applied, got it working few hours ago). Does it look acceptable?

I'd prefer this to go after the patch that pulls common code to single place, so that single place needs to be patched. Plus I guess I should add ifdefs, so that more advanced NAPI / tx coalescing code can be reactivated when it is fixed. Trivial fixes can go on top. Does that sound like a plan?

Which tree do you want patches against?

https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/ ?

Best regards,
Pavel

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +28 -12

index 0b706a7..c0016c8 100644
@@ -1395,9 +1397,10 @@ static void __stmmac_tx_clean(struct stmmac_priv *priv)
 
 static void stmmac_tx_clean(struct stmmac_priv *priv)
 {
-	spin_lock(&priv->tx_lock);
+	unsigned long flags;
+	spin_lock_irqsave(&priv->tx_lock, flags);
 	__stmmac_tx_clean(priv);
-	spin_unlock(&priv->tx_lock);	
+	spin_unlock_irqrestore(&priv->tx_lock, flags);	
 }
 
 static inline void stmmac_enable_dma_irq(struct stmmac_priv *priv)

@@ -1441,6 +1444,8 @@ static void stmmac_tx_err(struct stmmac_priv *priv)
 	netif_wake_queue(priv->dev);
 }
 
+static int stmmac_rx(struct stmmac_priv *priv, int limit);
+
 /**
  * stmmac_dma_interrupt - DMA ISR
  * @priv: driver private structure

@@ -1452,10 +1457,17 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
 {
 	int status;
 	int rxfifosz = priv->plat->rx_fifo_size;
+	unsigned long flags;
 
 	status = priv->hw->dma->dma_interrupt(priv->ioaddr, &priv->xstats);
 	if (likely((status & handle_rx)) || (status & handle_tx)) {
+		int r;
+		spin_lock_irqsave(&priv->tx_lock, flags);
+		r = stmmac_rx(priv, 999);
+		spin_unlock_irqrestore(&priv->tx_lock, flags);		
+#if 0
 		if (likely(napi_schedule_prep(&priv->napi))) {
 			//pr_err("napi: schedule\n");
 			stmmac_disable_dma_irq(priv);

@@ -1463,7 +1475,8 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
 		} else
 			pr_err("napi: schedule failed\n");
 #endif
+		stmmac_tx_clean(priv);
 	}
 	if (unlikely(status & tx_hard_error_bump_tc)) {
 		/* Try to bump up the dma threshold on this failure */

@@ -1638,7 +1651,7 @@ static void stmmac_tx_timer(unsigned long data)
 {
 	struct stmmac_priv *priv = (struct stmmac_priv *)data;
 
-	stmmac_tx_clean(priv);
+	//stmmac_tx_clean(priv);
 }
 
 /**

@@ -1990,6 +2003,8 @@ static void stmmac_xmit_common(struct sk_buff *skb, struct net_device *dev, int
 	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
 		mod_timer(&priv->txtimer,
 			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
+		priv->hw->desc->set_tx_ic(desc);
+		priv->xstats.tx_set_ic_bit++;
 	} else {
 		priv->tx_count_frames = 0;
 		priv->hw->desc->set_tx_ic(desc);

@@ -2038,8 +2053,9 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct dma_desc *desc, *first, *mss_desc = NULL;
 	u8 proto_hdr_len;
 	int i;
+	unsigned long flags;
 
-	spin_lock(&priv->tx_lock);
+	spin_lock_irqsave(&priv->tx_lock, flags);
 
 	/* Compute header lengths */
 	proto_hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);

@@ -2052,7 +2068,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 			/* This is a hard error, log it. */
 			pr_err("%s: Tx Ring full when queue awake\n", __func__);
 		}
-		spin_unlock(&priv->tx_lock);
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
 		return NETDEV_TX_BUSY;
 	}
 

@@ -2168,11 +2184,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	priv->hw->dma->set_tx_tail_ptr(priv->ioaddr, priv->tx_tail_addr,
 				       STMMAC_CHAN0);
 
-	spin_unlock(&priv->tx_lock);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 	return NETDEV_TX_OK;
 
 dma_map_err:
-	spin_unlock(&priv->tx_lock);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 	dev_err(priv->device, "Tx dma map failed\n");
 	dev_kfree_skb(skb);
 	priv->dev->stats.tx_dropped++;

@@ -2197,6 +2213,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct dma_desc *desc, *first;
 	unsigned int enh_desc;
 	unsigned int des;
+	unsigned int flags;
 
 	/* Manage oversized TCP frames for GMAC4 device */
 	if (skb_is_gso(skb) && priv->tso) {

@@ -2204,7 +2221,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 			return stmmac_tso_xmit(skb, dev);
 	}
 
-	spin_lock(&priv->tx_lock);
+	spin_lock_irqsave(&priv->tx_lock, flags);
 
 	if (unlikely(stmmac_tx_avail(priv) < nfrags + 1)) {
 		if (!netif_queue_stopped(dev)) {

@@ -2212,7 +2229,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 			/* This is a hard error, log it. */
 			pr_err("%s: Tx Ring full when queue awake\n", __func__);
 		}
-		spin_unlock(&priv->tx_lock);
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
 		return NETDEV_TX_BUSY;
 	}
 

@@ -2347,11 +2364,11 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 		priv->hw->dma->set_tx_tail_ptr(priv->ioaddr, priv->tx_tail_addr,
 					       STMMAC_CHAN0);
 
-	spin_unlock(&priv->tx_lock);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 	return NETDEV_TX_OK;
 
 dma_map_err:
-	spin_unlock(&priv->tx_lock);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 	dev_err(priv->device, "Tx dma map failed\n");
 	dev_kfree_skb(skb);
 	priv->dev->stats.tx_dropped++;

@@ -2634,7 +2651,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
 			else
 				skb->ip_summed = CHECKSUM_UNNECESSARY;
 
-			napi_gro_receive(&priv->napi, skb);
+			//napi_gro_receive(&priv->napi, skb);
+			netif_rx(skb);
 
 			priv->dev->stats.rx_packets++;
 			priv->dev->stats.rx_bytes += frame_len;

@@ -2662,6 +2680,7 @@ static int stmmac_poll(struct napi_struct *napi, int budget)
 	struct stmmac_priv *priv = container_of(napi, struct stmmac_priv, napi);
 	int work_done;
 
+	BUG();
 	priv->xstats.napi_poll++;
 	stmmac_tx_clean(priv);
 

Quoted 3 lines

Anyway... since you asked. I belive I have way to disable NAPI / tx coalescing in the driver. Unfortunately, locking is missing on the rx path, and needs to be extended to _irqsave variant on tx path.

I have just replied to a previous thread about that...

Quoted 8 lines

So patch currently looks like this (hand edited, can't be applied, got it working few hours ago). Does it look acceptable?

I'd prefer this to go after the patch that pulls common code to single place, so that single place needs to be patched. Plus I guess I should add ifdefs, so that more advanced NAPI / tx coalescing code can be reactivated when it is fixed. Trivial fixes can go on top. Does that sound like a plan?

Hmm, what I find strange is that, just this code is running since a long time on several platforms and Chip versions. No raise condition have been found or lock protection problems (also proving look mechanisms).

Pavel, I ask you sorry if I missed some problems so, if you can (as D. Miller asked) to send us a cover letter + all patches I will try to reply soon. I can do also some tests if you ask me that! I could run on 3.x and 4.x but I cannot promise you benchmarks.

Quoted 3 lines

Which tree do you want patches against?

https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/ ?

I think that bug fixing should be on top of net.git but I let Miller to decide.

Hi!

Quoted 5 lines

Quoted 3 lines

Anyway... since you asked. I belive I have way to disable NAPI / tx coalescing in the driver. Unfortunately, locking is missing on the rx path, and needs to be extended to _irqsave variant on tx path.

I have just replied to a previous thread about that...

Yeah, please reply to David's mail where he describes why it can't work.

Quoted 10 lines

Quoted 9 lines

So patch currently looks like this (hand edited, can't be applied, got it working few hours ago). Does it look acceptable?

I'd prefer this to go after the patch that pulls common code to single place, so that single place needs to be patched. Plus I guess I should add ifdefs, so that more advanced NAPI / tx coalescing code can be reactivated when it is fixed. Trivial fixes can go on top. Does that sound like a plan?

Quoted 4 lines

Hmm, what I find strange is that, just this code is running since a long time on several platforms and Chip versions. No raise condition have been found or lock protection problems (also proving look mechanisms).

Well, it works better for me when I disable CONFIG_SMP. It is normal that locking problems are hard to reproduce :-(.

Quoted 5 lines

Pavel, I ask you sorry if I missed some problems so, if you can (as D. Miller asked) to send us a cover letter + all patches I will try to reply soon. I can do also some tests if you ask me that! I could run on 3.x and 4.x but I cannot promise you benchmarks.

Actually... I have questions here. David normally pulls from you (can I have a address of your git tree?).

Could you apply these to your git?

[PATCH] stmmac ethernet: unify locking
[PATCH] stmmac: simplify flag assignment
[PATCH] stmmac: cleanup documenation, make it match reality

They are rather trivial and independend, I'm not sure what cover letter would say, besides "simple fixes".

Then I can re-do the reset on top of that...

Quoted 2 lines

Quoted 2 lines

Which tree do you want patches against?

Quoted 4 lines

Quoted 1 line

https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/ ?

I think that bug fixing should be on top of net.git but I let Miller to decide.

Hmm. It is "only" a performance problem (40msec delays).. I guess -next is better target.

Best regards,
Pavel

Quoted 14 lines

Quoted 13 lines

Quoted 8 lines

So patch currently looks like this (hand edited, can't be applied, got it working few hours ago). Does it look acceptable?

I'd prefer this to go after the patch that pulls common code to single place, so that single place needs to be patched. Plus I guess I should add ifdefs, so that more advanced NAPI / tx coalescing code can be reactivated when it is fixed. Trivial fixes can go on top. Does that sound like a plan?

Hmm, what I find strange is that, just this code is running since a long time on several platforms and Chip versions. No raise condition have been found or lock protection problems (also proving look mechanisms).

Quoted 2 lines

Well, it works better for me when I disable CONFIG_SMP. It is normal that locking problems are hard to reproduce :-(.

can you share me the test, maybe I can try to reproduce on ARM box. Are you using 3.x or 4.x GMAC?

Quoted 8 lines

Quoted 5 lines

Pavel, I ask you sorry if I missed some problems so, if you can (as D. Miller asked) to send us a cover letter + all patches I will try to reply soon. I can do also some tests if you ask me that! I could run on 3.x and 4.x but I cannot promise you benchmarks.

Actually... I have questions here. David normally pulls from you (can I have a address of your git tree?).

No I send the patches to the mailing list.

Simplify flag assignment.

Signed-off-by Pavel Machek <[email protected]>

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +2 -6

index ed20668..0b706a7 100644
@@ -2771,12 +2771,8 @@ static netdev_features_t stmmac_fix_features(struct net_device *dev,
 		features &= ~NETIF_F_CSUM_MASK;
 
 	/* Disable tso if asked by ethtool */
-	if ((priv->plat->tso_en) && (priv->dma_cap.tsoen)) {
-		if (features & NETIF_F_TSO)
-			priv->tso = true;
-		else
-			priv->tso = false;
-	}
+	if ((priv->plat->tso_en) && (priv->dma_cap.tsoen))
+		priv->tso = !!(features & NETIF_F_TSO);
 
 	return features;
 }

Fix english in documentation, make documentation match reality, remove options that were removed from code.

Signed-off-by Pavel Machek <[email protected]>

Tx coalescing in stmmac is broken in more than one way, so disable it for now.

First, low-res timers have resolution down to one per second. It is not acceptable to delay transmits for 40msec, and certainly not acceptable to delay them for 1000msec.

Second, the logic is wrong:

    if (likely(priv->tx_coal_frames > priv->tx_count_frames))
     	mod_timer(&priv->txtimer,
     	          STMMAC_COAL_TIMER(priv->tx_coal_timer));
    ...

doing tx_clean() after set number of packets, or set time after the first packet is transmitted would make sense. But that's not what the code does. As long as packets are being transmitted, you move the timer into the future.. so that finally you run out of the place, then wait for timer (!) and only then you do the cleaning.

Third, tx_cleanup is not safe to call from interrupt (tx_lock is not irqsave), but that's exactly what coalescing code does.

Signed-off-by Pavel Machek <[email protected]>

---

This is stable candidate, afaict. It is broken in 4.4, too.

drivers/net/ethernet/stmicro/stmmac/common.h +2 -2

index 3ced2e1..32ce148 100644
@@ -244,11 +244,11 @@ struct stmmac_extra_stats {
 /* Max/Min RI Watchdog Timer count value */
 #define MAX_DMA_RIWT		0xff
 #define MIN_DMA_RIWT		0x20
-/* Tx coalesce parameters */
+/* Tx coalesce parameters. Set STMMAC_TX_FRAMES to 0 to disable coalescing. */
 #define STMMAC_COAL_TX_TIMER	40000
 #define STMMAC_MAX_COAL_TX_TICK	100000
 #define STMMAC_TX_MAX_FRAMES	256
-#define STMMAC_TX_FRAMES	64
+#define STMMAC_TX_FRAMES	0
 
 /* Rx IPC status */
 enum rx_frame_status {

David, ping? Can I get you to apply this one?

As you noticed, tx coalescing is completely broken in that driver, and not easy to repair. This is simplest way to disable it. It can still be re-enabled from userspace, so code can be fixed in future.

From: Pavel Machek <[email protected]>
Date: Sun, 11 Dec 2016 20:07:50 +0100

Quoted 5 lines

David, ping? Can I get you to apply this one?

As you noticed, tx coalescing is completely broken in that driver, and not easy to repair. This is simplest way to disable it. It can still be re-enabled from userspace, so code can be fixed in future.

Sorry I'm not applying this, I want you and the driver maintainer to reach a real solution.

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

Calling skb_orphan() in the xmit handler made this issue disappear.

From: Lino Sanfilippo <[email protected]>
Date: Mon, 28 Nov 2016 14:07:51 +0100

Quoted 1 line

Calling skb_orphan() in the xmit handler made this issue disappear.

This is not the way to handle this problem.

The solution is to free the SKBs in a timely manner after the chip has transmitted the frame.

Quoted 9 lines · David Miller

From: Lino Sanfilippo <[email protected]>
Date: Mon, 28 Nov 2016 14:07:51 +0100

Quoted 1 line

Calling skb_orphan() in the xmit handler made this issue disappear.

This is not the way to handle this problem.

The solution is to free the SKBs in a timely manner after the chip has transmitted the frame.

Note that the 'pauses' described by Pavel are also caused by a too small SO_SNDBUF value on the UDP socket.

An immediate fix, with no kernel change is to increase it.

echo 1000000 >/proc/sys/net/core/wmem_default

or

val = 1000000;
setsockopt(fd, SOL_SOCKET, SO_SNDBUF, &val, sizeof(val));

I wonder if the best fix would be indeed to deactivate irq coalescing completely.
Does it make any sense at all to use it if a driver uses NAPI already?

From: Lino Sanfilippo <[email protected]>
Date: Mon, 28 Nov 2016 16:57:35 +0100

Quoted 3 lines

I wonder if the best fix would be indeed to deactivate irq coalescing completely.
Does it make any sense at all to use it if a driver uses NAPI already?

It absolutely does make sense, when it is implemented and functions properly.

Quoted 9 lines · David Miller

From: Lino Sanfilippo <[email protected]>
Date: Mon, 28 Nov 2016 14:07:51 +0100

Quoted 1 line

Calling skb_orphan() in the xmit handler made this issue disappear.

This is not the way to handle this problem.

The solution is to free the SKBs in a timely manner after the chip has transmitted the frame.

Note that the 'pauses' described by Pavel are also caused by a too small SO_SNDBUF value on the UDP socket.

An immediate fix, with no kernel change is to increase it.

echo 1000000 >/proc/sys/net/core/wmem_default

From: Lino Sanfilippo <[email protected]>
Date: Mon, 28 Nov 2016 14:07:51 +0100

Quoted 1 line

Calling skb_orphan() in the xmit handler made this issue disappear.

This is not the way to handle this problem.