Re: RFC: mac802154 Packet Queueing and Slave Devices

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/

On 09/10/2012 02:12 AM, Eric Dumazet wrote:

On Sun, 2012-09-09 at 20:43 -0400, Alan Ott wrote:

quoted

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/

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

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

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

Signed-off-by: Alan Ott <redacted>
---
 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

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

Signed-off-by: Alan Ott <redacted>
---
 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

From: Alan Ott <redacted>
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 <redacted>

Applied.

From: Alan Ott <redacted>
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 <redacted>

Applied.

From: Alan Ott <redacted>
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 <redacted>

Applied.