6lowpan: lockless tx queue of routing netlink device

6lowpan: lockless tx queue of routing netlink device

[Alexander Aring]

Hi,

I got some locking issues with CONFIG_PROVE_LOCKING enabled and need help.

Full output:

=============================================
[ INFO: possible recursive locking detected ]
3.13.0-08605-g8f2b630-dirty #105 Not tainted
---------------------------------------------
agetty/841 is trying to acquire lock:
 (_xmit_IEEE802154#2){+.-...}, at: [<c0356b39>] sch_direct_xmit+0x34/0x122

but task is already holding lock:
 (_xmit_IEEE802154#2){+.-...}, at: [<c0346926>] __dev_queue_xmit+0x26e/0x329

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(_xmit_IEEE802154#2);
  lock(_xmit_IEEE802154#2);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

6 locks held by agetty/841:
 #0:  (((&idev->mc_ifc_timer))){+.-...}, at: [<c012b6f2>] call_timer_fn+0x0/0xb3
 #1:  (rcu_read_lock){.+.+..}, at: [<c03b4335>] rcu_read_lock+0x0/0x21
 #2:  (rcu_read_lock_bh){.+....}, at: [<c039d39d>] rcu_lock_acquire+0x0/0x1c
 #3:  (rcu_read_lock_bh){.+....}, at: [<c03426e5>] rcu_lock_acquire+0x0/0x1c
 #4:  (_xmit_IEEE802154#2){+.-...}, at: [<c0346926>] __dev_queue_xmit+0x26e/0x329
 #5:  (rcu_read_lock_bh){.+....}, at: [<c03426e5>] rcu_lock_acquire+0x0/0x1c



The solution was for me to change
dev->type = ARPHRD_IEEE802154
to
dev->type = ARPHRD_6LOWPAN
of the rtnl device. What we really shall do in the near future. (I have
a patch for this).


Another solution was to add:
dev->features           |= NETIF_F_LLTX;
in setup callback of rtnl device.


This enables a lockless tx queue.
I am not sure if we can do a lockless tx queue here and the comment of
NETIF_F_LLTX says it's deprecated "/* do not use LLTX in new drivers */".
Exists there some alternative for this?



So a little bit more information about the current architecture which is
a little bit complex for tx. Maybe then it's more clear how to fix this
issue correctly.

To setup a lowpan interface you need to run:
"ip link add link $WPAN_INTERFACE name $LOWPAN_INTERFACE type lowpan"

This setups a lowpan interface which "sitting" on top of the
$WPAN_INTERFACE.

The lowpan rtnl implementation saves pointers from both interfaces we
name it:

"real_dev" <-- WPAN_INTERFACE
"dev" <-- LOWPAN_INTERFACE


If we get some "usually" ipv6 packets from LOWPAN_INTERFACE which calls
header_create function, then we doing some manipulating of sk_buff there.

After this we calling dev_hard_header with the callback of
WPAN_INTERFACE to generate the mac header.

Then we are in the xmit callback of LOWPAN_INTERFACE and doing a
skb->dev pointer change from LOWPAN_INTERFACE to the WPAN_INTERFACE and
calling dev_queue_xmit to send it via the WPAN_INTERFACE.
The skb->dev switch is necessary because we call then the xmit callback
of the WPAN_INTERFACE, the LOWPAN_INTERFACE is more a "virtual" interface.

I think that's the problem. We have two dev_queue_xmit calls first from 
LOWPAN_INTERFACE then the WPAN_INTERFACE, so we locking something twice.


That's very much complicated and I think we doing some hacked stuff
there but currently it works so (except the locking issue). :-)

- Alex

------------------------------------------------------------------------------
Managing the Performance of Cloud-Based Applications
Take advantage of what the Cloud has to offer - Avoid Common Pitfalls.
Read the Whitepaper.
http://pubads.g.doubleclick.net/gampad/clk?id=121051231&iu=/4140/ostg.clktrk

Re: 6lowpan: lockless tx queue of routing netlink device

[Eric Dumazet]

On Sun, 2014-02-09 at 11:20 +0100, Alexander Aring wrote:
> Hi,
> 
> I got some locking issues with CONFIG_PROVE_LOCKING enabled and need help.
> 
> Full output:
> 
> =============================================
> [ INFO: possible recursive locking detected ]
> 3.13.0-08605-g8f2b630-dirty #105 Not tainted
> ---------------------------------------------
> agetty/841 is trying to acquire lock:
>  (_xmit_IEEE802154#2){+.-...}, at: [<c0356b39>] sch_direct_xmit+0x34/0x122
> 
> but task is already holding lock:
>  (_xmit_IEEE802154#2){+.-...}, at: [<c0346926>] __dev_queue_xmit+0x26e/0x329
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(_xmit_IEEE802154#2);
>   lock(_xmit_IEEE802154#2);
> 
>  *** DEADLOCK ***
> 
>  May be due to missing lock nesting notation
> 
> 6 locks held by agetty/841:
>  #0:  (((&idev->mc_ifc_timer))){+.-...}, at: [<c012b6f2>] call_timer_fn+0x0/0xb3
>  #1:  (rcu_read_lock){.+.+..}, at: [<c03b4335>] rcu_read_lock+0x0/0x21
>  #2:  (rcu_read_lock_bh){.+....}, at: [<c039d39d>] rcu_lock_acquire+0x0/0x1c
>  #3:  (rcu_read_lock_bh){.+....}, at: [<c03426e5>] rcu_lock_acquire+0x0/0x1c
>  #4:  (_xmit_IEEE802154#2){+.-...}, at: [<c0346926>] __dev_queue_xmit+0x26e/0x329
>  #5:  (rcu_read_lock_bh){.+....}, at: [<c03426e5>] rcu_lock_acquire+0x0/0x1c
> 
> 
> 
> The solution was for me to change
> dev->type = ARPHRD_IEEE802154
> to
> dev->type = ARPHRD_6LOWPAN
> of the rtnl device. What we really shall do in the near future. (I have
> a patch for this).
> 
> 
> Another solution was to add:
> dev->features           |= NETIF_F_LLTX;
> in setup callback of rtnl device.
> 
> 
> This enables a lockless tx queue.
> I am not sure if we can do a lockless tx queue here and the comment of
> NETIF_F_LLTX says it's deprecated "/* do not use LLTX in new drivers */".
> Exists there some alternative for this?
> 
> 
> 
> So a little bit more information about the current architecture which is
> a little bit complex for tx. Maybe then it's more clear how to fix this
> issue correctly.
> 
> To setup a lowpan interface you need to run:
> "ip link add link $WPAN_INTERFACE name $LOWPAN_INTERFACE type lowpan"
> 
> This setups a lowpan interface which "sitting" on top of the
> $WPAN_INTERFACE.
> 
> The lowpan rtnl implementation saves pointers from both interfaces we
> name it:
> 
> "real_dev" <-- WPAN_INTERFACE
> "dev" <-- LOWPAN_INTERFACE
> 
> 
> If we get some "usually" ipv6 packets from LOWPAN_INTERFACE which calls
> header_create function, then we doing some manipulating of sk_buff there.
> 
> After this we calling dev_hard_header with the callback of
> WPAN_INTERFACE to generate the mac header.
> 
> Then we are in the xmit callback of LOWPAN_INTERFACE and doing a
> skb->dev pointer change from LOWPAN_INTERFACE to the WPAN_INTERFACE and
> calling dev_queue_xmit to send it via the WPAN_INTERFACE.
> The skb->dev switch is necessary because we call then the xmit callback
> of the WPAN_INTERFACE, the LOWPAN_INTERFACE is more a "virtual" interface.
> 
> I think that's the problem. We have two dev_queue_xmit calls first from 
> LOWPAN_INTERFACE then the WPAN_INTERFACE, so we locking something twice.
> 
> 
> That's very much complicated and I think we doing some hacked stuff
> there but currently it works so (except the locking issue). :-)

Please try the following fix, thanks for this report !

diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
index 48b25c0af4d0..069af33013c4 100644
--- a/net/ieee802154/6lowpan.c
+++ b/net/ieee802154/6lowpan.c
@@ -533,7 +533,27 @@ static struct header_ops lowpan_header_ops = {
 	.create	= lowpan_header_create,
 };
 
+static struct lock_class_key lowpan_tx_busylock;
+static struct lock_class_key lowpan_netdev_xmit_lock_key;
+
+static void lowpan_set_lockdep_class_one(struct net_device *dev,
+					 struct netdev_queue *txq,
+					 void *_unused)
+{
+	lockdep_set_class(&txq->_xmit_lock,
+			  &lowpan_netdev_xmit_lock_key);
+}
+
+
+static int lowpan_dev_init(struct net_device *dev)
+{
+	netdev_for_each_tx_queue(dev, lowpan_set_lockdep_class_one, NULL);
+	dev->qdisc_tx_busylock = &lowpan_tx_busylock;
+	return 0;
+}
+
 static const struct net_device_ops lowpan_netdev_ops = {
+	.ndo_init		= lowpan_dev_init,
 	.ndo_start_xmit		= lowpan_xmit,
 	.ndo_set_mac_address	= lowpan_set_address,
 };

Re: 6lowpan: lockless tx queue of routing netlink device

[Alexander Aring]

Hi Eric,

On Sun, Feb 09, 2014 at 04:41:47AM -0800, Eric Dumazet wrote:
> 
> Please try the following fix, thanks for this report !
> 
> diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
> index 48b25c0af4d0..069af33013c4 100644
> --- a/net/ieee802154/6lowpan.c
> +++ b/net/ieee802154/6lowpan.c
> @@ -533,7 +533,27 @@ static struct header_ops lowpan_header_ops = {
>  	.create	= lowpan_header_create,
>  };
>  
> +static struct lock_class_key lowpan_tx_busylock;
> +static struct lock_class_key lowpan_netdev_xmit_lock_key;
> +
> +static void lowpan_set_lockdep_class_one(struct net_device *dev,
> +					 struct netdev_queue *txq,
> +					 void *_unused)
> +{
> +	lockdep_set_class(&txq->_xmit_lock,
> +			  &lowpan_netdev_xmit_lock_key);
> +}
> +
> +
> +static int lowpan_dev_init(struct net_device *dev)
> +{
> +	netdev_for_each_tx_queue(dev, lowpan_set_lockdep_class_one, NULL);
> +	dev->qdisc_tx_busylock = &lowpan_tx_busylock;
> +	return 0;
> +}
> +
>  static const struct net_device_ops lowpan_netdev_ops = {
> +	.ndo_init		= lowpan_dev_init,
>  	.ndo_start_xmit		= lowpan_xmit,
>  	.ndo_set_mac_address	= lowpan_set_address,
>  };

thanks, this fixed the issue. What we should do as next?

Should I create a patch for this or do you want to make a patch?

- Alex

------------------------------------------------------------------------------
Android™ apps run on BlackBerry®10
Introducing the new BlackBerry 10.2.1 Runtime for Android apps.
Now with support for Jelly Bean, Bluetooth, Mapview and more.
Get your Android app in front of a whole new audience.  Start now.
http://pubads.g.doubleclick.net/gampad/clk?id=124407151&iu=/4140/ostg.clktrk

Re: 6lowpan: lockless tx queue of routing netlink device

[Eric Dumazet]

On Mon, 2014-02-10 at 18:33 +0100, Alexander Aring wrote:
> Hi Eric,
> 
> On Sun, Feb 09, 2014 at 04:41:47AM -0800, Eric Dumazet wrote:
> > 
> > Please try the following fix, thanks for this report !
> > 
> > diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
> > index 48b25c0af4d0..069af33013c4 100644
> > --- a/net/ieee802154/6lowpan.c
> > +++ b/net/ieee802154/6lowpan.c
> > @@ -533,7 +533,27 @@ static struct header_ops lowpan_header_ops = {
> >  	.create	= lowpan_header_create,
> >  };
> >  
> > +static struct lock_class_key lowpan_tx_busylock;
> > +static struct lock_class_key lowpan_netdev_xmit_lock_key;
> > +
> > +static void lowpan_set_lockdep_class_one(struct net_device *dev,
> > +					 struct netdev_queue *txq,
> > +					 void *_unused)
> > +{
> > +	lockdep_set_class(&txq->_xmit_lock,
> > +			  &lowpan_netdev_xmit_lock_key);
> > +}
> > +
> > +
> > +static int lowpan_dev_init(struct net_device *dev)
> > +{
> > +	netdev_for_each_tx_queue(dev, lowpan_set_lockdep_class_one, NULL);
> > +	dev->qdisc_tx_busylock = &lowpan_tx_busylock;
> > +	return 0;
> > +}
> > +
> >  static const struct net_device_ops lowpan_netdev_ops = {
> > +	.ndo_init		= lowpan_dev_init,
> >  	.ndo_start_xmit		= lowpan_xmit,
> >  	.ndo_set_mac_address	= lowpan_set_address,
> >  };
> 
> thanks, this fixed the issue. What we should do as next?
> 
> Should I create a patch for this or do you want to make a patch?

I'll take care of this, don't worry ;)

Thanks for testing !



------------------------------------------------------------------------------
Android™ apps run on BlackBerry®10
Introducing the new BlackBerry 10.2.1 Runtime for Android apps.
Now with support for Jelly Bean, Bluetooth, Mapview and more.
Get your Android app in front of a whole new audience.  Start now.
http://pubads.g.doubleclick.net/gampad/clk?id=124407151&iu=/4140/ostg.clktrk

Re: 6lowpan: lockless tx queue of routing netlink device

[Alexander Aring]

Hi,

On Mon, Feb 10, 2014 at 10:16:26AM -0800, Eric Dumazet wrote:
> On Mon, 2014-02-10 at 18:33 +0100, Alexander Aring wrote:
> > Hi Eric,
> > 
> > On Sun, Feb 09, 2014 at 04:41:47AM -0800, Eric Dumazet wrote:
> > > 
> > > Please try the following fix, thanks for this report !
> > > 
> > > diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
> > > index 48b25c0af4d0..069af33013c4 100644
> > > --- a/net/ieee802154/6lowpan.c
> > > +++ b/net/ieee802154/6lowpan.c
> > > @@ -533,7 +533,27 @@ static struct header_ops lowpan_header_ops = {
> > >  	.create	= lowpan_header_create,
> > >  };
> > >  
> > > +static struct lock_class_key lowpan_tx_busylock;
> > > +static struct lock_class_key lowpan_netdev_xmit_lock_key;
> > > +
> > > +static void lowpan_set_lockdep_class_one(struct net_device *dev,
> > > +					 struct netdev_queue *txq,
> > > +					 void *_unused)
> > > +{
> > > +	lockdep_set_class(&txq->_xmit_lock,
> > > +			  &lowpan_netdev_xmit_lock_key);
> > > +}
> > > +
> > > +
> > > +static int lowpan_dev_init(struct net_device *dev)
> > > +{
> > > +	netdev_for_each_tx_queue(dev, lowpan_set_lockdep_class_one, NULL);
> > > +	dev->qdisc_tx_busylock = &lowpan_tx_busylock;
> > > +	return 0;
> > > +}
> > > +
> > >  static const struct net_device_ops lowpan_netdev_ops = {
> > > +	.ndo_init		= lowpan_dev_init,
> > >  	.ndo_start_xmit		= lowpan_xmit,
> > >  	.ndo_set_mac_address	= lowpan_set_address,
> > >  };
> > 
> > thanks, this fixed the issue. What we should do as next?
> > 
> > Should I create a patch for this or do you want to make a patch?
> 
> I'll take care of this, don't worry ;)
> 
> Thanks for testing !
> 
no problem. Maybe you can try to give me some explanation what you did
there?

I see you make a lockdep_set_class for each tx queue and assign the 
lowpan_netdev_xmit_lock_key. So why it's better to make a own
"lock_key".

thanks.

- Alex

Re: 6lowpan: lockless tx queue of routing netlink device

[Eric Dumazet]

On Mon, 2014-02-10 at 19:21 +0100, Alexander Aring wrote:

> no problem. Maybe you can try to give me some explanation what you did
> there?
> 
> I see you make a lockdep_set_class for each tx queue and assign the 
> lowpan_netdev_xmit_lock_key. So why it's better to make a own
> "lock_key".

This will be explained in the patch I'll cook.

Re: 6lowpan: lockless tx queue of routing netlink device

[Alexander Aring]

On Mon, Feb 10, 2014 at 11:26:12AM -0800, Eric Dumazet wrote:
> On Mon, 2014-02-10 at 19:21 +0100, Alexander Aring wrote:
> 
> > no problem. Maybe you can try to give me some explanation what you did
> > there?
> > 
> > I see you make a lockdep_set_class for each tx queue and assign the 
> > lowpan_netdev_xmit_lock_key. So why it's better to make a own
> > "lock_key".
> 
> This will be explained in the patch I'll cook.
> 
Okay, then you can add a

Tested-by: Alexander Aring <alex.aring@gmail.com>

Thanks!

- Alex

[PATCH] 6lowpan: fix lockdep splats

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

When a device ndo_start_xmit() calls again dev_queue_xmit(),
lockdep can complain because dev_queue_xmit() is re-entered and the
spinlocks protecting tx queues share a common lockdep class.

Same issue was fixed for bonding/l2tp/ppp in commits 

0daa2303028a6 ("[PATCH] bonding: lockdep annotation")
49ee49202b4ac ("bonding: set qdisc_tx_busylock to avoid LOCKDEP splat")
23d3b8bfb8eb2 ("net: qdisc busylock needs lockdep annotations ")
303c07db487be ("ppp: set qdisc_tx_busylock to avoid LOCKDEP splat ")

Reported-by: Alexander Aring <alex.aring@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Tested-by: Alexander Aring <alex.aring@gmail.com>
---
 net/ieee802154/6lowpan.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
index 8bfb40153fe7..8edfea5da572 100644
--- a/net/ieee802154/6lowpan.c
+++ b/net/ieee802154/6lowpan.c
@@ -530,7 +530,27 @@ static struct header_ops lowpan_header_ops = {
 	.create	= lowpan_header_create,
 };
 
+static struct lock_class_key lowpan_tx_busylock;
+static struct lock_class_key lowpan_netdev_xmit_lock_key;
+
+static void lowpan_set_lockdep_class_one(struct net_device *dev,
+					 struct netdev_queue *txq,
+					 void *_unused)
+{
+	lockdep_set_class(&txq->_xmit_lock,
+			  &lowpan_netdev_xmit_lock_key);
+}
+
+
+static int lowpan_dev_init(struct net_device *dev)
+{
+	netdev_for_each_tx_queue(dev, lowpan_set_lockdep_class_one, NULL);
+	dev->qdisc_tx_busylock = &lowpan_tx_busylock;
+	return 0;
+}
+
 static const struct net_device_ops lowpan_netdev_ops = {
+	.ndo_init		= lowpan_dev_init,
 	.ndo_start_xmit		= lowpan_xmit,
 	.ndo_set_mac_address	= lowpan_set_address,
 };

Re: [PATCH] 6lowpan: fix lockdep splats

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 10 Feb 2014 11:42:35 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> When a device ndo_start_xmit() calls again dev_queue_xmit(),
> lockdep can complain because dev_queue_xmit() is re-entered and the
> spinlocks protecting tx queues share a common lockdep class.
> 
> Same issue was fixed for bonding/l2tp/ppp in commits 
> 
> 0daa2303028a6 ("[PATCH] bonding: lockdep annotation")
> 49ee49202b4ac ("bonding: set qdisc_tx_busylock to avoid LOCKDEP splat")
> 23d3b8bfb8eb2 ("net: qdisc busylock needs lockdep annotations ")
> 303c07db487be ("ppp: set qdisc_tx_busylock to avoid LOCKDEP splat ")
> 
> Reported-by: Alexander Aring <alex.aring@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Tested-by: Alexander Aring <alex.aring@gmail.com>

Applied, thanks Eric.