RFC: mac802154 Packet Queueing and Slave Devices

RFC: mac802154 Packet Queueing and Slave Devices

[Alan Ott]

Hi,

Tony and I were recently talking about packet queueing on 802.15.4. What
currently happens (in net/mac802154/tx.c) is that each tx packet (skb)
is stuck on a work queue, and the worker function then sends each packet
to the hardware driver in order.

The problem with this is that it defeats the netif flow control. The
networking layer thinks the packet is sent as soon as it's put on the
workqueue (because the function that queues it returns NETDEV_TX_OK to
the networking layer), and the workqueue can then get arbitrarily large
if an application tries to send a lot of data. (Tony has shown this with
iperf)

The way the 802.15.4 drivers are currently written, their xmit function
blocks until the hardware confirms the packet has been sent. Any
hardware queueing is either not done (at86rf230 and mrf24j40 (and other
non-mainline (yet) drivers)[1]), or is done completely in the firmware
side (as in serial.c (for Econotag), not in mainline yet).

Solution 1:
If we want to keep the driver interface this way (no queueing on the
driver side and each driver's .xmit() function blocks), then we should
call netif_stop_queue()/netif_wake_queue() on the mac802154-subsystem
side[2].

Solution 2:
If we instead want to move to a non-blocking .xmit() function, like
ethernet and wifi currently have, we should then push the
netif_*_queue() functions to the drivers. This has the added benefit of
increased efficiency for devices which have a hardware queue (like the
Econotag, which is managed by the serial.c driver), as netif_*_queue()
functions won't have to be turned on and off repeatedly.

Solution 2 is more invasive. Note that right now we can't add
netif_*_queue() functions to the drivers, because the drivers have no
way to get to the net_device pointer. That is a different, but related
problem, which we might as well get to now. Right now there is the idea
of hardware devices each having multiple virtual slave devices
(represented by mac802154_sub_if_data, net/mac802154/mac802154.h). These
slave devices each have a net_device pointer. The drivers only get a
pointer to the ieee802154_dev, which represents the physical hardware.
They get no net_device (and there's no way for them to get to a
net_device pointer because there are multiple net_devices (one for each
slave interface) which could be sending them data). One of the problems
here is that each of these slave interfaces can potentially (and by
design) be on a different channel, which seems to cause a major problem
since the hardware radio can only receive on a single channel at a time.

I propose implementation of solution #1 in the short term, in parallel
with discussion about the intent of slave devices what their intended
design goals were and how they can be made to work as designed (if
possible).

Alan.

[1] While I can't speak for all devices, the mrf24j40 has no hardware
queue (or, it has a single packet queue).
[2] netif_stop_queue() would go in mac802154_tx() and netif_wake_queue()
would go in mac802154_xmit_worker() once xmit() returns.

Re: RFC: mac802154 Packet Queueing and Slave Devices

[Eric Dumazet]

On Sun, 2012-09-09 at 20:43 -0400, Alan Ott wrote:
> Hi,
> 
> Tony and I were recently talking about packet queueing on 802.15.4. What
> currently happens (in net/mac802154/tx.c) is that each tx packet (skb)
> is stuck on a work queue, and the worker function then sends each packet
> to the hardware driver in order.
> 
> The problem with this is that it defeats the netif flow control.

And qdisc ability to better control bufferbloat...

By the way, mac802154_tx() looks buggy :

if (!(priv->phy->channels_supported[page] & (1 << chan))) {
	WARN_ON(1);
	// Here, a kfree_skb(skb) is missing.
	return NETDEV_TX_OK;
}
if (skb_cow_head(skb, priv->hw.extra_tx_headroom)) {
	dev_kfree_skb(skb);  // should be kfree_skb(skb)
	return NETDEV_TX_OK;
}
work = kzalloc(sizeof(struct xmit_work), GFP_ATOMIC);
if (!work)
	return NETDEV_TX_BUSY;

NETDEV_TX_BUSY is going to loop. So if there is really no more memory,
its a deadlock. You should instead kfree_skb(skb) and return
NETDEV_TX_OK.

Also mac802154_wpan_xmit() returns NETDEV_TX_OK without kfree_skb(skb)
here :

if (chan == MAC802154_CHAN_NONE ||
    page >= WPAN_NUM_PAGES ||
    chan >= WPAN_NUM_CHANNELS)
	return NETDEV_TX_OK;




------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: RFC: mac802154 Packet Queueing and Slave Devices

[Alan Ott]

On 09/10/2012 02:12 AM, Eric Dumazet wrote:
> On Sun, 2012-09-09 at 20:43 -0400, Alan Ott wrote:
>> Tony and I were recently talking about packet queueing on 802.15.4.
>>
>> The problem with this is that it defeats the netif flow control.
> And qdisc ability to better control bufferbloat...
>
> By the way, mac802154_tx() looks buggy :
>
> Also mac802154_wpan_xmit() returns NETDEV_TX_OK without kfree_skb(skb)
>

Hi Eric,

Thanks for the review. I'll get this patched up.

Alan.


------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

[PATCH] 6lowpan: consider checksum bytes in fragmentation threshold

[Alan Ott]

Change the threshold for framentation of a lowpan packet from
using the MTU size to now use the MTU size minus the checksum length,
which is added by the hardware. For IEEE 802.15.4, this effectively
changes it from 127 bytes to 125 bytes.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 net/ieee802154/6lowpan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
index 6d42c17..f651da6 100644
--- a/net/ieee802154/6lowpan.c
+++ b/net/ieee802154/6lowpan.c
@@ -1047,7 +1047,8 @@ static netdev_tx_t lowpan_xmit(struct sk_buff *skb, struct net_device *dev)
 		goto error;
 	}
 
-	if (skb->len <= IEEE802154_MTU) {
+	/* Send directly if less than the MTU minus the 2 checksum bytes. */
+	if (skb->len <= IEEE802154_MTU - IEEE802154_MFR_SIZE) {
 		err = dev_queue_xmit(skb);
 		goto out;
 	}
-- 
1.7.11.2

Re: [PATCH] 6lowpan: consider checksum bytes in fragmentation threshold

[Alan Ott]

On 11/29/2012 08:55 PM, Alan Ott wrote:
> Change the threshold for framentation of a lowpan packet from
> using the MTU size to now use the MTU size minus the checksum length,
> which is added by the hardware. For IEEE 802.15.4, this effectively
> changes it from 127 bytes to 125 bytes.
>

Sorry, this was put in the wrong thread. One day I'll get one of these
first-try. :(

[PATCH 1/2] mac802154: fix memory leaks

[Alan Ott]

kfree_skb() was not getting called in the case of some failures.
This was pointed out by Eric Dumazet.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 net/mac802154/tx.c   | 5 ++++-
 net/mac802154/wpan.c | 4 +++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index 1a4df39..db63914 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -85,6 +85,7 @@ netdev_tx_t mac802154_tx(struct mac802154_priv *priv, struct sk_buff *skb,
 
 	if (!(priv->phy->channels_supported[page] & (1 << chan))) {
 		WARN_ON(1);
+		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}
 
@@ -103,8 +104,10 @@ netdev_tx_t mac802154_tx(struct mac802154_priv *priv, struct sk_buff *skb,
 	}
 
 	work = kzalloc(sizeof(struct xmit_work), GFP_ATOMIC);
-	if (!work)
+	if (!work) {
+		kfree_skb(skb);
 		return NETDEV_TX_BUSY;
+	}
 
 	INIT_WORK(&work->work, mac802154_xmit_worker);
 	work->skb = skb;
diff --git a/net/mac802154/wpan.c b/net/mac802154/wpan.c
index f30f6d4..1191039 100644
--- a/net/mac802154/wpan.c
+++ b/net/mac802154/wpan.c
@@ -327,8 +327,10 @@ mac802154_wpan_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	if (chan == MAC802154_CHAN_NONE ||
 	    page >= WPAN_NUM_PAGES ||
-	    chan >= WPAN_NUM_CHANNELS)
+	    chan >= WPAN_NUM_CHANNELS) {
+		kfree_skb(skb);
 		return NETDEV_TX_OK;
+	}
 
 	skb->skb_iif = dev->ifindex;
 	dev->stats.tx_packets++;
-- 
1.7.11.2

[PATCH 2/2] mac802154: use kfree_skb() instead of dev_kfree_skb()

[Alan Ott]

kfree_skb() indicates failure, which is where this is being used.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 net/mac802154/tx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index db63914..4e09d07 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -99,7 +99,7 @@ netdev_tx_t mac802154_tx(struct mac802154_priv *priv, struct sk_buff *skb,
 	}
 
 	if (skb_cow_head(skb, priv->hw.extra_tx_headroom)) {
-		dev_kfree_skb(skb);
+		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}
 
-- 
1.7.11.2

Re: [PATCH] 6lowpan: consider checksum bytes in fragmentation threshold

[David Miller]

From: Alan Ott <alan@signal11.us>
Date: Thu, 29 Nov 2012 20:55:44 -0500

> Change the threshold for framentation of a lowpan packet from
> using the MTU size to now use the MTU size minus the checksum length,
> which is added by the hardware. For IEEE 802.15.4, this effectively
> changes it from 127 bytes to 125 bytes.
> 
> Signed-off-by: Alan Ott <alan@signal11.us>

Applied.

Re: [PATCH 1/2] mac802154: fix memory leaks

[David Miller]

From: Alan Ott <alan@signal11.us>
Date: Thu, 29 Nov 2012 23:25:10 -0500

> kfree_skb() was not getting called in the case of some failures.
> This was pointed out by Eric Dumazet.
> 
> Signed-off-by: Alan Ott <alan@signal11.us>

Applied.

Re: [PATCH 2/2] mac802154: use kfree_skb() instead of dev_kfree_skb()

[David Miller]

From: Alan Ott <alan@signal11.us>
Date: Thu, 29 Nov 2012 23:25:11 -0500

> kfree_skb() indicates failure, which is where this is being used.
> 
> Signed-off-by: Alan Ott <alan@signal11.us>

Applied.

Re: RFC: mac802154 Packet Queueing and Slave Devices

[Alan Ott]

On 09/09/2012 08:43 PM, Alan Ott wrote:
> Tony and I were recently talking about packet queueing on 802.15.4. What
> currently happens (in net/mac802154/tx.c) is that each tx packet (skb)
> is stuck on a work queue, and the worker function then sends each packet
> to the hardware driver in order.
> 
> The problem with this is that it defeats the netif flow control. The
> networking layer thinks the packet is sent as soon as it's put on the
> workqueue (because the function that queues it returns NETDEV_TX_OK to
> the networking layer), and the workqueue can then get arbitrarily large
> if an application tries to send a lot of data. (Tony has shown this with
> iperf)
> 

So I tried fixing this using netif_stop_queue() and netif_wake_queue(),
the standard way. The flow control works, but I'm now losing packets.

It happens like this:

ipv6           -> 6lowpan   -> net core -> mac802154         -> hardware
 single packet     fragment                 netif_stop_queue()
                   fragment
                   fragment
                   fragment

  Above: a single ipv6 packet is split into fragments by 6lowpan. Each
  fragment is sent through the networking core where it ends up in
  mac802154, which will call netif_stop_queue() and netif_wake_queue()
  for flow control as packets are sent.


The problem is that since many ieee802154 hardware devices can only hold
one packet at a time in their tx buffer, netif_stop_queue() gets called
after the first fragment. Since the 6lowpan code is trying to, in the
above case, send 4 packets, the remaining 3 will get dropped when
they're handed to the networking core (dev_queue_xmit()) when the queue
is stopped.

So as a solution, one could envision 6lowpan putting the fragments into
a queue, and submitting one at a time, as the queue gets woken. The
problem with this is that there's no way to get notification for when a
queue is woken. I checked both ppp and ax25 (which would seem to have
this same issue), and they both have a fragment queue, but they rely on
external events (mostly unrelated to the queue being woken) to trigger
sending packets from the queue (see calls to ax25_kick()). That seems
hacky at best.

A thread from pppoe[1] talks about what's a similar issue. The patch
from that email was never merged. Even so, their solution seems a bit
hacky too (because it would basically cause a kick to (in this case)
6lowpan, whenever an skb gets destroyed (ie: after it's sent). With the
desire for 6lowpan to be a generic protocol[2], one would want it to be
efficient on MAC layers which do support longer queues[3].

What am I missing here? What's the right way to do this?

Alan.

[1] http://thread.gmane.org/gmane.linux.network/233089
[2] There has been some discussion about using 6lowpan on Bluetooth
low-energy.
[3] There's also the case where 2 6lowpan instances are on attached to
the same hardware, or where 6lowpan and raw are being used concurrently.

Re: RFC: mac802154 Packet Queueing and Slave Devices

[Alan Ott]

On 03/21/2013 12:09 PM, Alan Ott wrote:
> On 09/09/2012 08:43 PM, Alan Ott wrote:
>> Tony and I were recently talking about packet queueing on 802.15.4. What
>> currently happens (in net/mac802154/tx.c) is that each tx packet (skb)
>> is stuck on a work queue, and the worker function then sends each packet
>> to the hardware driver in order.
>>
>> The problem with this is that it defeats the netif flow control. The
>> networking layer thinks the packet is sent as soon as it's put on the
>> workqueue (because the function that queues it returns NETDEV_TX_OK to
>> the networking layer), and the workqueue can then get arbitrarily large
>> if an application tries to send a lot of data. (Tony has shown this with
>> iperf)
>>
> So I tried fixing this using netif_stop_queue() and netif_wake_queue(),
> the standard way. The flow control works, but I'm now losing packets.
>
> It happens like this:
>
> ipv6           -> 6lowpan   -> net core -> mac802154         -> hardware
>  single packet     fragment                 netif_stop_queue()
>                    fragment
>                    fragment
>                    fragment
>
>   Above: a single ipv6 packet is split into fragments by 6lowpan. Each
>   fragment is sent through the networking core where it ends up in
>   mac802154, which will call netif_stop_queue() and netif_wake_queue()
>   for flow control as packets are sent.
>
>
> The problem is that since many ieee802154 hardware devices can only hold
> one packet at a time in their tx buffer, netif_stop_queue() gets called
> after the first fragment. Since the 6lowpan code is trying to, in the
> above case, send 4 packets, the remaining 3 will get dropped when
> they're handed to the networking core (dev_queue_xmit()) when the queue
> is stopped.
>
> So as a solution, one could envision 6lowpan putting the fragments into
> a queue, and submitting one at a time, as the queue gets woken. The
> problem with this is that there's no way to get notification for when a
> queue is woken. I checked both ppp and ax25 (which would seem to have
> this same issue), and they both have a fragment queue, but they rely on
> external events (mostly unrelated to the queue being woken) to trigger
> sending packets from the queue (see calls to ax25_kick()). That seems
> hacky at best.
>
> A thread from pppoe[1] talks about what's a similar issue. The patch
> from that email was never merged. Even so, their solution seems a bit
> hacky too (because it would basically cause a kick to (in this case)
> 6lowpan, whenever an skb gets destroyed (ie: after it's sent). With the
> desire for 6lowpan to be a generic protocol[2], one would want it to be
> efficient on MAC layers which do support longer queues[3].
>
> What am I missing here? What's the right way to do this?
>
> Alan.
>
> [1] http://thread.gmane.org/gmane.linux.network/233089
> [2] There has been some discussion about using 6lowpan on Bluetooth
> low-energy.
> [3] There's also the case where 2 6lowpan instances are on attached to
> the same hardware, or where 6lowpan and raw are being used concurrently.

I guess the more condensed question is, as a protocol which generates
fragments, what's the right way to handle queue management from the device?

Alan.