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

Groups > linux.kernel > #1538046 > unrolled thread

Remove private locks to avoid possible deadlock

Back to article view | Back to linux.kernel

Contents

  Remove private locks to avoid possible deadlock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-07 21:10 +0100
    [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-07 21:10 +0100
      Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Francois Romieu <romieu@fr.zoreil.com> - 2016-12-08 00:20 +0100
        Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-08 21:40 +0100
          Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-08 23:00 +0100
            Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-08 23:20 +0100
              Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-08 23:20 +0100
                Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-08 23:50 +0100
          Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Francois Romieu <romieu@fr.zoreil.com> - 2016-12-09 00:30 +0100
            Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-09 12:30 +0100
              Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-10 03:30 +0100
    [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-07 21:10 +0100
      Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-07 22:00 +0100
      Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-07 22:00 +0100
      Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-07 22:40 +0100
        Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-07 22:50 +0100
          Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-07 23:40 +0100
            Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-08 00:30 +0100
        Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock David Miller <davem@davemloft.net> - 2016-12-08 00:50 +0100
          Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-08 15:20 +0100
            Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock David Miller <davem@davemloft.net> - 2016-12-08 16:30 +0100
              Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-08 17:00 +0100

Page 1 of 2  [1] 2  Next page →

#1538046 — Remove private locks to avoid possible deadlock

Hi,

these patches fix possible deadlock situations in the sxgbe and stmmac
driver. Please note that the patches are only compile tested so it would
be great if someone could do tests with the concerning HW.

Regards,
Lino

[toc] | [next] | [standalone]

#1538048 — [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

The driver uses a private lock for synchronization between the xmit
function and the xmit completion handler, but since the NETIF_F_LLTX flag
is not set, the xmit function is also called with the xmit_lock held.

On the other hand the xmit completion handler first takes the private lock
and (in case that the tx queue has been stopped) the xmit_lock, leading
to a reverse locking order and the potential danger of a deadlock.

Fix this by removing the private lock completely and synchronizing the xmit
function and completion handler solely by means of the xmit_lock. By doing
this also remove the now unnecessary double check for a stopped tx queue.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h |  1 -
 drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c   | 27 +++++------------------
 2 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
index 5cb51b6..c61f260 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
@@ -384,7 +384,6 @@ struct sxgbe_tx_queue {
 	dma_addr_t *tx_skbuff_dma;
 	struct sk_buff **tx_skbuff;
 	struct timer_list txtimer;
-	spinlock_t tx_lock;	/* lock for tx queues */
 	unsigned int cur_tx;
 	unsigned int dirty_tx;
 	u32 tx_count_frames;
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
index ea44a24..22d3b0b 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
@@ -426,9 +426,6 @@ static int init_tx_ring(struct device *dev, u8 queue_no,
 	tx_ring->dirty_tx = 0;
 	tx_ring->cur_tx = 0;
 
-	/* initialise TX queue lock */
-	spin_lock_init(&tx_ring->tx_lock);
-
 	return 0;
 
 dmamem_err:
@@ -743,7 +740,7 @@ static void sxgbe_tx_queue_clean(struct sxgbe_tx_queue *tqueue)
 
 	dev_txq = netdev_get_tx_queue(priv->dev, queue_no);
 
-	spin_lock(&tqueue->tx_lock);
+	__netif_tx_lock(dev_txq, smp_processor_id());
 
 	priv->xstats.tx_clean++;
 	while (tqueue->dirty_tx != tqueue->cur_tx) {
@@ -781,18 +778,13 @@ static void sxgbe_tx_queue_clean(struct sxgbe_tx_queue *tqueue)
 
 	/* wake up queue */
 	if (unlikely(netif_tx_queue_stopped(dev_txq) &&
-		     sxgbe_tx_avail(tqueue, tx_rsize) > SXGBE_TX_THRESH(priv))) {
-		netif_tx_lock(priv->dev);
-		if (netif_tx_queue_stopped(dev_txq) &&
-		    sxgbe_tx_avail(tqueue, tx_rsize) > SXGBE_TX_THRESH(priv)) {
-			if (netif_msg_tx_done(priv))
-				pr_debug("%s: restart transmit\n", __func__);
-			netif_tx_wake_queue(dev_txq);
-		}
-		netif_tx_unlock(priv->dev);
+	    sxgbe_tx_avail(tqueue, tx_rsize) > SXGBE_TX_THRESH(priv))) {
+		if (netif_msg_tx_done(priv))
+			pr_debug("%s: restart transmit\n", __func__);
+		netif_tx_wake_queue(dev_txq);
 	}
 
-	spin_unlock(&tqueue->tx_lock);
+	__netif_tx_unlock(dev_txq);
 }
 
 /**
@@ -1304,9 +1296,6 @@ static netdev_tx_t sxgbe_xmit(struct sk_buff *skb, struct net_device *dev)
 		      tqueue->hwts_tx_en)))
 		ctxt_desc_req = 1;
 
-	/* get the spinlock */
-	spin_lock(&tqueue->tx_lock);
-
 	if (priv->tx_path_in_lpi_mode)
 		sxgbe_disable_eee_mode(priv);
 
@@ -1316,8 +1305,6 @@ static netdev_tx_t sxgbe_xmit(struct sk_buff *skb, struct net_device *dev)
 			netdev_err(dev, "%s: Tx Ring is full when %d queue is awake\n",
 				   __func__, txq_index);
 		}
-		/* release the spin lock in case of BUSY */
-		spin_unlock(&tqueue->tx_lock);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -1436,8 +1423,6 @@ static netdev_tx_t sxgbe_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	priv->hw->dma->enable_dma_transmission(priv->ioaddr, txq_index);
 
-	spin_unlock(&tqueue->tx_lock);
-
 	return NETDEV_TX_OK;
 }
 
-- 
1.9.1

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

#1538165 — Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Lino Sanfilippo <LinoSanfilippo@gmx.de> :
> The driver uses a private lock for synchronization between the xmit
> function and the xmit completion handler, but since the NETIF_F_LLTX flag
> is not set, the xmit function is also called with the xmit_lock held.
> 
> On the other hand the xmit completion handler first takes the private lock
> and (in case that the tx queue has been stopped) the xmit_lock, leading
> to a reverse locking order and the potential danger of a deadlock.

netif_tx_stop_queue is used by:
1. xmit function before releasing lock and returning.
2. sxgbe_restart_tx_queue()
   <- sxgbe_tx_interrupt
   <- sxgbe_reset_all_tx_queues()
      <- sxgbe_tx_timeout()

Given xmit won't be called again until tx queue is enabled, it's not clear
how a deadlock could happen due to #1.

Regardless of deadlocks anywhere else, #2 has some serious problem due to
the lack of exclusion between the tx queue restart handler and the xmit
handler.

-- 
Ueimor

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

#1538864 — Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi,

On 08.12.2016 00:15, Francois Romieu wrote:
> Lino Sanfilippo <LinoSanfilippo@gmx.de> :
>> The driver uses a private lock for synchronization between the xmit
>> function and the xmit completion handler, but since the NETIF_F_LLTX flag
>> is not set, the xmit function is also called with the xmit_lock held.
>> 
>> On the other hand the xmit completion handler first takes the private lock
>> and (in case that the tx queue has been stopped) the xmit_lock, leading
>> to a reverse locking order and the potential danger of a deadlock.
> 
> netif_tx_stop_queue is used by:
> 1. xmit function before releasing lock and returning.
> 2. sxgbe_restart_tx_queue()
>    <- sxgbe_tx_interrupt
>    <- sxgbe_reset_all_tx_queues()
>       <- sxgbe_tx_timeout()
> 
> Given xmit won't be called again until tx queue is enabled, it's not clear
> how a deadlock could happen due to #1.
> 


After spending more thoughts on this I tend to agree with you. Yes, we have the
different locking order for the xmit_lock and the private lock in two concurrent
threads. And one of the first things one learns about locking is that this is a
good way to create a deadlock sooner or later. But in our case the deadlock 
can only occur if the xmit function and the tx completion handler perceive different
 states for the tx queue, or to be more specific: 
the completion handler sees the tx queue in state "stopped" while the xmit handler 
sees it in state "running" at the same time. Only then both functions would try to
take both locks, which could lead to a deadlock.

OTOH Pavel said that he actually could produce a deadlock. Now I wonder if this is caused
by that locking scheme (in a way I have not figured out yet) or if it is a different issue.

Regards,
Lino

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

#1538907 — Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

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

On Thu 2016-12-08 21:32:12, Lino Sanfilippo wrote:
> Hi,
> 
> On 08.12.2016 00:15, Francois Romieu wrote:
> > Lino Sanfilippo <LinoSanfilippo@gmx.de> :
> >> The driver uses a private lock for synchronization between the xmit
> >> function and the xmit completion handler, but since the NETIF_F_LLTX flag
> >> is not set, the xmit function is also called with the xmit_lock held.
> >> 
> >> On the other hand the xmit completion handler first takes the private lock
> >> and (in case that the tx queue has been stopped) the xmit_lock, leading
> >> to a reverse locking order and the potential danger of a deadlock.
> > 
> > netif_tx_stop_queue is used by:
> > 1. xmit function before releasing lock and returning.
> > 2. sxgbe_restart_tx_queue()
> >    <- sxgbe_tx_interrupt
> >    <- sxgbe_reset_all_tx_queues()
> >       <- sxgbe_tx_timeout()
> > 
> > Given xmit won't be called again until tx queue is enabled, it's not clear
> > how a deadlock could happen due to #1.
> > 
> 
> 
> After spending more thoughts on this I tend to agree with you. Yes, we have the
> different locking order for the xmit_lock and the private lock in two concurrent
> threads. And one of the first things one learns about locking is that this is a
> good way to create a deadlock sooner or later. But in our case the deadlock 
> can only occur if the xmit function and the tx completion handler perceive different
>  states for the tx queue, or to be more specific: 
> the completion handler sees the tx queue in state "stopped" while the xmit handler 
> sees it in state "running" at the same time. Only then both functions would try to
> take both locks, which could lead to a deadlock.
> 
> OTOH Pavel said that he actually could produce a deadlock. Now I wonder if this is caused
> by that locking scheme (in a way I have not figured out yet) or if it is a different issue.

Pavel has some problems, but that's on different hardware.. and it is
possible that it is deadlock (or something else) somewhere else.

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]

#1538920 — Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi,

On 08.12.2016 22:54, Pavel Machek wrote:
> On Thu 2016-12-08 21:32:12, Lino Sanfilippo wrote:
>> Hi,
>> 
>> On 08.12.2016 00:15, Francois Romieu wrote:
>> > Lino Sanfilippo <LinoSanfilippo@gmx.de> :
>> >> The driver uses a private lock for synchronization between the xmit
>> >> function and the xmit completion handler, but since the NETIF_F_LLTX flag
>> >> is not set, the xmit function is also called with the xmit_lock held.
>> >> 
>> >> On the other hand the xmit completion handler first takes the private lock
>> >> and (in case that the tx queue has been stopped) the xmit_lock, leading
>> >> to a reverse locking order and the potential danger of a deadlock.
>> > 
>> > netif_tx_stop_queue is used by:
>> > 1. xmit function before releasing lock and returning.
>> > 2. sxgbe_restart_tx_queue()
>> >    <- sxgbe_tx_interrupt
>> >    <- sxgbe_reset_all_tx_queues()
>> >       <- sxgbe_tx_timeout()
>> > 
>> > Given xmit won't be called again until tx queue is enabled, it's not clear
>> > how a deadlock could happen due to #1.
>> > 
>> 
>> 
>> After spending more thoughts on this I tend to agree with you. Yes, we have the
>> different locking order for the xmit_lock and the private lock in two concurrent
>> threads. And one of the first things one learns about locking is that this is a
>> good way to create a deadlock sooner or later. But in our case the deadlock 
>> can only occur if the xmit function and the tx completion handler perceive different
>>  states for the tx queue, or to be more specific: 
>> the completion handler sees the tx queue in state "stopped" while the xmit handler 
>> sees it in state "running" at the same time. Only then both functions would try to
>> take both locks, which could lead to a deadlock.
>> 
>> OTOH Pavel said that he actually could produce a deadlock. Now I wonder if this is caused
>> by that locking scheme (in a way I have not figured out yet) or if it is a different issue.
> 
> Pavel has some problems, but that's on different hardware.. and it is
> possible that it is deadlock (or something else) somewhere else.
> 

Right, it is different hardware. But the locking situation in xmit function and tx completion handler
is very similar in both drivers. So if a deadlock is not possible in sxgbe it should 
also not be possible in stmmac (at least not due to the different locking order). 
So maybe there is no real issue that we could fix with removing the private lock and we should
keep it as it is.

Regards,
Lino

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

#1538921 — Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

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

On Thu 2016-12-08 23:12:10, Lino Sanfilippo wrote:
> Hi,
> 
> On 08.12.2016 22:54, Pavel Machek wrote:
> > On Thu 2016-12-08 21:32:12, Lino Sanfilippo wrote:
> >> Hi,
> >> 
> >> On 08.12.2016 00:15, Francois Romieu wrote:
> >> > Lino Sanfilippo <LinoSanfilippo@gmx.de> :
> >> >> The driver uses a private lock for synchronization between the xmit
> >> >> function and the xmit completion handler, but since the NETIF_F_LLTX flag
> >> >> is not set, the xmit function is also called with the xmit_lock held.
> >> >> 
> >> >> On the other hand the xmit completion handler first takes the private lock
> >> >> and (in case that the tx queue has been stopped) the xmit_lock, leading
> >> >> to a reverse locking order and the potential danger of a deadlock.
> >> > 
> >> > netif_tx_stop_queue is used by:
> >> > 1. xmit function before releasing lock and returning.
> >> > 2. sxgbe_restart_tx_queue()
> >> >    <- sxgbe_tx_interrupt
> >> >    <- sxgbe_reset_all_tx_queues()
> >> >       <- sxgbe_tx_timeout()
> >> > 
> >> > Given xmit won't be called again until tx queue is enabled, it's not clear
> >> > how a deadlock could happen due to #1.
> >> > 
> >> 
> >> 
> >> After spending more thoughts on this I tend to agree with you. Yes, we have the
> >> different locking order for the xmit_lock and the private lock in two concurrent
> >> threads. And one of the first things one learns about locking is that this is a
> >> good way to create a deadlock sooner or later. But in our case the deadlock 
> >> can only occur if the xmit function and the tx completion handler perceive different
> >>  states for the tx queue, or to be more specific: 
> >> the completion handler sees the tx queue in state "stopped" while the xmit handler 
> >> sees it in state "running" at the same time. Only then both functions would try to
> >> take both locks, which could lead to a deadlock.
> >> 
> >> OTOH Pavel said that he actually could produce a deadlock. Now I wonder if this is caused
> >> by that locking scheme (in a way I have not figured out yet) or if it is a different issue.
> > 
> > Pavel has some problems, but that's on different hardware.. and it is
> > possible that it is deadlock (or something else) somewhere else.
> > 
> 
> Right, it is different hardware. But the locking situation in xmit function and tx completion handler
> is very similar in both drivers. So if a deadlock is not possible in sxgbe it should 
> also not be possible in stmmac (at least not due to the different locking order). 
> So maybe there is no real issue that we could fix with removing the private lock and we should
> keep it as it is.

Well.. the locking is pretty confused there. Having private lock that
mirrors lock from network layer is confusing and ugly... that should
be reason to fix it.
									Pavel

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

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

#1538936 — Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

On 08.12.2016 23:18, Pavel Machek wrote:
> On Thu 2016-12-08 23:12:10, Lino Sanfilippo wrote:
>> Hi,
>> 
>> On 08.12.2016 22:54, Pavel Machek wrote:
>> > On Thu 2016-12-08 21:32:12, Lino Sanfilippo wrote:
>> >> Hi,
>> >> 
>> >> On 08.12.2016 00:15, Francois Romieu wrote:
>> >> > Lino Sanfilippo <LinoSanfilippo@gmx.de> :
>> >> >> The driver uses a private lock for synchronization between the xmit
>> >> >> function and the xmit completion handler, but since the NETIF_F_LLTX flag
>> >> >> is not set, the xmit function is also called with the xmit_lock held.
>> >> >> 
>> >> >> On the other hand the xmit completion handler first takes the private lock
>> >> >> and (in case that the tx queue has been stopped) the xmit_lock, leading
>> >> >> to a reverse locking order and the potential danger of a deadlock.
>> >> > 
>> >> > netif_tx_stop_queue is used by:
>> >> > 1. xmit function before releasing lock and returning.
>> >> > 2. sxgbe_restart_tx_queue()
>> >> >    <- sxgbe_tx_interrupt
>> >> >    <- sxgbe_reset_all_tx_queues()
>> >> >       <- sxgbe_tx_timeout()
>> >> > 
>> >> > Given xmit won't be called again until tx queue is enabled, it's not clear
>> >> > how a deadlock could happen due to #1.
>> >> > 
>> >> 
>> >> 
>> >> After spending more thoughts on this I tend to agree with you. Yes, we have the
>> >> different locking order for the xmit_lock and the private lock in two concurrent
>> >> threads. And one of the first things one learns about locking is that this is a
>> >> good way to create a deadlock sooner or later. But in our case the deadlock 
>> >> can only occur if the xmit function and the tx completion handler perceive different
>> >>  states for the tx queue, or to be more specific: 
>> >> the completion handler sees the tx queue in state "stopped" while the xmit handler 
>> >> sees it in state "running" at the same time. Only then both functions would try to
>> >> take both locks, which could lead to a deadlock.
>> >> 
>> >> OTOH Pavel said that he actually could produce a deadlock. Now I wonder if this is caused
>> >> by that locking scheme (in a way I have not figured out yet) or if it is a different issue.
>> > 
>> > Pavel has some problems, but that's on different hardware.. and it is
>> > possible that it is deadlock (or something else) somewhere else.
>> > 
>> 
>> Right, it is different hardware. But the locking situation in xmit function and tx completion handler
>> is very similar in both drivers. So if a deadlock is not possible in sxgbe it should 
>> also not be possible in stmmac (at least not due to the different locking order). 
>> So maybe there is no real issue that we could fix with removing the private lock and we should
>> keep it as it is.
> 
> Well.. the locking is pretty confused there. Having private lock that
> mirrors lock from network layer is confusing and ugly... that should
> be reason to fix it.
> 									Pavel
> 

Ok. Then I will resend the patches for both drivers with a different (less dramatic) commit message
in which the change is not longer described as a fix for deadlock but rather as a code 
cleanup/improvement, ok?

Regards,
Lino

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

#1538954 — Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Lino Sanfilippo <LinoSanfilippo@gmx.de> :
[...]
> OTOH Pavel said that he actually could produce a deadlock. Now I wonder if
> this is caused by that locking scheme (in a way I have not figured out yet)
> or if it is a different issue.

stmmac_tx_err races with stmmac_xmit.

-- 
Ueimor

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

#1539291 — Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

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

On Fri 2016-12-09 00:19:43, Francois Romieu wrote:
> Lino Sanfilippo <LinoSanfilippo@gmx.de> :
> [...]
> > OTOH Pavel said that he actually could produce a deadlock. Now I wonder if
> > this is caused by that locking scheme (in a way I have not figured out yet)
> > or if it is a different issue.
> 
> stmmac_tx_err races with stmmac_xmit.

Umm, yes, that looks real.

And that means that removing tx_lock will not be completely trivial
:-(. Lino, any ideas there?

netif_tx_lock_irqsave() would help, but afaict that one does not
exist.

Plus, does someone know how to trigger the status == tx_hard_error? I
tried powering down the switch, but that did not do it.

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]

#1539759 — Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi,

On 09.12.2016 12:21, Pavel Machek wrote:
> On Fri 2016-12-09 00:19:43, Francois Romieu wrote:
>> Lino Sanfilippo <LinoSanfilippo@gmx.de> :
>> [...]
>> > OTOH Pavel said that he actually could produce a deadlock. Now I wonder if
>> > this is caused by that locking scheme (in a way I have not figured out yet)
>> > or if it is a different issue.
>> 
>> stmmac_tx_err races with stmmac_xmit.
> 
> Umm, yes, that looks real.
> 
> And that means that removing tx_lock will not be completely trivial
> :-(. Lino, any ideas there?
> 

Ok, the race is there but it looks like a problem that is not related to 
the use or removal of the private lock.
By a glimpse into other drivers (e.g sky2 or e1000), a possible way to handle a 
tx error is to start a separate task and restart the tx path in that task instead
the irq handler (or timer in case of the watchdog).

In that task we could do:
1. deactivate napi
2. deactivate irqs
3. wait for running napi/irqs do complete (_sync)
4. call stmmac_tx_err()
5. reenable napi
6. reenable irqs

We have to ensure that no xmit() is executing while stmmac_tx_err() does the cleanup,
so stmmac_tx_err() should IMO rather call netif_tx_disable() instead of netif_stop_queue()
(the former grabs the xmit lock before it sets __QUEUE_STATE_DRV_XOFF to disable
the queue).

Regards,
Lino

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

#1538053 — [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock

The driver uses a private lock for synchronization between the xmit
function and the xmit completion handler, but since the NETIF_F_LLTX flag
is not set, the xmit function is also called with the xmit_lock held.

On the other hand the xmit completion handler first takes the private lock
and (in case that the tx queue has been stopped) the xmit_lock, leading to
a reverse locking order and the potential danger of a deadlock.

Fix this by removing the private lock completely and synchronizing the xmit
function and completion handler solely by means of the xmit_lock. By doing
this remove also the now unnecessary double check for a stopped tx queue.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |  1 -
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 28 +++++------------------
 2 files changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 4d2a759..7e69b11 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -64,7 +64,6 @@ struct stmmac_priv {
 	dma_addr_t dma_tx_phy;
 	int tx_coalesce;
 	int hwts_tx_en;
-	spinlock_t tx_lock;
 	bool tx_path_in_lpi_mode;
 	struct timer_list txtimer;
 	bool tso;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index caf069a..db46ec4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1307,7 +1307,7 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
 	unsigned int bytes_compl = 0, pkts_compl = 0;
 	unsigned int entry = priv->dirty_tx;
 
-	spin_lock(&priv->tx_lock);
+	netif_tx_lock(priv->dev);
 
 	priv->xstats.tx_clean++;
 
@@ -1378,22 +1378,17 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
 	netdev_completed_queue(priv->dev, pkts_compl, bytes_compl);
 
 	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) {
-			if (netif_msg_tx_done(priv))
-				pr_debug("%s: restart transmit\n", __func__);
-			netif_wake_queue(priv->dev);
-		}
-		netif_tx_unlock(priv->dev);
+	    stmmac_tx_avail(priv) > STMMAC_TX_THRESH)) {
+		if (netif_msg_tx_done(priv))
+			pr_debug("%s: restart transmit\n", __func__);
+		netif_wake_queue(priv->dev);
 	}
 
 	if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
 		stmmac_enable_eee_mode(priv);
 		mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(eee_timer));
 	}
-	spin_unlock(&priv->tx_lock);
+	netif_tx_unlock(priv->dev);
 }
 
 static inline void stmmac_enable_dma_irq(struct stmmac_priv *priv)
@@ -1998,8 +1993,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	u8 proto_hdr_len;
 	int i;
 
-	spin_lock(&priv->tx_lock);
-
 	/* Compute header lengths */
 	proto_hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
 
@@ -2011,7 +2004,6 @@ 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);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -2146,11 +2138,9 @@ 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);
 	return NETDEV_TX_OK;
 
 dma_map_err:
-	spin_unlock(&priv->tx_lock);
 	dev_err(priv->device, "Tx dma map failed\n");
 	dev_kfree_skb(skb);
 	priv->dev->stats.tx_dropped++;
@@ -2182,10 +2172,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);
-
 	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. */
@@ -2357,11 +2344,9 @@ 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);
 	return NETDEV_TX_OK;
 
 dma_map_err:
-	spin_unlock(&priv->tx_lock);
 	dev_err(priv->device, "Tx dma map failed\n");
 	dev_kfree_skb(skb);
 	priv->dev->stats.tx_dropped++;
@@ -3347,7 +3332,6 @@ int stmmac_dvr_probe(struct device *device,
 	netif_napi_add(ndev, &priv->napi, stmmac_poll, 64);
 
 	spin_lock_init(&priv->lock);
-	spin_lock_init(&priv->tx_lock);
 
 	ret = register_netdev(ndev);
 	if (ret) {
-- 
1.9.1

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

#1538073 — Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock

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

Hi!

> The driver uses a private lock for synchronization between the xmit
> function and the xmit completion handler, but since the NETIF_F_LLTX flag
> is not set, the xmit function is also called with the xmit_lock held.
> 
> On the other hand the xmit completion handler first takes the private lock
> and (in case that the tx queue has been stopped) the xmit_lock, leading to
> a reverse locking order and the potential danger of a deadlock.
> 
> Fix this by removing the private lock completely and synchronizing the xmit
> function and completion handler solely by means of the xmit_lock. By doing
> this remove also the now unnecessary double check for a stopped tx queue.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Does not seem to apply to net-next based on
adc176c5472214971d77c1a61c83db9b01e9cdc7. Aha, that's the printk()
changes, probably would apply to mainline.

> index caf069a..db46ec4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1307,7 +1307,7 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
>  	unsigned int bytes_compl = 0, pkts_compl = 0;
>  	unsigned int entry = priv->dirty_tx;
>  
> -	spin_lock(&priv->tx_lock);
> +	netif_tx_lock(priv->dev);
>  
>  	priv->xstats.tx_clean++;
>

Should it use "netif_tx_lock_bh"?

I could not reproduce the deadlock without this patch, nor can I
detect anything wrong with this patch, so I guess that is:

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

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]

#1538078 — Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock

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

Hi!

> The driver uses a private lock for synchronization between the xmit
> function and the xmit completion handler, but since the NETIF_F_LLTX flag
> is not set, the xmit function is also called with the xmit_lock held.
> 
> On the other hand the xmit completion handler first takes the private lock
> and (in case that the tx queue has been stopped) the xmit_lock, leading to
> a reverse locking order and the potential danger of a deadlock.
> 
> Fix this by removing the private lock completely and synchronizing the xmit
> function and completion handler solely by means of the xmit_lock. By doing
> this remove also the now unnecessary double check for a stopped tx queue.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Oops, sorry no, that broke the driver after a while:

(So please ignore my tested-by:)

root@wagabuibui:/data/tmp/udpt# ./udp-test raw 10.0.0.6 1234 1000 100
30
Sending 100 packets (1000b each) at an interval of 30ms, expected data
rate:3333333b/s (3373333b/s incl udp overhead)
[   30.948626] socfpga-dwmac ff702000.ethernet eth0: Link is Up -
100Mbps/Full - flow control rx/tx
[   31.076064] Link is Up - 100/Full
[   32.979526] random: crng init done
[  262.244030] ------------[ cut here ]------------
[  262.248669] WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:316
dev_watchdog+0x254/0x26c
[  262.256916] NETDEV WATCHDOG: eth0 (socfpga-dwmac): transmit queue 0
timed out
[  262.264028] Modules linked in:
[  262.267102] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
4.9.0-rc7-118095-g2d70d9b-dirty #339
[  262.275328] Hardware name: Altera SOCFPGA
[  262.279352] [<8010f758>] (unwind_backtrace) from [<8010affc>]
(show_stack+0x10/0x14)

									Pavel

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

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

#1538094 — Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock

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

On Wed 2016-12-07 21:05:38, Lino Sanfilippo wrote:
> The driver uses a private lock for synchronization between the xmit
> function and the xmit completion handler, but since the NETIF_F_LLTX flag
> is not set, the xmit function is also called with the xmit_lock held.
> 
> On the other hand the xmit completion handler first takes the private lock
> and (in case that the tx queue has been stopped) the xmit_lock, leading to
> a reverse locking order and the potential danger of a deadlock.
> 
> Fix this by removing the private lock completely and synchronizing the xmit
> function and completion handler solely by means of the xmit_lock. By doing
> this remove also the now unnecessary double check for a stopped tx queue.
> 

FYI, here's modified version. I believe _bh versions are needed, and
I'm testing that version now. (Oh and I also ported it to net-next).

It survived 30 minutes of testing so far...

Best regards,
									Pavel

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Signed-off-by: Pavel Machek <pavel@denx.de>

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index dbacb80..eab04ae 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -64,7 +64,6 @@ struct stmmac_priv {
 	dma_addr_t dma_tx_phy;
 	int tx_coalesce;
 	int hwts_tx_en;
-	spinlock_t tx_lock;
 	bool tx_path_in_lpi_mode;
 	struct timer_list txtimer;
 	bool tso;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 982c952..7415bc2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1308,7 +1308,7 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
 	unsigned int bytes_compl = 0, pkts_compl = 0;
 	unsigned int entry = priv->dirty_tx;
 
-	spin_lock(&priv->tx_lock);
+	netif_tx_lock_bh(priv->dev);
 
 	priv->xstats.tx_clean++;
 
@@ -1378,23 +1378,18 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
 
 	netdev_completed_queue(priv->dev, pkts_compl, bytes_compl);
 
-	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) {
-			netif_dbg(priv, tx_done, priv->dev,
-				  "%s: restart transmit\n", __func__);
-			netif_wake_queue(priv->dev);
-		}
-		netif_tx_unlock(priv->dev);
+	if (netif_queue_stopped(priv->dev) &&
+	    stmmac_tx_avail(priv) > STMMAC_TX_THRESH) {
+		netif_dbg(priv, tx_done, priv->dev,
+			  "%s: restart transmit\n", __func__);
+		netif_wake_queue(priv->dev);
 	}
 
 	if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
 		stmmac_enable_eee_mode(priv);
 		mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(eee_timer));
 	}
-	spin_unlock(&priv->tx_lock);
+	netif_tx_unlock_bh(priv->dev);
 }
 
 static inline void stmmac_enable_dma_irq(struct stmmac_priv *priv)
@@ -2006,8 +2001,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	u8 proto_hdr_len;
 	int i;
 
-	spin_lock(&priv->tx_lock);
-
 	/* Compute header lengths */
 	proto_hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
 
@@ -2021,7 +2014,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 				   "%s: Tx Ring full when queue awake\n",
 				   __func__);
 		}
-		spin_unlock(&priv->tx_lock);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -2156,11 +2148,9 @@ 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);
 	return NETDEV_TX_OK;
 
 dma_map_err:
-	spin_unlock(&priv->tx_lock);
 	dev_err(priv->device, "Tx dma map failed\n");
 	dev_kfree_skb(skb);
 	priv->dev->stats.tx_dropped++;
@@ -2192,10 +2182,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);
-
 	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. */
@@ -2366,11 +2353,9 @@ 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);
 	return NETDEV_TX_OK;
 
 dma_map_err:
-	spin_unlock(&priv->tx_lock);
 	netdev_err(priv->dev, "Tx DMA map failed\n");
 	dev_kfree_skb(skb);
 	priv->dev->stats.tx_dropped++;
@@ -3357,7 +3342,6 @@ int stmmac_dvr_probe(struct device *device,
 	netif_napi_add(ndev, &priv->napi, stmmac_poll, 64);
 
 	spin_lock_init(&priv->lock);
-	spin_lock_init(&priv->tx_lock);
 
 	ret = register_netdev(ndev);
 	if (ret) {




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

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

#1538101 — Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock

Hi Pavel,

On 07.12.2016 22:37, Pavel Machek wrote:
> On Wed 2016-12-07 21:05:38, Lino Sanfilippo wrote:
>> The driver uses a private lock for synchronization between the xmit
>> function and the xmit completion handler, but since the NETIF_F_LLTX flag
>> is not set, the xmit function is also called with the xmit_lock held.
>> 
>> On the other hand the xmit completion handler first takes the private lock
>> and (in case that the tx queue has been stopped) the xmit_lock, leading to
>> a reverse locking order and the potential danger of a deadlock.
>> 
>> Fix this by removing the private lock completely and synchronizing the xmit
>> function and completion handler solely by means of the xmit_lock. By doing
>> this remove also the now unnecessary double check for a stopped tx queue.
>> 
> 
> FYI, here's modified version. I believe _bh versions are needed, and
> I'm testing that version now. (Oh and I also ported it to net-next).
> 
> It survived 30 minutes of testing so far...
> 

First off, thanks for testing.
Hmm. I dont understand why _bh would be needed. We call that function from
BH context only (napi poll and timer).
Any idea?

Lino

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

#1538124 — Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock

On 07.12.2016 22:43, Lino Sanfilippo wrote:
> Hi Pavel,
> 
> On 07.12.2016 22:37, Pavel Machek wrote:
>> On Wed 2016-12-07 21:05:38, Lino Sanfilippo wrote:
>>> The driver uses a private lock for synchronization between the xmit
>>> function and the xmit completion handler, but since the NETIF_F_LLTX flag
>>> is not set, the xmit function is also called with the xmit_lock held.
>>> 
>>> On the other hand the xmit completion handler first takes the private lock
>>> and (in case that the tx queue has been stopped) the xmit_lock, leading to
>>> a reverse locking order and the potential danger of a deadlock.
>>> 
>>> Fix this by removing the private lock completely and synchronizing the xmit
>>> function and completion handler solely by means of the xmit_lock. By doing
>>> this remove also the now unnecessary double check for a stopped tx queue.
>>> 
>> 
>> FYI, here's modified version. I believe _bh versions are needed, and
>> I'm testing that version now. (Oh and I also ported it to net-next).
>> 
>> It survived 30 minutes of testing so far...
>> 
> 
> First off, thanks for testing.
> Hmm. I dont understand why _bh would be needed. We call that function from
> BH context only (napi poll and timer).
> Any idea?
> 

Could this once again be caused by irq coalescing? When the tx queue has been stopped
the cleanup handler has to wakeup the queue within a certain time span, otherwise the
watchdog will complain (as it happened in your test). Could you retest this with
irq coalescing disabled?

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

#1538172 — Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock

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

On Wed 2016-12-07 23:34:19, Lino Sanfilippo wrote:
> On 07.12.2016 22:43, Lino Sanfilippo wrote:
> > Hi Pavel,
> > 
> > On 07.12.2016 22:37, Pavel Machek wrote:
> >> On Wed 2016-12-07 21:05:38, Lino Sanfilippo wrote:
> >>> The driver uses a private lock for synchronization between the xmit
> >>> function and the xmit completion handler, but since the NETIF_F_LLTX flag
> >>> is not set, the xmit function is also called with the xmit_lock held.
> >>> 
> >>> On the other hand the xmit completion handler first takes the private lock
> >>> and (in case that the tx queue has been stopped) the xmit_lock, leading to
> >>> a reverse locking order and the potential danger of a deadlock.
> >>> 
> >>> Fix this by removing the private lock completely and synchronizing the xmit
> >>> function and completion handler solely by means of the xmit_lock. By doing
> >>> this remove also the now unnecessary double check for a stopped tx queue.
> >>> 
> >> 
> >> FYI, here's modified version. I believe _bh versions are needed, and
> >> I'm testing that version now. (Oh and I also ported it to net-next).
> >> 
> >> It survived 30 minutes of testing so far...
> >> 
> > 
> > First off, thanks for testing.
> > Hmm. I dont understand why _bh would be needed. We call that function from
> > BH context only (napi poll and timer).
> > Any idea?
> > 
> 
> Could this once again be caused by irq coalescing? When the tx queue has been stopped
> the cleanup handler has to wakeup the queue within a certain time span, otherwise the
> watchdog will complain (as it happened in your test). Could you retest this with
> irq coalescing disabled?

I actually had TX coalescing disabled, with

-#define STMMAC_TX_FRAMES       64
+#define STMMAC_TX_FRAMES       0


										Pavel

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

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

#1538188 — Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock

From: Pavel Machek <pavel@ucw.cz>
Date: Wed, 7 Dec 2016 22:37:57 +0100

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 982c952..7415bc2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1308,7 +1308,7 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
>  	unsigned int bytes_compl = 0, pkts_compl = 0;
>  	unsigned int entry = priv->dirty_tx;
>  
> -	spin_lock(&priv->tx_lock);
> +	netif_tx_lock_bh(priv->dev);
>  
>  	priv->xstats.tx_clean++;
>  

stmmac_tx_clean() runs from either the timer or the NAPI poll handler,
both execute from software interrupts, therefore _bh() should be
unnecessary.

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

#1538567 — Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock

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

On Wed 2016-12-07 18:41:11, David Miller wrote:
> From: Pavel Machek <pavel@ucw.cz>
> Date: Wed, 7 Dec 2016 22:37:57 +0100
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 982c952..7415bc2 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1308,7 +1308,7 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
> >  	unsigned int bytes_compl = 0, pkts_compl = 0;
> >  	unsigned int entry = priv->dirty_tx;
> >  
> > -	spin_lock(&priv->tx_lock);
> > +	netif_tx_lock_bh(priv->dev);
> >  
> >  	priv->xstats.tx_clean++;
> >  
> 
> stmmac_tx_clean() runs from either the timer or the NAPI poll handler,
> both execute from software interrupts, therefore _bh() should be
> unnecessary.

I've tried the test again with netif_tx_lock() (not _bh()) and it
survived for more then four hours. Strange...

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]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web