[BUG] RTNL and flush_scheduled_work deadlocks

[BUG] RTNL and flush_scheduled_work deadlocks

[Stephen Hemminger]

Ben found this but the problem seems pretty widespread.

The following places are subject to deadlock between flush_scheduled_work
and the RTNL mutex. What can happen is that a work queue routine (like
bridge port_carrier_check) is waiting forever for RTNL, and the driver
routine has called flush_scheduled_work with RTNL held and is waiting
for the work queue to clear.

Several other places have comments like: "can't call flush_scheduled_work
here or it will deadlock". Most of the problem places are in device close
routine. My recommendation would be to add a check for device netif_running in
what ever work routine is used, and move the flush_scheduled_work to the
remove routine.

8139too.c: rtl8139_close --> rtl8139_stop_thread
r8169.c:   rtl8169_down
cassini.c: cas_change_mtu
iseries_veth.c: veth_stop_connection
s2io.c: s2io_close
sis190.c: sis190_down

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Ben Greear]

Stephen Hemminger wrote:
> Ben found this but the problem seems pretty widespread.
> 
> The following places are subject to deadlock between flush_scheduled_work
> and the RTNL mutex. What can happen is that a work queue routine (like
> bridge port_carrier_check) is waiting forever for RTNL, and the driver
> routine has called flush_scheduled_work with RTNL held and is waiting
> for the work queue to clear.
> 
> Several other places have comments like: "can't call flush_scheduled_work
> here or it will deadlock". Most of the problem places are in device close
> routine. My recommendation would be to add a check for device netif_running in
> what ever work routine is used, and move the flush_scheduled_work to the
> remove routine.

I seem to be able to trigger this within about 1 minute on a
particular 2.6.18.2 system with some 8139too devices, so if someone
has a patch that could be tested, I'll gladly test it.  For
whatever reason, I haven't hit this problem on 2.6.20 yet, but
that could easily be dumb luck, and I haven't been running .20
very much.

To add to the list below, tg3 has this problem as well, as far as I
can tell by looking at the code.

Thanks,
Ben

> 
> 8139too.c: rtl8139_close --> rtl8139_stop_thread
> r8169.c:   rtl8169_down
> cassini.c: cas_change_mtu
> iseries_veth.c: veth_stop_connection
> s2io.c: s2io_close
> sis190.c: sis190_down
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Francois Romieu]

Ben Greear <greearb@candelatech.com> :
[...]
> I seem to be able to trigger this within about 1 minute on a
> particular 2.6.18.2 system with some 8139too devices, so if someone
> has a patch that could be tested, I'll gladly test it.  For
> whatever reason, I haven't hit this problem on 2.6.20 yet, but
> that could easily be dumb luck, and I haven't been running .20
> very much.

Bandaid below. It is not complete if your device hits the tx_watchdog
hard but it should help.

I'll return in 24h.

diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
index 35ad5cf..cbee350 100644
--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -1603,18 +1603,20 @@ static void rtl8139_thread (struct work_struct *work)
 	struct net_device *dev = tp->mii.dev;
 	unsigned long thr_delay = next_tick;
 
+	rtnl_lock();
+
+	if (!netif_running(dev))
+		goto unlock;
+
 	if (tp->watchdog_fired) {
 		tp->watchdog_fired = 0;
 		rtl8139_tx_timeout_task(work);
-	} else if (rtnl_trylock()) {
-		rtl8139_thread_iter (dev, tp, tp->mmio_addr);
-		rtnl_unlock ();
-	} else {
-		/* unlikely race.  mitigate with fast poll. */
-		thr_delay = HZ / 2;
-	}
+	} else
+		rtl8139_thread_iter(dev, tp, tp->mmio_addr);
 
 	schedule_delayed_work(&tp->thread, thr_delay);
+unlock:
+	rtnl_unlock ();
 }
 
 static void rtl8139_start_thread(struct rtl8139_private *tp)
@@ -1626,19 +1628,11 @@ static void rtl8139_start_thread(struct rtl8139_private *tp)
 		return;
 
 	tp->have_thread = 1;
+	tp->watchdog_fired = 0;
 
 	schedule_delayed_work(&tp->thread, next_tick);
 }
 
-static void rtl8139_stop_thread(struct rtl8139_private *tp)
-{
-	if (tp->have_thread) {
-		cancel_rearming_delayed_work(&tp->thread);
-		tp->have_thread = 0;
-	} else
-		flush_scheduled_work();
-}
-
 static inline void rtl8139_tx_clear (struct rtl8139_private *tp)
 {
 	tp->cur_tx = 0;
@@ -2233,8 +2227,6 @@ static int rtl8139_close (struct net_device *dev)
 
 	netif_stop_queue (dev);
 
-	rtl8139_stop_thread(tp);
-
 	if (netif_msg_ifdown(tp))
 		printk(KERN_DEBUG "%s: Shutting down ethercard, status was 0x%4.4x.\n",
 			dev->name, RTL_R16 (IntrStatus));

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Ben Greear]

Francois Romieu wrote:
> Ben Greear <greearb@candelatech.com> :
> [...]
>> I seem to be able to trigger this within about 1 minute on a
>> particular 2.6.18.2 system with some 8139too devices, so if someone
>> has a patch that could be tested, I'll gladly test it.  For
>> whatever reason, I haven't hit this problem on 2.6.20 yet, but
>> that could easily be dumb luck, and I haven't been running .20
>> very much.
> 
> Bandaid below. It is not complete if your device hits the tx_watchdog
> hard but it should help.

So far, I've been running several hours without problems, so
this does appear to be helping.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

[PATCH 1/4] r8169: RTNL and flush_scheduled_work deadlock

[Francois Romieu]

flush_scheduled_work() in net_device->close has a slight tendency
to deadlock with tasks on the workqueue that hold RTNL.

rtl8169_close/down simply need the recovery tasks to not meddle
with the hardware while the device is going down.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/r8169.c |   25 ++++++++++++++++++-------
 1 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 5598d86..13cf06e 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -1733,6 +1733,8 @@ rtl8169_remove_one(struct pci_dev *pdev)
 	assert(dev != NULL);
 	assert(tp != NULL);
 
+	flush_scheduled_work();
+
 	unregister_netdev(dev);
 	rtl8169_release_board(pdev, dev, tp->mmio_addr);
 	pci_set_drvdata(pdev, NULL);
@@ -2161,10 +2163,13 @@ static void rtl8169_reinit_task(struct work_struct *work)
 	struct net_device *dev = tp->dev;
 	int ret;
 
-	if (netif_running(dev)) {
-		rtl8169_wait_for_quiescence(dev);
-		rtl8169_close(dev);
-	}
+	rtnl_lock();
+
+	if (!netif_running(dev))
+		goto out_unlock;
+
+	rtl8169_wait_for_quiescence(dev);
+	rtl8169_close(dev);
 
 	ret = rtl8169_open(dev);
 	if (unlikely(ret < 0)) {
@@ -2179,6 +2184,9 @@ static void rtl8169_reinit_task(struct work_struct *work)
 		}
 		rtl8169_schedule_work(dev, rtl8169_reinit_task);
 	}
+
+out_unlock:
+	rtnl_unlock();
 }
 
 static void rtl8169_reset_task(struct work_struct *work)
@@ -2187,8 +2195,10 @@ static void rtl8169_reset_task(struct work_struct *work)
 		container_of(work, struct rtl8169_private, task.work);
 	struct net_device *dev = tp->dev;
 
+	rtnl_lock();
+
 	if (!netif_running(dev))
-		return;
+		goto out_unlock;
 
 	rtl8169_wait_for_quiescence(dev);
 
@@ -2210,6 +2220,9 @@ static void rtl8169_reset_task(struct work_struct *work)
 		}
 		rtl8169_schedule_work(dev, rtl8169_reset_task);
 	}
+
+out_unlock:
+	rtnl_unlock();
 }
 
 static void rtl8169_tx_timeout(struct net_device *dev)
@@ -2722,8 +2735,6 @@ static void rtl8169_down(struct net_device *dev)
 
 	netif_stop_queue(dev);
 
-	flush_scheduled_work();
-
 core_down:
 	spin_lock_irq(&tp->lock);
 
-- 
1.4.4.4

[PATCH 2/4] sis190: RTNL and flush_scheduled_work deadlock

[Francois Romieu]

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/sis190.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/sis190.c b/drivers/net/sis190.c
index 45d91b1..b08508b 100644
--- a/drivers/net/sis190.c
+++ b/drivers/net/sis190.c
@@ -909,6 +909,9 @@ static void sis190_phy_task(struct work_struct *work)
 
 	rtnl_lock();
 
+	if (!netif_running(dev))
+		goto out_unlock;
+
 	val = mdio_read(ioaddr, phy_id, MII_BMCR);
 	if (val & BMCR_RESET) {
 		// FIXME: needlessly high ?  -- FR 02/07/2005
@@ -981,6 +984,7 @@ static void sis190_phy_task(struct work_struct *work)
 		netif_carrier_on(dev);
 	}
 
+out_unlock:
 	rtnl_unlock();
 }
 
@@ -1102,8 +1106,6 @@ static void sis190_down(struct net_device *dev)
 
 	netif_stop_queue(dev);
 
-	flush_scheduled_work();
-
 	do {
 		spin_lock_irq(&tp->lock);
 
@@ -1857,6 +1859,7 @@ static void __devexit sis190_remove_one(struct pci_dev *pdev)
 	struct net_device *dev = pci_get_drvdata(pdev);
 
 	sis190_mii_remove(dev);
+	flush_scheduled_work();
 	unregister_netdev(dev);
 	sis190_release_board(pdev);
 	pci_set_drvdata(pdev, NULL);
-- 
1.4.4.4

[PATCH 3/4] 8139too: RTNL and flush_scheduled_work deadlock

[Francois Romieu]

Your usual dont-flush_scheduled_work-with-RTNL-held stuff.

It is a bit different here since the thread runs permanently
or is only occasionally kicked for recovery depending on the
hardware revision.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/8139too.c |   40 +++++++++++++++++-----------------------
 1 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
index 35ad5cf..99304b2 100644
--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -1109,6 +1109,8 @@ static void __devexit rtl8139_remove_one (struct pci_dev *pdev)
 
 	assert (dev != NULL);
 
+	flush_scheduled_work();
+
 	unregister_netdev (dev);
 
 	__rtl8139_cleanup_dev (dev);
@@ -1603,18 +1605,21 @@ static void rtl8139_thread (struct work_struct *work)
 	struct net_device *dev = tp->mii.dev;
 	unsigned long thr_delay = next_tick;
 
+	rtnl_lock();
+
+	if (!netif_running(dev))
+		goto out_unlock;
+
 	if (tp->watchdog_fired) {
 		tp->watchdog_fired = 0;
 		rtl8139_tx_timeout_task(work);
-	} else if (rtnl_trylock()) {
-		rtl8139_thread_iter (dev, tp, tp->mmio_addr);
-		rtnl_unlock ();
-	} else {
-		/* unlikely race.  mitigate with fast poll. */
-		thr_delay = HZ / 2;
-	}
+	} else
+		rtl8139_thread_iter(dev, tp, tp->mmio_addr);
 
-	schedule_delayed_work(&tp->thread, thr_delay);
+	if (tp->have_thread)
+		schedule_delayed_work(&tp->thread, thr_delay);
+out_unlock:
+	rtnl_unlock ();
 }
 
 static void rtl8139_start_thread(struct rtl8139_private *tp)
@@ -1626,19 +1631,11 @@ static void rtl8139_start_thread(struct rtl8139_private *tp)
 		return;
 
 	tp->have_thread = 1;
+	tp->watchdog_fired = 0;
 
 	schedule_delayed_work(&tp->thread, next_tick);
 }
 
-static void rtl8139_stop_thread(struct rtl8139_private *tp)
-{
-	if (tp->have_thread) {
-		cancel_rearming_delayed_work(&tp->thread);
-		tp->have_thread = 0;
-	} else
-		flush_scheduled_work();
-}
-
 static inline void rtl8139_tx_clear (struct rtl8139_private *tp)
 {
 	tp->cur_tx = 0;
@@ -1696,12 +1693,11 @@ static void rtl8139_tx_timeout (struct net_device *dev)
 {
 	struct rtl8139_private *tp = netdev_priv(dev);
 
+	tp->watchdog_fired = 1;
 	if (!tp->have_thread) {
-		INIT_DELAYED_WORK(&tp->thread, rtl8139_tx_timeout_task);
+		INIT_DELAYED_WORK(&tp->thread, rtl8139_thread);
 		schedule_delayed_work(&tp->thread, next_tick);
-	} else
-		tp->watchdog_fired = 1;
-
+	}
 }
 
 static int rtl8139_start_xmit (struct sk_buff *skb, struct net_device *dev)
@@ -2233,8 +2229,6 @@ static int rtl8139_close (struct net_device *dev)
 
 	netif_stop_queue (dev);
 
-	rtl8139_stop_thread(tp);
-
 	if (netif_msg_ifdown(tp))
 		printk(KERN_DEBUG "%s: Shutting down ethercard, status was 0x%4.4x.\n",
 			dev->name, RTL_R16 (IntrStatus));
-- 
1.4.4.4

[PATCH 4/4] s2io: RTNL and flush_scheduled_work deadlock

[Francois Romieu]

Mantra: don't use flush_scheduled_work with RTNL held.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/s2io.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index e8e0d94..fd85648 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -3758,7 +3758,6 @@ static int s2io_close(struct net_device *dev)
 {
 	struct s2io_nic *sp = dev->priv;
 
-	flush_scheduled_work();
 	netif_stop_queue(dev);
 	/* Reset card, kill tasklet and free Tx and Rx buffers. */
 	s2io_card_down(sp);
@@ -5847,9 +5846,14 @@ static void s2io_set_link(struct work_struct *work)
 	register u64 val64;
 	u16 subid;
 
+	rtnl_lock();
+
+	if (!netif_running(dev))
+		goto out_unlock;
+
 	if (test_and_set_bit(0, &(nic->link_state))) {
 		/* The card is being reset, no point doing anything */
-		return;
+		goto out_unlock;
 	}
 
 	subid = nic->pdev->subsystem_device;
@@ -5903,6 +5907,9 @@ static void s2io_set_link(struct work_struct *work)
 		s2io_link(nic, LINK_DOWN);
 	}
 	clear_bit(0, &(nic->link_state));
+
+out_unlock:
+	rtnl_lock();
 }
 
 static int set_rxd_buffer_pointer(struct s2io_nic *sp, struct RxD_t *rxdp,
@@ -6356,6 +6363,11 @@ static void s2io_restart_nic(struct work_struct *work)
 	struct s2io_nic *sp = container_of(work, struct s2io_nic, rst_timer_task);
 	struct net_device *dev = sp->dev;
 
+	rtnl_lock();
+
+	if (!netif_running(dev))
+		goto out_unlock;
+
 	s2io_card_down(sp);
 	if (s2io_card_up(sp)) {
 		DBG_PRINT(ERR_DBG, "%s: Device bring up failed\n",
@@ -6364,7 +6376,8 @@ static void s2io_restart_nic(struct work_struct *work)
 	netif_wake_queue(dev);
 	DBG_PRINT(ERR_DBG, "%s: was reset by Tx watchdog timer\n",
 		  dev->name);
-
+out_unlock:
+	rtnl_unlock();
 }
 
 /**
@@ -7173,6 +7186,8 @@ static void __devexit s2io_rem_nic(struct pci_dev *pdev)
 		return;
 	}
 
+	flush_scheduled_work();
+
 	sp = dev->priv;
 	unregister_netdev(dev);
 
-- 
1.4.4.4

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Jarek Poplawski]

On 14-02-2007 22:27, Stephen Hemminger wrote:
> Ben found this but the problem seems pretty widespread.
> 
> The following places are subject to deadlock between flush_scheduled_work
> and the RTNL mutex. What can happen is that a work queue routine (like
> bridge port_carrier_check) is waiting forever for RTNL, and the driver
> routine has called flush_scheduled_work with RTNL held and is waiting
> for the work queue to clear.
> 
> Several other places have comments like: "can't call flush_scheduled_work
> here or it will deadlock". Most of the problem places are in device close
> routine. My recommendation would be to add a check for device netif_running in
> what ever work routine is used, and move the flush_scheduled_work to the
> remove routine.
> 
> 8139too.c: rtl8139_close --> rtl8139_stop_thread
> r8169.c:   rtl8169_down
> cassini.c: cas_change_mtu
> iseries_veth.c: veth_stop_connection
> s2io.c: s2io_close
> sis190.c: sis190_down
> 

There is probably more than this...

I think the same problem is with
cancel_rearming_delayed_work. Plus indirect calling
these functions: eg. by ieee8021softmac_stop.

I found these dangerous places (probably not all):

cxgb3/cxgb3_main.c (cxgb_close -> cxgb_down),

macb.c (macb_close),

skge.c (skge_down),

wireless/bcm43xx/bcm43xx_main.c (bcm_net_stop both
	ieee80211...  and flush_...),
wireless/zd1211rw/zd_mac.c (zd_mac_stop ->
	housekeeping_disable),

chelsio/my3126.c (t1_interrupts_disable ->
	my3126_interrupt_disable), /* not sure */

drivers/usb/net/kaweth.c (kaweth_close ->
	kaweth_kill_urbs)

Regards,
Jarek P.

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Ben Greear]

Jarek Poplawski wrote:
> On 14-02-2007 22:27, Stephen Hemminger wrote:
>   
>> Ben found this but the problem seems pretty widespread.
>>
>> The following places are subject to deadlock between flush_scheduled_work
>> and the RTNL mutex. What can happen is that a work queue routine (like
>> bridge port_carrier_check) is waiting forever for RTNL, and the driver
>> routine has called flush_scheduled_work with RTNL held and is waiting
>> for the work queue to clear.
>>
>> Several other places have comments like: "can't call flush_scheduled_work
>> here or it will deadlock". Most of the problem places are in device close
>> routine. My recommendation would be to add a check for device netif_running in
>> what ever work routine is used, and move the flush_scheduled_work to the
>> remove routine.
>>
>> 8139too.c: rtl8139_close --> rtl8139_stop_thread
>> r8169.c:   rtl8169_down
>> cassini.c: cas_change_mtu
>> iseries_veth.c: veth_stop_connection
>> s2io.c: s2io_close
>> sis190.c: sis190_down
>>
>>     
>
> There is probably more than this...
>   

Maybe there should be something like an ASSERT_NOT_RTNL() in the 
flush_scheduled_work()
method?  If it's performance criticial, #ifdef it out if we're not 
debugging locks?

Ben

-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 3/4] 8139too: RTNL and flush_scheduled_work deadlock

[Jarek Poplawski]

On 15-02-2007 23:37, Francois Romieu wrote:
> Your usual dont-flush_scheduled_work-with-RTNL-held stuff.
> 
> It is a bit different here since the thread runs permanently
> or is only occasionally kicked for recovery depending on the
> hardware revision.
> 
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> ---
>  drivers/net/8139too.c |   40 +++++++++++++++++-----------------------
>  1 files changed, 17 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
> index 35ad5cf..99304b2 100644
> --- a/drivers/net/8139too.c
> +++ b/drivers/net/8139too.c
> @@ -1109,6 +1109,8 @@ static void __devexit rtl8139_remove_one (struct pci_dev *pdev)
>  
>  	assert (dev != NULL);
>  
> +	flush_scheduled_work();
> +

IMHO there should be rather cancel_rearming_delayed_work
instead of this.

>  	unregister_netdev (dev);
>  
>  	__rtl8139_cleanup_dev (dev);
> @@ -1603,18 +1605,21 @@ static void rtl8139_thread (struct work_struct *work)
>  	struct net_device *dev = tp->mii.dev;
>  	unsigned long thr_delay = next_tick;
>  
> +	rtnl_lock();
> +
> +	if (!netif_running(dev))
> +		goto out_unlock;

I wonder, why you don't do netif_running before
rtnl_lock? It's an atomic operation.

And I'm not sure if increasing rtnl_lock range
is really needed here.

Regards,
Jarek P.

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Jarek Poplawski]

On Thu, Feb 15, 2007 at 11:40:32PM -0800, Ben Greear wrote:
...
> Maybe there should be something like an ASSERT_NOT_RTNL() in the 
> flush_scheduled_work()
> method?  If it's performance criticial, #ifdef it out if we're not 
> debugging locks?

Yes! I thought about the same (at first). But in my
opinion it was not enough, so I thought about doing
this in flush_workqueue. But in my next opinion it
was not enough too. Now I think something like this
should be done in rtnl_lock (under some debugging #if
of course). 

Cheers,
Jarek P.

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Ben Greear]

Jarek Poplawski wrote:
> On Thu, Feb 15, 2007 at 11:40:32PM -0800, Ben Greear wrote:
> ...
>   
>> Maybe there should be something like an ASSERT_NOT_RTNL() in the 
>> flush_scheduled_work()
>> method?  If it's performance criticial, #ifdef it out if we're not 
>> debugging locks?
>>     
>
> Yes! I thought about the same (at first). But in my
> opinion it was not enough, so I thought about doing
> this in flush_workqueue. But in my next opinion it
> was not enough too. Now I think something like this
> should be done in rtnl_lock (under some debugging #if
> of course). 
>   
The reason these bugs have been hidden is that most of the time, there 
is nothing
on the pending work queue that will try to grab RTNL.  But, the 
flush_work_queue
is still called with RTNL held, so an assert would find this much 
earlier than
waiting for someone to get lucky and actually catch (and debug and report)
a deadlock...

I don't see how asserting it in the rtnl_lock would help anything, 
because at that
point we are about to deadlock anyway...  (and this is probably very 
rare, as mentioned above.)

Thanks,
Ben

> Cheers,
> Jarek P.
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Jarek Poplawski]

On Fri, Feb 16, 2007 at 12:23:05AM -0800, Ben Greear wrote:
> Jarek Poplawski wrote:
> >On Thu, Feb 15, 2007 at 11:40:32PM -0800, Ben Greear wrote:
> >...
> >  
> >>Maybe there should be something like an ASSERT_NOT_RTNL() in the 
> >>flush_scheduled_work()
> >>method?  If it's performance criticial, #ifdef it out if we're not 
> >>debugging locks?
> >>    
> >
> >Yes! I thought about the same (at first). But in my
> >opinion it was not enough, so I thought about doing
> >this in flush_workqueue. But in my next opinion it
> >was not enough too. Now I think something like this
> >should be done in rtnl_lock (under some debugging #if
> >of course). 
> >  
> The reason these bugs have been hidden is that most of the time, there 
> is nothing
> on the pending work queue that will try to grab RTNL.  But, the 
> flush_work_queue
> is still called with RTNL held, so an assert would find this much 
> earlier than
> waiting for someone to get lucky and actually catch (and debug and report)
> a deadlock...

Yes. Regarding this, you are right - it should be better.
But, anyway cancel_rearming... is equally dangerous,
so there is a question where: in all places where used,
in workqueue.c or maybe make it semi obligatory in
networking and add some net wrappers e.g.:
net_flush_work_queue and net_cancel_rearmnig with this
ASSERT_NO_RTNL?

> 
> I don't see how asserting it in the rtnl_lock would help anything, 
> because at that
> point we are about to deadlock anyway...  (and this is probably very 
> rare, as mentioned above.)

But it's happening now. Probably lockdep is not enough
and rtnl_lock is probably used in too many places, so I
meant some additional checks would be possible.
I wrote "something like this" and mean some check if we
have this lock already before trying to get it again.

But maybe it's really too much and your proposal is
sufficient for this.

Jarek P.

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Jarek Poplawski]

On Fri, Feb 16, 2007 at 10:04:25AM +0100, Jarek Poplawski wrote:
> On Fri, Feb 16, 2007 at 12:23:05AM -0800, Ben Greear wrote:
...
> > I don't see how asserting it in the rtnl_lock would help anything, 
> > because at that
> > point we are about to deadlock anyway...  (and this is probably very 
> > rare, as mentioned above.)
> 
> But it's happening now. Probably lockdep is not enough
> and rtnl_lock is probably used in too many places, so I
> meant some additional checks would be possible.

And of course it already could be done by DEBUG_MUTEXES or
DEBUG_SPINLOCK, so I gone too far and it's really bad idea.

Jarek P.

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Ben Greear]

Jarek Poplawski wrote:
> On Fri, Feb 16, 2007 at 10:04:25AM +0100, Jarek Poplawski wrote:
>   
>> On Fri, Feb 16, 2007 at 12:23:05AM -0800, Ben Greear wrote:
>>     
> ...
>   
>>> I don't see how asserting it in the rtnl_lock would help anything, 
>>> because at that
>>> point we are about to deadlock anyway...  (and this is probably very 
>>> rare, as mentioned above.)
>>>       
>> But it's happening now. Probably lockdep is not enough
>> and rtnl_lock is probably used in too many places, so I
>> meant some additional checks would be possible.
>>     
>
> And of course it already could be done by DEBUG_MUTEXES or
> DEBUG_SPINLOCK, so I gone too far and it's really bad idea.
>   

Well, I had lockdep and all of the locking debugging I could find 
enabled, but
it did not catch this problem..I had to use sysctl -t and manually dig 
through the backtraces
to find the deadlock....

It may be that lockdep could be enhanced to catch this sort of thing....

Ben

-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Stephen Hemminger]

On Thu, 15 Feb 2007 23:40:32 -0800
Ben Greear <greearb@candelatech.com> wrote:

> Jarek Poplawski wrote:
> > On 14-02-2007 22:27, Stephen Hemminger wrote:
> >   
> >> Ben found this but the problem seems pretty widespread.
> >>
> >> The following places are subject to deadlock between flush_scheduled_work
> >> and the RTNL mutex. What can happen is that a work queue routine (like
> >> bridge port_carrier_check) is waiting forever for RTNL, and the driver
> >> routine has called flush_scheduled_work with RTNL held and is waiting
> >> for the work queue to clear.
> >>
> >> Several other places have comments like: "can't call flush_scheduled_work
> >> here or it will deadlock". Most of the problem places are in device close
> >> routine. My recommendation would be to add a check for device netif_running in
> >> what ever work routine is used, and move the flush_scheduled_work to the
> >> remove routine.
> >>
> >> 8139too.c: rtl8139_close --> rtl8139_stop_thread
> >> r8169.c:   rtl8169_down
> >> cassini.c: cas_change_mtu
> >> iseries_veth.c: veth_stop_connection
> >> s2io.c: s2io_close
> >> sis190.c: sis190_down
> >>
> >>     
> >
> > There is probably more than this...
> >   
> 
> Maybe there should be something like an ASSERT_NOT_RTNL() in the 
> flush_scheduled_work()
> method?  If it's performance criticial, #ifdef it out if we're not 
> debugging locks?
> 

You can't safely add a check like that. What if another cpu had acquired
RTNL and was unrelated.


-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Ben Greear]

Stephen Hemminger wrote:
> On Thu, 15 Feb 2007 23:40:32 -0800
> Ben Greear <greearb@candelatech.com> wrote:

>> Maybe there should be something like an ASSERT_NOT_RTNL() in the 
>> flush_scheduled_work()
>> method?  If it's performance criticial, #ifdef it out if we're not 
>> debugging locks?
>>
> 
> You can't safely add a check like that. What if another cpu had acquired
> RTNL and was unrelated.

I guess there isn't a way to see if *this* thread is the owner of the RTNL
currently?  I think lockdep knows the owner...maybe could query it somehow,
or just save the owner in the mutex object when debugging is enabled...

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 3/4] 8139too: RTNL and flush_scheduled_work deadlock

[Francois Romieu]

Jarek Poplawski <jarkao2@o2.pl> :
[...]
> > diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
> > index 35ad5cf..99304b2 100644
> > --- a/drivers/net/8139too.c
> > +++ b/drivers/net/8139too.c
> > @@ -1109,6 +1109,8 @@ static void __devexit rtl8139_remove_one (struct pci_dev *pdev)
> >  
> >  	assert (dev != NULL);
> >  
> > +	flush_scheduled_work();
> > +
> 
> IMHO there should be rather cancel_rearming_delayed_work
> instead of this.

The delayed_work is initialized even if tp->have_thread is false,
so cancel_rearming_delayed_work() will work, yes. Feel free to
send a patch.

[...]
> > @@ -1603,18 +1605,21 @@ static void rtl8139_thread (struct work_struct *work)
> >  	struct net_device *dev = tp->mii.dev;
> >  	unsigned long thr_delay = next_tick;
> >  
> > +	rtnl_lock();
> > +
> > +	if (!netif_running(dev))
> > +		goto out_unlock;
> 
> I wonder, why you don't do netif_running before
> rtnl_lock ? It's an atomic operation. And I'm not sure if increasing
> rtnl_lock range is really needed here.

thread    A: netif_running()
user task B: rtnl_lock()
user task B: dev->close()
user task B: rtnl_unlock()
thread    A: rtnl_lock()
thread    A: mess with closed device

Btw, the thread runs every 3*HZ at most.

-- 
Ueimor

Re: [PATCH 3/4] 8139too: RTNL and flush_scheduled_work deadlock

[Stephen Hemminger]

On Fri, 16 Feb 2007 21:20:34 +0100
Francois Romieu <romieu@fr.zoreil.com> wrote:

> Jarek Poplawski <jarkao2@o2.pl> :
> [...]
> > > diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
> > > index 35ad5cf..99304b2 100644
> > > --- a/drivers/net/8139too.c
> > > +++ b/drivers/net/8139too.c
> > > @@ -1109,6 +1109,8 @@ static void __devexit rtl8139_remove_one (struct pci_dev *pdev)
> > >  
> > >  	assert (dev != NULL);
> > >  
> > > +	flush_scheduled_work();
> > > +
> > 
> > IMHO there should be rather cancel_rearming_delayed_work
> > instead of this.
> 
> The delayed_work is initialized even if tp->have_thread is false,
> so cancel_rearming_delayed_work() will work, yes. Feel free to
> send a patch.
> 
> [...]
> > > @@ -1603,18 +1605,21 @@ static void rtl8139_thread (struct work_struct *work)
> > >  	struct net_device *dev = tp->mii.dev;
> > >  	unsigned long thr_delay = next_tick;
> > >  
> > > +	rtnl_lock();
> > > +
> > > +	if (!netif_running(dev))
> > > +		goto out_unlock;
> > 
> > I wonder, why you don't do netif_running before
> > rtnl_lock ? It's an atomic operation. And I'm not sure if increasing
> > rtnl_lock range is really needed here.
> 
> thread    A: netif_running()
> user task B: rtnl_lock()
> user task B: dev->close()
> user task B: rtnl_unlock()
> thread    A: rtnl_lock()
> thread    A: mess with closed device
> 
> Btw, the thread runs every 3*HZ at most.

You need to hold a dev reference (dev_hold) as well. to keep the device
from disappearing. or do a flush_scheduled_work in the remove routine.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH 3/4] 8139too: RTNL and flush_scheduled_work deadlock

[Francois Romieu]

Stephen Hemminger <shemminger@linux-foundation.org> :
[...]
> You need to hold a dev reference (dev_hold) as well. to keep the device
> from disappearing. or do a flush_scheduled_work in the remove routine.

The patched drivers do #2 (before unregister_netdev).

-- 
Ueimor

[PATCH 1/2] RTNL and flush_scheduled_work deadlocks

[Jarek Poplawski]

On Fri, Feb 16, 2007 at 11:04:02AM -0800, Ben Greear wrote:
> Stephen Hemminger wrote:
> >On Thu, 15 Feb 2007 23:40:32 -0800
> >Ben Greear <greearb@candelatech.com> wrote:
> 
> >>Maybe there should be something like an ASSERT_NOT_RTNL() in the 
> >>flush_scheduled_work()
> >>method?  If it's performance criticial, #ifdef it out if we're not 
> >>debugging locks?
> >>
> >
> >You can't safely add a check like that. What if another cpu had acquired
> >RTNL and was unrelated.
> 
> I guess there isn't a way to see if *this* thread is the owner of the RTNL
> currently?  I think lockdep knows the owner...maybe could query it somehow,
> or just save the owner in the mutex object when debugging is enabled...

Here is my patch proposal to enable such thing
(and to make ASSERT_RTNL simpler btw.).

Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>

---

 include/linux/rtnetlink.h |   13 +++++++++++--
 net/core/rtnetlink.c      |   29 ++++++++++++++++++++++++++---
 2 files changed, 37 insertions(+), 5 deletions(-)


diff -Nurp linux-2.6.20-git13-/include/linux/rtnetlink.h linux-2.6.20-git13/include/linux/rtnetlink.h
--- linux-2.6.20-git13-/include/linux/rtnetlink.h	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20-git13/include/linux/rtnetlink.h	2007-02-18 22:56:36.000000000 +0100
@@ -704,16 +704,25 @@ extern int rtnl_trylock(void);
 
 extern void rtnetlink_init(void);
 extern void __rtnl_unlock(void);
+extern int rtnl_assert(void);
 
 #define ASSERT_RTNL() do { \
-	if (unlikely(rtnl_trylock())) { \
-		rtnl_unlock(); \
+	if (unlikely(!rtnl_assert())) { \
 		printk(KERN_ERR "RTNL: assertion failed at %s (%d)\n", \
 		       __FILE__,  __LINE__); \
 		dump_stack(); \
 	} \
 } while(0)
 
+/* Current process shouldn't hold RTNL lock: */
+#define ASSERT_NOT_RTNL() do { \
+	if (unlikely(rtnl_assert())) { \
+		printk(KERN_ERR "NOT_RTNL: assertion failed at %s (%d)\n", \
+		       __FILE__,  __LINE__); \
+		dump_stack(); \
+	} \
+} while(0)
+
 #define BUG_TRAP(x) do { \
 	if (unlikely(!(x))) { \
 		printk(KERN_ERR "KERNEL: assertion (%s) failed at %s (%d)\n", \
diff -Nurp linux-2.6.20-git13-/net/core/rtnetlink.c linux-2.6.20-git13/net/core/rtnetlink.c
--- linux-2.6.20-git13-/net/core/rtnetlink.c	2007-02-15 20:07:11.000000000 +0100
+++ linux-2.6.20-git13/net/core/rtnetlink.c	2007-02-18 22:50:13.000000000 +0100
@@ -57,20 +57,33 @@
 #endif	/* CONFIG_NET_WIRELESS_RTNETLINK */
 
 static DEFINE_MUTEX(rtnl_mutex);
+static struct thread_info *rtnl_owner;
 static struct sock *rtnl;
 
 void rtnl_lock(void)
 {
+#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
+	WARN_ON(rtnl_owner == current_thread_info());
+#endif
 	mutex_lock(&rtnl_mutex);
+	rtnl_owner = current_thread_info();
 }
 
 void __rtnl_unlock(void)
 {
+#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
+	WARN_ON(rtnl_owner != current_thread_info());
+#endif
+	rtnl_owner = NULL;
 	mutex_unlock(&rtnl_mutex);
 }
 
 void rtnl_unlock(void)
 {
+#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
+	WARN_ON(rtnl_owner != current_thread_info());
+#endif
+	rtnl_owner = NULL;
 	mutex_unlock(&rtnl_mutex);
 	if (rtnl && rtnl->sk_receive_queue.qlen)
 		rtnl->sk_data_ready(rtnl, 0);
@@ -79,7 +92,16 @@ void rtnl_unlock(void)
 
 int rtnl_trylock(void)
 {
-	return mutex_trylock(&rtnl_mutex);
+	int ret;
+	
+	if ((ret = mutex_trylock(&rtnl_mutex)))
+		rtnl_owner = current_thread_info();
+	return ret;
+}
+
+int rtnl_assert(void)
+{
+	return (rtnl_owner == current_thread_info());
 }
 
 int rtattr_parse(struct rtattr *tb[], int maxattr, struct rtattr *rta, int len)
@@ -805,9 +827,9 @@ static void rtnetlink_rcv(struct sock *s
 	unsigned int qlen = 0;
 
 	do {
-		mutex_lock(&rtnl_mutex);
+		rtnl_lock();
 		netlink_run_queue(sk, &qlen, &rtnetlink_rcv_msg);
-		mutex_unlock(&rtnl_mutex);
+		__rtnl_unlock();
 
 		netdev_run_todo();
 	} while (qlen);
@@ -893,3 +915,4 @@ EXPORT_SYMBOL(rtnl_unlock);
 EXPORT_SYMBOL(rtnl_unicast);
 EXPORT_SYMBOL(rtnl_notify);
 EXPORT_SYMBOL(rtnl_set_sk_err);
+EXPORT_SYMBOL(rtnl_assert);

Re: [PATCH 1/2] RTNL and flush_scheduled_work deadlocks

[Ben Greear]

Jarek Poplawski wrote:
> On Fri, Feb 16, 2007 at 11:04:02AM -0800, Ben Greear wrote:
>   
>> Stephen Hemminger wrote:
>>     
>>> On Thu, 15 Feb 2007 23:40:32 -0800
>>> Ben Greear <greearb@candelatech.com> wrote:
>>>       
>>>> Maybe there should be something like an ASSERT_NOT_RTNL() in the 
>>>> flush_scheduled_work()
>>>> method?  If it's performance criticial, #ifdef it out if we're not 
>>>> debugging locks?
>>>>
>>>>         
>>> You can't safely add a check like that. What if another cpu had acquired
>>> RTNL and was unrelated.
>>>       
>> I guess there isn't a way to see if *this* thread is the owner of the RTNL
>> currently?  I think lockdep knows the owner...maybe could query it somehow,
>> or just save the owner in the mutex object when debugging is enabled...
>>     
>
> Here is my patch proposal to enable such thing
> (and to make ASSERT_RTNL simpler btw.).
>   
For performance reasons, I'd leave the rtnl_owner inside the
#if debugging locking code....

You are also changing the semantics of ASSERT_RTNL (assert *this thread* 
has rtnl, from the
old behaviour:  assert *some thread* has rtnl).  It may be better this
way, but it could break code that assumes the old behaviour.

Ben


> Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>
>
> ---
>
>  include/linux/rtnetlink.h |   13 +++++++++++--
>  net/core/rtnetlink.c      |   29 ++++++++++++++++++++++++++---
>  2 files changed, 37 insertions(+), 5 deletions(-)
>
>
> diff -Nurp linux-2.6.20-git13-/include/linux/rtnetlink.h linux-2.6.20-git13/include/linux/rtnetlink.h
> --- linux-2.6.20-git13-/include/linux/rtnetlink.h	2007-02-04 19:44:54.000000000 +0100
> +++ linux-2.6.20-git13/include/linux/rtnetlink.h	2007-02-18 22:56:36.000000000 +0100
> @@ -704,16 +704,25 @@ extern int rtnl_trylock(void);
>  
>  extern void rtnetlink_init(void);
>  extern void __rtnl_unlock(void);
> +extern int rtnl_assert(void);
>  
>  #define ASSERT_RTNL() do { \
> -	if (unlikely(rtnl_trylock())) { \
> -		rtnl_unlock(); \
> +	if (unlikely(!rtnl_assert())) { \
>  		printk(KERN_ERR "RTNL: assertion failed at %s (%d)\n", \
>  		       __FILE__,  __LINE__); \
>  		dump_stack(); \
>  	} \
>  } while(0)
>  
> +/* Current process shouldn't hold RTNL lock: */
> +#define ASSERT_NOT_RTNL() do { \
> +	if (unlikely(rtnl_assert())) { \
> +		printk(KERN_ERR "NOT_RTNL: assertion failed at %s (%d)\n", \
> +		       __FILE__,  __LINE__); \
> +		dump_stack(); \
> +	} \
> +} while(0)
> +
>  #define BUG_TRAP(x) do { \
>  	if (unlikely(!(x))) { \
>  		printk(KERN_ERR "KERNEL: assertion (%s) failed at %s (%d)\n", \
> diff -Nurp linux-2.6.20-git13-/net/core/rtnetlink.c linux-2.6.20-git13/net/core/rtnetlink.c
> --- linux-2.6.20-git13-/net/core/rtnetlink.c	2007-02-15 20:07:11.000000000 +0100
> +++ linux-2.6.20-git13/net/core/rtnetlink.c	2007-02-18 22:50:13.000000000 +0100
> @@ -57,20 +57,33 @@
>  #endif	/* CONFIG_NET_WIRELESS_RTNETLINK */
>  
>  static DEFINE_MUTEX(rtnl_mutex);
> +static struct thread_info *rtnl_owner;
>  static struct sock *rtnl;
>  
>  void rtnl_lock(void)
>  {
> +#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
> +	WARN_ON(rtnl_owner == current_thread_info());
> +#endif
>  	mutex_lock(&rtnl_mutex);
> +	rtnl_owner = current_thread_info();
>  }
>  
>  void __rtnl_unlock(void)
>  {
> +#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
> +	WARN_ON(rtnl_owner != current_thread_info());
> +#endif
> +	rtnl_owner = NULL;
>  	mutex_unlock(&rtnl_mutex);
>  }
>  
>  void rtnl_unlock(void)
>  {
> +#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
> +	WARN_ON(rtnl_owner != current_thread_info());
> +#endif
> +	rtnl_owner = NULL;
>  	mutex_unlock(&rtnl_mutex);
>  	if (rtnl && rtnl->sk_receive_queue.qlen)
>  		rtnl->sk_data_ready(rtnl, 0);
> @@ -79,7 +92,16 @@ void rtnl_unlock(void)
>  
>  int rtnl_trylock(void)
>  {
> -	return mutex_trylock(&rtnl_mutex);
> +	int ret;
> +	
> +	if ((ret = mutex_trylock(&rtnl_mutex)))
> +		rtnl_owner = current_thread_info();
> +	return ret;
> +}
> +
> +int rtnl_assert(void)
> +{
> +	return (rtnl_owner == current_thread_info());
>  }
>  
>  int rtattr_parse(struct rtattr *tb[], int maxattr, struct rtattr *rta, int len)
> @@ -805,9 +827,9 @@ static void rtnetlink_rcv(struct sock *s
>  	unsigned int qlen = 0;
>  
>  	do {
> -		mutex_lock(&rtnl_mutex);
> +		rtnl_lock();
>  		netlink_run_queue(sk, &qlen, &rtnetlink_rcv_msg);
> -		mutex_unlock(&rtnl_mutex);
> +		__rtnl_unlock();
>  
>  		netdev_run_todo();
>  	} while (qlen);
> @@ -893,3 +915,4 @@ EXPORT_SYMBOL(rtnl_unlock);
>  EXPORT_SYMBOL(rtnl_unicast);
>  EXPORT_SYMBOL(rtnl_notify);
>  EXPORT_SYMBOL(rtnl_set_sk_err);
> +EXPORT_SYMBOL(rtnl_assert);
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

[PATCH 2/2] RTNL and flush_scheduled_work deadlocks

[Jarek Poplawski]

On Fri, Feb 16, 2007 at 11:04:02AM -0800, Ben Greear wrote:
> Stephen Hemminger wrote:
> >On Thu, 15 Feb 2007 23:40:32 -0800
> >Ben Greear <greearb@candelatech.com> wrote:
> 
> >>Maybe there should be something like an ASSERT_NOT_RTNL() in the 
> >>flush_scheduled_work()
> >>method?  If it's performance criticial, #ifdef it out if we're not 
> >>debugging locks?
> >>
> >
> >You can't safely add a check like that. What if another cpu had acquired
> >RTNL and was unrelated.
> 
> I guess there isn't a way to see if *this* thread is the owner of the RTNL
> currently?  I think lockdep knows the owner...maybe could query it somehow,
> or just save the owner in the mutex object when debugging is enabled...


And here is my patch proposal to fix the whole problem
other way: by enabling all those flushes with RTNL.

The main idea is the function calling flush has RTNL
lock but, while flushing, it doesn't use this for
anything else. So it can "lend" this lock to work
functions being flushed. If work function needs RTNL
lock and it is running under a flush, it can do like
it had this lock because a function with a real RTNL
lock doesn't do anything except waiting for a flush
end. And if there is no flush, this work function
simply can use the real RTNL lock. (And of course these
work functions can't call a flush directly or
indirectly themselves!)

So to use this we only need such changes:

 ... some_delayed_work_func(...)
 {
	...
-	rtnl_lock();
+	rtnl_lock_work();
	...
-	rtnl_unlock();
+	rtnl_unlock_work();
 }

 ... some_delayed_work_func(...)
 {
	...
+	rtnl_set_flush();
	flush_scheduled_work;
	/* or cancel_rearming_delayed_work(...); etc. */
+	rtnl_unset_flush();
 }

(Or there could be added some wrappers like:
rtnl_flush_scheduled_work etc. with these rtnl_set/unset
included.)

PS: destroy_workqueue() is one of dangerous too.

This patch should be applied after my earlier:
PATCH 1/2. (But if this idea is wrong, PATCH 1/2
could be used independently.)

Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>

---

 include/linux/rtnetlink.h |   15 ++++++++++
 net/core/rtnetlink.c      |   65 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)


diff -Nurp linux-2.6.20-git13-rtnl1-/include/linux/rtnetlink.h linux-2.6.20-git13-rtnl1/include/linux/rtnetlink.h
--- linux-2.6.20-git13-rtnl1-/include/linux/rtnetlink.h	2007-02-18 23:05:26.000000000 +0100
+++ linux-2.6.20-git13-rtnl1/include/linux/rtnetlink.h	2007-02-18 23:25:28.000000000 +0100
@@ -706,6 +706,21 @@ extern void rtnetlink_init(void);
 extern void __rtnl_unlock(void);
 extern int rtnl_assert(void);
 
+/* 
+ * To use in work and delayed_work functions instead of rtnl_lock etc.
+ * (rtnl_unlock_work should be used instead of __rtnl_unlock and rtnl_unlock;
+ * rtnl_assert, ASSERT_RTNL and ASSERT_NOT_RTNL need no changes):
+ */
+extern void rtnl_lock_work(void);
+extern void rtnl_unlock_work(void);
+extern int rtnl_trylock_work(void);
+/*
+ * To use by functions holding RTNL lock - just before and after calling
+ * flush_scheduled_work or other functions with workqueue flushing:
+ */
+extern void rtnl_set_flush(void);
+extern void rtnl_unset_flush(void);
+
 #define ASSERT_RTNL() do { \
 	if (unlikely(!rtnl_assert())) { \
 		printk(KERN_ERR "RTNL: assertion failed at %s (%d)\n", \
diff -Nurp linux-2.6.20-git13-rtnl1-/net/core/rtnetlink.c linux-2.6.20-git13-rtnl1/net/core/rtnetlink.c
--- linux-2.6.20-git13-rtnl1-/net/core/rtnetlink.c	2007-02-18 22:50:13.000000000 +0100
+++ linux-2.6.20-git13-rtnl1/net/core/rtnetlink.c	2007-02-18 22:49:16.000000000 +0100
@@ -58,6 +58,11 @@
 
 static DEFINE_MUTEX(rtnl_mutex);
 static struct thread_info *rtnl_owner;
+/* rtnl_mutex proxy while flushing workqueue: */
+static DEFINE_MUTEX(rtnl_mutex_work);
+/* set/unset under rtnl_mutex by flush caller: */
+static struct thread_info *rtnl_flush_owner;
+
 static struct sock *rtnl;
 
 void rtnl_lock(void)
@@ -104,6 +109,61 @@ int rtnl_assert(void)
 	return (rtnl_owner == current_thread_info());
 }
 
+void rtnl_lock_work(void)
+{
+#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
+	WARN_ON(rtnl_owner == current_thread_info());
+#endif
+	mutex_lock(&rtnl_mutex_work);
+	if (!rtnl_flush_owner)
+		mutex_lock(&rtnl_mutex);
+
+	rtnl_owner = current_thread_info();
+}
+
+void rtnl_unlock_work(void)
+{
+#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
+	WARN_ON(rtnl_owner != current_thread_info());
+#endif
+	rtnl_owner = rtnl_flush_owner;
+	if (!rtnl_flush_owner)
+		mutex_unlock(&rtnl_mutex);
+
+	mutex_unlock(&rtnl_mutex_work);
+}
+
+int rtnl_trylock_work(void)
+{
+	int ret;
+
+	if ((ret = mutex_trylock(&rtnl_mutex_work)))
+		if (!rtnl_flush_owner)
+			ret = mutex_trylock(&rtnl_mutex);
+
+	if (ret)
+		rtnl_owner = current_thread_info();
+
+	return ret;
+}
+
+void rtnl_set_flush(void)
+{
+#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
+	WARN_ON(rtnl_owner != current_thread_info() || rtnl_flush_owner);
+#endif
+	rtnl_flush_owner = current_thread_info();
+}
+
+void rtnl_unset_flush(void)
+{
+#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
+	WARN_ON(rtnl_owner != current_thread_info() ||
+		rtnl_owner != rtnl_flush_owner);
+#endif
+	rtnl_flush_owner = NULL;
+}
+
 int rtattr_parse(struct rtattr *tb[], int maxattr, struct rtattr *rta, int len)
 {
 	memset(tb, 0, sizeof(struct rtattr*)*maxattr);
@@ -916,3 +976,8 @@ EXPORT_SYMBOL(rtnl_unicast);
 EXPORT_SYMBOL(rtnl_notify);
 EXPORT_SYMBOL(rtnl_set_sk_err);
 EXPORT_SYMBOL(rtnl_assert);
+EXPORT_SYMBOL(rtnl_lock_work);
+EXPORT_SYMBOL(rtnl_trylock_work);
+EXPORT_SYMBOL(rtnl_unlock_work);
+EXPORT_SYMBOL(rtnl_set_flush);
+EXPORT_SYMBOL(rtnl_unset_flush);

Re: [PATCH 1/2] RTNL and flush_scheduled_work deadlocks

[Jarek Poplawski]

On Sun, Feb 18, 2007 at 10:27:19PM -0800, Ben Greear wrote:
> Jarek Poplawski wrote:
> >On Fri, Feb 16, 2007 at 11:04:02AM -0800, Ben Greear wrote:
> >  
> >>Stephen Hemminger wrote:
> >>    
> >>>On Thu, 15 Feb 2007 23:40:32 -0800
> >>>Ben Greear <greearb@candelatech.com> wrote:
> >>>      
> >>>>Maybe there should be something like an ASSERT_NOT_RTNL() in the 
> >>>>flush_scheduled_work()
> >>>>method?  If it's performance criticial, #ifdef it out if we're not 
> >>>>debugging locks?
> >>>>
> >>>>        
> >>>You can't safely add a check like that. What if another cpu had acquired
> >>>RTNL and was unrelated.
> >>>      
> >>I guess there isn't a way to see if *this* thread is the owner of the RTNL
> >>currently?  I think lockdep knows the owner...maybe could query it 
> >>somehow,
> >>or just save the owner in the mutex object when debugging is enabled...
> >>    
> >
> >Here is my patch proposal to enable such thing
> >(and to make ASSERT_RTNL simpler btw.).
> >  
> For performance reasons, I'd leave the rtnl_owner inside the
> #if debugging locking code....

This is needed with my second patch. But it is only
proposal, so all could be enhanced of course.
But I don't thing current ASSERT_RTNL has anything
to do with performance. And after all it's for mutex
(slow) path, so I'm not sure if performance is such a
problem.

> You are also changing the semantics of ASSERT_RTNL (assert *this thread* 
> has rtnl, from the
> old behaviour:  assert *some thread* has rtnl).  It may be better this
> way, but it could break code that assumes the old behaviour.

Sure, this should be verified. But this old behavior isn't
very fast and reliable (there is a possibility, we are
asserted wrongly because RTNL lock was held at the moment
by somebody else).

Cheers,
Jarek P.

Re: [PATCH 2/2] RTNL and flush_scheduled_work deadlocks

[Jarek Poplawski]

On Mon, Feb 19, 2007 at 07:55:48AM +0100, Jarek Poplawski wrote:
...
> So to use this we only need such changes:
> 
>  ... some_delayed_work_func(...)
>  {
> 	...
> -	rtnl_lock();
> +	rtnl_lock_work();
> 	...
> -	rtnl_unlock();
> +	rtnl_unlock_work();
>  }
> 
>  ... some_delayed_work_func(...)

Sorry! Of course:

  ... some_closing_with_flush_func(...)
>  {
> 	...
> +	rtnl_set_flush();
> 	flush_scheduled_work;
> 	/* or cancel_rearming_delayed_work(...); etc. */
> +	rtnl_unset_flush();
>  }

Jarek P.

Re: [PATCH 1/2] RTNL and flush_scheduled_work deadlocks

[Jarek Poplawski]

On Mon, Feb 19, 2007 at 08:11:59AM +0100, Jarek Poplawski wrote:
> On Sun, Feb 18, 2007 at 10:27:19PM -0800, Ben Greear wrote:
..
> > You are also changing the semantics of ASSERT_RTNL (assert *this thread* 
> > has rtnl, from the
> > old behaviour:  assert *some thread* has rtnl).  It may be better this
> > way, but it could break code that assumes the old behaviour.

If any code could assume the old behaviour it's simply
a bug (eg. doing ASSERT_RTNL two times one after
another could give different results). And it's
logically wrong to: the same process is trying to
acquire the same lock second time (pseudo recursively:
it makes difficult lock verifying eg. by lockdep). 

Jarek P.

Re: [PATCH 3/4] 8139too: RTNL and flush_scheduled_work deadlock

[Jarek Poplawski]

On Fri, Feb 16, 2007 at 09:20:34PM +0100, Francois Romieu wrote:
> Jarek Poplawski <jarkao2@o2.pl> :
...
> > > @@ -1603,18 +1605,21 @@ static void rtl8139_thread (struct work_struct *work)
> > >  	struct net_device *dev = tp->mii.dev;
> > >  	unsigned long thr_delay = next_tick;
> > >  
> > > +	rtnl_lock();
> > > +
> > > +	if (!netif_running(dev))
> > > +		goto out_unlock;
> > 
> > I wonder, why you don't do netif_running before
> > rtnl_lock ? It's an atomic operation. And I'm not sure if increasing
> > rtnl_lock range is really needed here.
> 
> thread    A: netif_running()
> user task B: rtnl_lock()
> user task B: dev->close()
> user task B: rtnl_unlock()
> thread    A: rtnl_lock()
> thread    A: mess with closed device
> 
> Btw, the thread runs every 3*HZ at most.

You are right (mostly)! But I think rtnl_lock is special
and should be spared (even this 3*HZ) and here it's used
for some mainly internal purpose (close synchronization).
And it looks like mainly for this internal reason holding
of rtnl_lock is increased. And because rtnl_lock is quite
popular you have to take into consideration that after
this 3*HZ it could spend some time waiting for the lock.
So, maybe it would be nicer to check this netif_running
twice (after rtnl_lock where needed), but maybe it's a
mater of taste only, and yours is better, as well.
(Btw. I didn't verify this, but I hope you checked that
places not under rtnl_lock before the patch are safe from
some locking problems now.)

Jarek P.

Re: [PATCH 3/4] 8139too: RTNL and flush_scheduled_work deadlock

[Francois Romieu]

Cc: list trimmed.

Jarek Poplawski <jarkao2@o2.pl> :
> On Fri, Feb 16, 2007 at 09:20:34PM +0100, Francois Romieu wrote:
[...]
> > Btw, the thread runs every 3*HZ at most.
> 
> You are right (mostly)! But I think rtnl_lock is special
> and should be spared (even this 3*HZ) and here it's used
> for some mainly internal purpose (close synchronization).
> And it looks like mainly for this internal reason holding
> of rtnl_lock is increased. And because rtnl_lock is quite
> popular you have to take into consideration that after
> this 3*HZ it could spend some time waiting for the lock.
> So, maybe it would be nicer to check this netif_running
> twice (after rtnl_lock where needed), but maybe it's a
> mater of taste only, and yours is better, as well.

The region protected by RTNL has been widened to include a tx_timeout
handler. It is supposed to handle an occasional error, something that
should not even happen at 3*HZ. Optimizing it is useless, especially
on an high-end performer like the 8139.

> (Btw. I didn't verify this, but I hope you checked that
> places not under rtnl_lock before the patch are safe from
> some locking problems now.)

I did. It is not a reason to trust the patch though.

-- 
Ueimor

Re: [BUG] RTNL and flush_scheduled_work deadlocks

[Jarek Poplawski]

On Fri, Feb 16, 2007 at 08:06:25AM -0800, Ben Greear wrote:
...
> Well, I had lockdep and all of the locking debugging I could find 
> enabled, but
> it did not catch this problem..I had to use sysctl -t and manually dig 
> through the backtraces
> to find the deadlock....
> 
> It may be that lockdep could be enhanced to catch this sort of thing....

I think you are really good at traceing very interesting
(subtle) problems.

I guess the scenario is like this:

1) some process takes some lock (e.g. RTNL), 
2) kthread runs a work function, which tries to get the
   same lock,
3) the process with the lock calls flush_scheduled_work,
4) the flush_cpu_workqueue waits for kthread to finish.

So, the process #1 (with the lock) waits for the end 
of the process #2, which waits for the lock held by
process #1.

Of course it's a lockup - similar to circular dependency
but not the same: there is only one lock. I don't think
lockdep could be blamed here - if it's not a lock it
can't know the reason of process' #1 waiting.

In my opinion the solution should be looked for in the
workqueue code. My idea is: maybe there should be used
some additional lock taken by kthread before running
the workqueue and by a process calling the flush. Then
lockdep shouldn't have any problems with this dependency.
This lock could be #ifdef DEBUG_LOCK... so only where
it could be analyzed. Of course there may be some simpler
solution of this otherwise hard to track problem.

I CC this message to Ingo Molnar and hope he could find
some time to think about it.

Regards,
Jarek P.

Re: [PATCH 1/4] r8169: RTNL and flush_scheduled_work deadlock

[Jeff Garzik]

Francois Romieu wrote:
> flush_scheduled_work() in net_device->close has a slight tendency
> to deadlock with tasks on the workqueue that hold RTNL.
> 
> rtl8169_close/down simply need the recovery tasks to not meddle
> with the hardware while the device is going down.
> 
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> ---
>  drivers/net/r8169.c |   25 ++++++++++++++++++-------
>  1 files changed, 18 insertions(+), 7 deletions(-)

applied 1-4

[PATCH v.2] RTNL and flush_scheduled_work deadlocks

[Jarek Poplawski]

On Sun, Feb 18, 2007 at 10:27:19PM -0800, Ben Greear wrote:
> Jarek Poplawski wrote:
> >On Fri, Feb 16, 2007 at 11:04:02AM -0800, Ben Greear wrote:
...
> >>>On Thu, 15 Feb 2007 23:40:32 -0800
> >>>Ben Greear <greearb@candelatech.com> wrote:
> >>>      
> >>>>Maybe there should be something like an ASSERT_NOT_RTNL() in the 
> >>>>flush_scheduled_work()
> >>>>method?  If it's performance criticial, #ifdef it out if we're not 
> >>>>debugging locks?
...
> For performance reasons, I'd leave the rtnl_owner inside the
> #if debugging locking code....
> 
> You are also changing the semantics of ASSERT_RTNL (assert *this thread* 
> has rtnl, from the
> old behaviour:  assert *some thread* has rtnl).  It may be better this
> way, but it could break code that assumes the old behaviour.
> 
> Ben

Hi,

I'm not sure anybody is interested yet, but I think it's
a good idea so here is a reworked proposal. 

Jarek P.
===

[NET] DEBUG_ASSERT_RTNL and DEBUG_ASSERT_NOT_RTNL macros

Debug RTNL macros usable with CONFIG_DEBUG_MUTEXES on.
Based on suggestions of Ben Greear (to help debugging
flush_schedule_work).

Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>
Cc: Ben Greear <greearb@candelatech.com>

---

 include/linux/rtnetlink.h |   27 +++++++++++++++++++++++++++
 net/core/rtnetlink.c      |   10 ++++++++++
 2 files changed, 37 insertions(+)


diff -Nurp linux-2.6.21-rc2-git2-/include/linux/rtnetlink.h linux-2.6.21-rc2-git2/include/linux/rtnetlink.h
--- linux-2.6.21-rc2-git2-/include/linux/rtnetlink.h	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.21-rc2-git2/include/linux/rtnetlink.h	2007-03-04 14:47:00.000000000 +0100
@@ -728,6 +728,33 @@ rtattr_failure:
 	return table;
 }
 
+#ifdef CONFIG_DEBUG_MUTEXES
+extern int debug_rtnl_assert(void);
+
+#define DEBUG_ASSERT_RTNL() do { \
+	if (unlikely(!debug_rtnl_assert())) { \
+		printk(KERN_ERR "DEBUG RTNL:" \
+		       " assertion failed at %s (%d)\n", \
+		       __FILE__,  __LINE__); \
+		dump_stack(); \
+	} \
+} while(0)
+
+#define DEBUG_ASSERT_NOT_RTNL() do { \
+	if (unlikely(debug_rtnl_assert())) { \
+		printk(KERN_ERR "DEBUG NOT RTNL:" \
+		       " assertion failed at %s (%d)\n", \
+		       __FILE__,  __LINE__); \
+		dump_stack(); \
+	} \
+} while(0)
+
+#else
+/* debug_rtnl_assert() invalid here */
+#define DEBUG_ASSERT_RTNL()
+#define DEBUG_ASSERT_NOT_RTNL()
+#endif /* CONFIG_DEBUG_MUTEXES */
+
 #endif /* __KERNEL__ */
 
 
diff -Nurp linux-2.6.21-rc2-git2-/net/core/rtnetlink.c linux-2.6.21-rc2-git2/net/core/rtnetlink.c
--- linux-2.6.21-rc2-git2-/net/core/rtnetlink.c	2007-02-21 19:46:47.000000000 +0100
+++ linux-2.6.21-rc2-git2/net/core/rtnetlink.c	2007-03-04 15:23:34.000000000 +0100
@@ -82,6 +82,13 @@ int rtnl_trylock(void)
 	return mutex_trylock(&rtnl_mutex);
 }
 
+#ifdef CONFIG_DEBUG_MUTEXES
+int debug_rtnl_assert(void)
+{
+	return (rtnl_mutex.owner == current_thread_info());
+}
+#endif
+
 int rtattr_parse(struct rtattr *tb[], int maxattr, struct rtattr *rta, int len)
 {
 	memset(tb, 0, sizeof(struct rtattr*)*maxattr);
@@ -893,3 +900,6 @@ EXPORT_SYMBOL(rtnl_unlock);
 EXPORT_SYMBOL(rtnl_unicast);
 EXPORT_SYMBOL(rtnl_notify);
 EXPORT_SYMBOL(rtnl_set_sk_err);
+#ifdef CONFIG_DEBUG_MUTEXES
+EXPORT_SYMBOL(debug_rtnl_assert);
+#endif

Re: [PATCH 3/4] 8139too: RTNL and flush_scheduled_work deadlock

[Ben Greear]

Francois Romieu wrote:
> Your usual dont-flush_scheduled_work-with-RTNL-held stuff.
> 
> It is a bit different here since the thread runs permanently
> or is only occasionally kicked for recovery depending on the
> hardware revision.

It looks like this has not made it into the 2.6.20 stable series
patches...  Any reason not to add it there?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 3/4] 8139too: RTNL and flush_scheduled_work deadlock

[Francois Romieu]

Ben Greear <greearb@candelatech.com> :
[...]
> It looks like this has not made it into the 2.6.20 stable series
> patches...  Any reason not to add it there?

No. Go ahead and submit it.

-- 
Ueimor

Anybody got a battery for my Ultra 10 ?