Simplify lowpan receive path so skb is freed in lowpan_rcv when dropped.

Signed-off-by Martin Townsend<[email protected]>

---
  include/net/6lowpan.h         |  2 +-
  net/6lowpan/iphc.c            | 29 ++++++++++++++++-------------
  net/bluetooth/6lowpan.c       | 12 ++++++++----
  net/ieee802154/6lowpan_rtnl.c | 12 ++++++------
  4 files changed, 31 insertions(+), 24 deletions(-)

include/net/6lowpan.h +1 -1

index d7e9169..aa0381e 100644
@@ -374,7 +374,7 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
 
  typedef int (*skb_delivery_cb)(struct sk_buff *skb);
 
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+int lowpan_process_data(struct sk_buff **skb_inout, struct net_device *dev,
  		const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
  		const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
  		u8 iphc0, u8 iphc1, skb_delivery_cb skb_deliver);

net/6lowpan/iphc.c +16 -13

index 92eed6d..18dac0a 100644
@@ -179,11 +179,11 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
 
  	new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
  			      GFP_ATOMIC);
-	kfree_skb(skb);
-
  	if (!new)
  		return -ENOMEM;
 
+	kfree_skb(skb);
+
  	skb_push(new, sizeof(struct ipv6hdr));
  	skb_reset_network_header(new);
  	skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));

@@ -196,6 +196,8 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
  		       new->data, new->len);
 
  	stat = deliver_skb(new);
+	if (stat < 0)
+		stat = NET_RX_DROP;
 
  	kfree_skb(new);
 

@@ -333,7 +335,7 @@ err:
  /* TTL uncompression values */
  static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
 
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+int lowpan_process_data(struct sk_buff **skb_inout, struct net_device *dev,
  			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
  			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
  			u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)

@@ -341,6 +343,8 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
  	struct ipv6hdr hdr = {};
  	u8 tmp, num_context = 0;
  	int err;
+	int err_ret = -EINVAL;
+	struct sk_buff *skb = *skb_inout;
 
  	raw_dump_table(__func__, "raw skb data dump uncompressed",
  		       skb->data, skb->len);

@@ -461,7 +465,6 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
  	/* UDP data uncompression */
  	if (iphc0 & LOWPAN_IPHC_NH_C) {
  		struct udphdr uh;
-		struct sk_buff *new;
 
  		if (uncompress_udp_header(skb, &uh))
  			goto drop;

@@ -469,14 +472,15 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
  		/* replace the compressed UDP head by the uncompressed UDP
  		 * header
  		 */
-		new = skb_copy_expand(skb, sizeof(struct udphdr),
+		skb = skb_copy_expand(skb, sizeof(struct udphdr),
  				      skb_tailroom(skb), GFP_ATOMIC);
-		kfree_skb(skb);
-
-		if (!new)
-			return -ENOMEM;
-
-		skb = new;
+		if (!skb) {
+			err_ret = -ENOMEM;
+			goto drop;
+		}
+		
+		kfree_skb(*skb_inout);
+		*skb_inout = skb;
 
  		skb_push(skb, sizeof(struct udphdr));
  		skb_reset_transport_header(skb);

@@ -503,8 +507,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
  	return skb_deliver(skb, &hdr, dev, deliver_skb);
 
  drop:
-	kfree_skb(skb);
-	return -EINVAL;
+	return err_ret;
  }
  EXPORT_SYMBOL_GPL(lowpan_process_data);
 

net/bluetooth/6lowpan.c +8 -4

index f5df93f..38a2987 100644
@@ -215,7 +215,7 @@ static int give_skb_to_upper(struct sk_buff *skb)
  	return ret;
  }
 
-static int process_data(struct sk_buff *skb, struct net_device *netdev,
+static int process_data(struct sk_buff **skb_inout, struct net_device *netdev,
  			struct l2cap_chan *chan)
  {
  	const u8 *saddr, *daddr;

@@ -223,6 +223,7 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
  	struct lowpan_dev *dev;
  	struct lowpan_peer *peer;
  	unsigned long flags;
+	struct sk_buff *skb = *skb_inout;
 
  	dev = lowpan_dev(netdev);
 

@@ -245,13 +246,12 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
  	if (lowpan_fetch_skb_u8(skb, &iphc1))
  		goto drop;
 
-	return lowpan_process_data(skb, netdev,
+	return lowpan_process_data(skb_inout, netdev,
  				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
  				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
  				   iphc0, iphc1, give_skb_to_upper);
 
  drop:
-	kfree_skb(skb);
  	return -EINVAL;
  }
 

@@ -300,7 +300,11 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
  			if (!local_skb)
  				goto drop;
 
-			ret = process_data(local_skb, dev, chan);
+			ret = process_data(&local_skb, dev, chan);
+			if (ret < 0) {
+				kfree_skb(local_skb);
+				goto drop;
+			}
  			if (ret != NET_RX_SUCCESS)
  				goto drop;
 

net/ieee802154/6lowpan_rtnl.c +6 -6

index 3154775..3ae7646 100644
@@ -166,11 +166,12 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb)
  	return stat;
  }
 
-static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
+static int process_data(struct sk_buff **skb_inout, const struct ieee802154_hdr *hdr)
  {
  	u8 iphc0, iphc1;
  	struct ieee802154_addr_sa sa, da;
  	void *sap, *dap;
+	struct sk_buff *skb = *skb_inout;
 
  	raw_dump_table(__func__, "raw skb data dump", skb->data, skb->len);
  	/* at least two bytes will be used for the encoding */

@@ -196,13 +197,12 @@ static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
  	else
  		dap = &da.hwaddr;
 
-	return lowpan_process_data(skb, skb->dev, sap, sa.addr_type,
+	return lowpan_process_data(skb_inout, skb->dev, sap, sa.addr_type,
  				   IEEE802154_ADDR_LEN, dap, da.addr_type,
  				   IEEE802154_ADDR_LEN, iphc0, iphc1,
  				   lowpan_give_skb_to_devices);
 
  drop:
-	kfree_skb(skb);
  	return -EINVAL;
  }
 

@@ -485,14 +485,14 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
  	} else {
  		switch (skb->data[0] & 0xe0) {
  		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
-			ret = process_data(skb, &hdr);
+			ret = process_data(&skb, &hdr);
  			if (ret == NET_RX_DROP)
  				goto drop;
  			break;
  		case LOWPAN_DISPATCH_FRAG1:	/* first fragment header */
  			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1);
  			if (ret == 1) {
-				ret = process_data(skb, &hdr);
+				ret = process_data(&skb, &hdr);
  				if (ret == NET_RX_DROP)
  					goto drop;
  			}

@@ -500,7 +500,7 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
  		case LOWPAN_DISPATCH_FRAGN:	/* next fragments headers */
  			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
  			if (ret == 1) {
-				ret = process_data(skb, &hdr);
+				ret = process_data(&skb, &hdr);
  				if (ret == NET_RX_DROP)
  					goto drop;
  			}

this patch wasn't able to apply and now it doesn't apply either (because lot of other changes). I thought you will resend a v3 and then I will care about reviewing. Then putting Acked-by, then Marcel will put it on the right repositories.

If you can, describe the changes in the commit msg (please note the 80 line width there).

This patch is also something for bluetooth (not bluetooth-next), it's a bug fix. But this depends how Marcel will dealing with this issue. It's a bugfix with a huge change of lines. I would base it on bluetooth, because it's a bug fix.

Hi Alex,

Quoted 5 lines

this patch wasn't able to apply and now it doesn't apply either (because lot of other changes). I thought you will resend a v3 and then I will care about reviewing. Then putting Acked-by, then Marcel will put it on the right repositories.

Quoted 7 lines

If you can, describe the changes in the commit msg (please note the 80 line width there).

This patch is also something for bluetooth (not bluetooth-next), it's a bug fix. But this depends how Marcel will dealing with this issue. It's a bugfix with a huge change of lines. I would base it on bluetooth, because it's a bug fix.

simplifying something is not for -stable or a bug fix. If there is a memory leak, then fix that memory leak first in a separate patch. The rules are pretty clear here. Only bug fixes after the merge window has closed.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

I'll respin and include the memory leak fix and this patch and a couple of others I have and send as a series to bluetooth. What bluetooth git repository should I base the series on?

Hi Martin,

Quoted 5 lines · Martin Townsend

Hi,

I'll respin and include the memory leak fix and this patch and a couple of others I have and send as a series to bluetooth. What bluetooth git repository should I base the series on?

Okay,

net-next is open. That means the merge window has closed. Fix the memory leak in a separate patch based on bluetooth at first of your series. New features based on bluetooth-next, if these depends on each other write this after the "---" in your commit msg.

Quoted 16 lines · Marcel Holtmann

Hi Alex,

Quoted 5 lines

this patch wasn't able to apply and now it doesn't apply either (because lot of other changes). I thought you will resend a v3 and then I will care about reviewing. Then putting Acked-by, then Marcel will put it on the right repositories.

Quoted 7 lines

If you can, describe the changes in the commit msg (please note the 80 line width there).

This patch is also something for bluetooth (not bluetooth-next), it's a bug fix. But this depends how Marcel will dealing with this issue. It's a bugfix with a huge change of lines. I would base it on bluetooth, because it's a bug fix.

simplifying something is not for -stable or a bug fix. If there is a memory leak, then fix that memory leak first in a separate patch. The rules are pretty clear here. Only bug fixes after the merge window has closed.

Quoted 8 lines

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

I'll respin and include the memory leak fix and this patch and a couple of others I have and send as a series to bluetooth. What bluetooth git repository should I base the series on?

Hi Martin,

Quoted 25 lines · Martin Townsend

Quoted 16 lines · Marcel Holtmann

Hi Alex,

Quoted 5 lines

this patch wasn't able to apply and now it doesn't apply either (because lot of other changes). I thought you will resend a v3 and then I will care about reviewing. Then putting Acked-by, then Marcel will put it on the right repositories.

Quoted 7 lines

If you can, describe the changes in the commit msg (please note the 80 line width there).

This patch is also something for bluetooth (not bluetooth-next), it's a bug fix. But this depends how Marcel will dealing with this issue. It's a bugfix with a huge change of lines. I would base it on bluetooth, because it's a bug fix.

simplifying something is not for -stable or a bug fix. If there is a memory leak, then fix that memory leak first in a separate patch. The rules are pretty clear here. Only bug fixes after the merge window has closed.

Quoted 8 lines

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines

Hi,

I'll respin and include the memory leak fix and this patch and a couple of others I have and send as a series to bluetooth. What bluetooth git repository should I base the series on?

What's the state about to fix this bad issue? :-)

I didn't saw any new patches because of this.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines

Quoted 6 lines

Hi,

I'll respin and include the memory leak fix and this patch and a couple of others I have and send as a series to bluetooth. What bluetooth git repository should I base the series on?

What's the state about to fix this bad issue? :-)

Quoted 7 lines

I didn't saw any new patches because of this.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sorry for the delay, been on hols for a few days, I will create the patches tomorrow if I get time.

I have also implemented Generic Header Compression[0] which is still in draft at the moment but I can't imagine it will change much before being released. I'm going to integrate it into our linux repository this week. Would you be interested in putting it in linux-wpan or linux-wpan-next or would you prefer to wait until it gets it RFC status?

Hi Martin,

On Mon, Sep 08, 2014 at 07:13:02PM +0100, Martin Townsend wrote:
...

Quoted 16 lines

Quoted 8 lines

Quoted 6 lines

Hi,

I'll respin and include the memory leak fix and this patch and a couple of others I have and send as a series to bluetooth. What bluetooth git repository should I base the series on?

What's the state about to fix this bad issue? :-)

Quoted 7 lines

I didn't saw any new patches because of this.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines

Sorry for the delay, been on hols for a few days, I will create the patches tomorrow if I get time.

I have also implemented Generic Header Compression[0] which is still in draft at the moment but I can't imagine it will change much before being released. I'm going to integrate it into our linux repository this week. Would you be interested in putting it in linux-wpan or linux-wpan-next or would you prefer to wait until it gets it RFC status?

no, for me it's okay to have this mainline. I heard that a draft becomes no RFC when nobody implements it.

Hi Martin,

Quoted 22 lines · Alexander Aring

Hi Martin,

On Mon, Sep 08, 2014 at 07:13:02PM +0100, Martin Townsend wrote:
...

Quoted 18 lines

Quoted 8 lines

Quoted 6 lines

Hi,

I'll respin and include the memory leak fix and this patch and a couple of others I have and send as a series to bluetooth. What bluetooth git repository should I base the series on?

What's the state about to fix this bad issue? :-)

Quoted 7 lines

I didn't saw any new patches because of this.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sorry for the delay, been on hols for a few days, I will create the patches tomorrow if I get time.

Quoted 9 lines

Quoted 6 lines

I have also implemented Generic Header Compression[0] which is still in draft at the moment but I can't imagine it will change much before being released. I'm going to integrate it into our linux repository this week. Would you be interested in putting it in linux-wpan or linux-wpan-next or would you prefer to wait until it gets it RFC status?

no, for me it's okay to have this mainline. I heard that a draft becomes no RFC when nobody implements it.

I thought more about that, you mean the receiving part only? So the uncompression. The point is that we don't have no interface for an user that can decide if he like to use UDP compression like RFC 6282 or UDP compression like GHC. This is only relevant for the transmit part. So compression is optionally. (We should have some interface to make this configurable by user -> adding this to the nhc layer, later).

On the uncompression part, means the receiving part we can support both. UDP RFC 6282 or UDP like GHC, the next header id value should be different there. That means currently we can receive every packets but transmit only RFC6282 compression formats.

So for receiving this, it's okay. But for compression, since we don't have some interface to make this configurable we should use RFC 6282.

Same opinion here? We can talk about that point.

- Alex

Quoted 6 lines

I thought more about that, you mean the receiving part only? So the uncompression. The point is that we don't have no interface for an user that can decide if he like to use UDP compression like RFC 6282 or UDP compression like GHC. This is only relevant for the transmit part. So compression is optionally. (We should have some interface to make this configurable by user -> adding this to the nhc layer, later).

I've implemented compression and decompression. You are right in that we need a mechanism of configuring what gets compressed by what method.

Quoted 7 lines

On the uncompression part, means the receiving part we can support both. UDP RFC 6282 or UDP like GHC, the next header id value should be different there. That means currently we can receive every packets but transmit only RFC6282 compression formats.

So for receiving this, it's okay. But for compression, since we don't have some interface to make this configurable we should use RFC 6282.

So I will ensure UDP is compressed by 6282. Then I was going to start out by just compressing ICMPv6 with GHC and monitor how much data is saved by using GHC. Later on we will implement a mechanism of configuring what gets compressed and by which compression method.

The GHC spec states that a device indicates it's GHC capability using a 6LoWPAN Capability Indication Option (6CIO), this is an ND option. As far as I can see there is no type assigned yet by IANA so I was wondering if we should have this as an experimental configuration item in the kernel?

Hi Martin,

On Tue, Sep 09, 2014 at 10:28:36AM +0100, Martin Townsend wrote:
...

Quoted 7 lines

Quoted 6 lines

I thought more about that, you mean the receiving part only? So the uncompression. The point is that we don't have no interface for an user that can decide if he like to use UDP compression like RFC 6282 or UDP compression like GHC. This is only relevant for the transmit part. So compression is optionally. (We should have some interface to make this configurable by user -> adding this to the nhc layer, later).

I've implemented compression and decompression. You are right in that we need a mechanism of configuring what gets compressed by what method.

ok. But how we deal with that currently with GHC UDP and UDP RFC6282 compression. We can't not support both compression methods.

btw. how we should call it now? Uncompression or decompression, I can also name the callbacks to decompression. I am not a native speaker so I will ask you which is better now. :-)

Quoted 8 lines

Quoted 7 lines

On the uncompression part, means the receiving part we can support both. UDP RFC 6282 or UDP like GHC, the next header id value should be different there. That means currently we can receive every packets but transmit only RFC6282 compression formats.

So for receiving this, it's okay. But for compression, since we don't have some interface to make this configurable we should use RFC 6282.

So I will ensure UDP is compressed by 6282. Then I was going to start out by just compressing ICMPv6 with GHC and monitor how much data is saved by using GHC. Later on we will implement a mechanism of configuring what gets compressed and by which compression method.

Okay, you mean that you will leave UDP compression by 6282 but insert a receive handling (decompression) for UDP GHC?

RFC6282 doesn't describe any compression/decompression(or uncompression) format for ICMPv6, so we could handle there compression and uncompression. I understand now you did it that way, or?

About the mechanism by user:

There are several ways about to do it from userspace. I know now sysfs or netlink. Do you have already some idea how you want to make this configurable by user?

btw.
This reminds me a little bit like setting led trigger type, (blink, heartbeat, mmc, net, etc...). This is done by sysfs.

Quoted 1 line

Quoted 1 line

The GHC spec states that a device indicates it's GHC capability using a 6LoWPAN Capability Indication Option (6CIO), this is an ND option. As far as I can see there is no type assigned yet by IANA so I was wondering if we should have this as an experimental configuration item in the kernel?

Yes, please make a bool into net/6lowpan/Kconfig and add support for drafts only if selected.

In code you simple need to use "if (IS_ENABLED(CONFIG_FOO))" to registration the nhc format into the nhc framework/layer or not.

Hi Martin,

On Tue, Sep 09, 2014 at 10:28:36AM +0100, Martin Townsend wrote:
...

Quoted 6 lines

Quoted 6 lines

I thought more about that, you mean the receiving part only? So the uncompression. The point is that we don't have no interface for an user that can decide if he like to use UDP compression like RFC 6282 or UDP compression like GHC. This is only relevant for the transmit part. So compression is optionally. (We should have some interface to make this configurable by user -> adding this to the nhc layer, later).

Quoted 1 line

I've implemented compression and decompression. You are right in that we need a mechanism of configuring what gets compressed by what method.

ok. But how we deal with that currently with GHC UDP and UDP RFC6282 compression. We can't not support both compression methods.

btw. how we should call it now? Uncompression or decompression, I can also name the callbacks to decompression. I am not a native speaker so I will ask you which is better now. :-)

Quoted 8 lines

Quoted 8 lines

On the uncompression part, means the receiving part we can support both. UDP RFC 6282 or UDP like GHC, the next header id value should be different there. That means currently we can receive every packets but transmit only RFC6282 compression formats.

So for receiving this, it's okay. But for compression, since we don't have some interface to make this configurable we should use RFC 6282.

Quoted 1 line

So I will ensure UDP is compressed by 6282. Then I was going to start out by just compressing ICMPv6 with GHC and monitor how much data is saved by using GHC. Later on we will implement a mechanism of configuring what gets compressed and by which compression method.

Okay, you mean that you will leave UDP compression by 6282 but insert a receive handling (decompression) for UDP GHC?

RFC6282 doesn't describe any compression/decompression(or uncompression) format for ICMPv6, so we could handle there compression and uncompression. I understand now you did it that way, or?

About the mechanism by user:

There are several ways about to do it from userspace. I know now sysfs or netlink. Do you have already some idea how you want to make this configurable by user?

btw.
This reminds me a little bit like setting led trigger type, (blink, heartbeat, mmc, net, etc...). This is done by sysfs.

Quoted 1 line

The GHC spec states that a device indicates it's GHC capability using a 6LoWPAN Capability Indication Option (6CIO), this is an ND option. As far as I can see there is no type assigned yet by IANA so I was wondering if we should have this as an experimental configuration item in the kernel?

Yes, please make a bool into net/6lowpan/Kconfig and add support for drafts only if selected.

In code you simple need to use "if (IS_ENABLED(CONFIG_FOO))" to registration the nhc format into the nhc framework/layer or not.

Replace FOO with a propber 6LOWPAN_NHC_DRAFTS or something else. You can write in the help what exactly this means.

- Alex

Hi Alex,

Quoted 10 lines · Alexander Aring

Hi Martin,

On Tue, Sep 09, 2014 at 10:28:36AM +0100, Martin Townsend wrote:
...

Quoted 6 lines

Quoted 6 lines

I thought more about that, you mean the receiving part only? So the uncompression. The point is that we don't have no interface for an user that can decide if he like to use UDP compression like RFC 6282 or UDP compression like GHC. This is only relevant for the transmit part. So compression is optionally. (We should have some interface to make this configurable by user -> adding this to the nhc layer, later).

Quoted 7 lines

Quoted 1 line

I've implemented compression and decompression. You are right in that we need a mechanism of configuring what gets compressed by what method.

ok. But how we deal with that currently with GHC UDP and UDP RFC6282 compression. We can't not support both compression methods.

btw. how we should call it now? Uncompression or decompression, I can also name the callbacks to decompression. I am not a native speaker so I will ask you which is better now. :-)

As an English speaker I have to admit I don't know. Here's one link I found on the subject
http://english.stackexchange.com/questions/56480/difference-between-uncompress-and-decompress
to confuse you even more :)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

On the uncompression part, means the receiving part we can support both. UDP RFC 6282 or UDP like GHC, the next header id value should be different there. That means currently we can receive every packets but transmit only RFC6282 compression formats.

So for receiving this, it's okay. But for compression, since we don't have some interface to make this configurable we should use RFC 6282.

Quoted 7 lines

Quoted 1 line

So I will ensure UDP is compressed by 6282. Then I was going to start out by just compressing ICMPv6 with GHC and monitor how much data is saved by using GHC. Later on we will implement a mechanism of configuring what gets compressed and by which compression method.

Okay, you mean that you will leave UDP compression by 6282 but insert a receive handling (decompression) for UDP GHC?

RFC6282 doesn't describe any compression/decompression(or uncompression) format for ICMPv6, so we could handle there compression and uncompression. I understand now you did it that way, or?

For the moment I will assume all ICMPv6 traffic is compressed and decompressed with GHC as this will be the only Next Header Compression format. In future we need something better. We also need a method of knowing what compression formats a device supports. I can see a list of compression formats which could also be a list by protocol. Then when sending to a device you would select the highest ranking supported compression format for that device.

Hi Martin,

Quoted 31 lines · Martin Townsend

Hi Alex,

Quoted 10 lines · Alexander Aring

Hi Martin,

On Tue, Sep 09, 2014 at 10:28:36AM +0100, Martin Townsend wrote:
...

Quoted 6 lines

Quoted 6 lines

I thought more about that, you mean the receiving part only? So the uncompression. The point is that we don't have no interface for an user that can decide if he like to use UDP compression like RFC 6282 or UDP compression like GHC. This is only relevant for the transmit part. So compression is optionally. (We should have some interface to make this configurable by user -> adding this to the nhc layer, later).

Quoted 7 lines

Quoted 1 line

I've implemented compression and decompression. You are right in that we need a mechanism of configuring what gets compressed by what method.

ok. But how we deal with that currently with GHC UDP and UDP RFC6282 compression. We can't not support both compression methods.

btw. how we should call it now? Uncompression or decompression, I can also name the callbacks to decompression. I am not a native speaker so I will ask you which is better now. :-)

As an English speaker I have to admit I don't know. Here's one link I found on the subject
http://english.stackexchange.com/questions/56480/difference-between-uncompress-and-decompress
to confuse you even more :)

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

On the uncompression part, means the receiving part we can support both. UDP RFC 6282 or UDP like GHC, the next header id value should be different there. That means currently we can receive every packets but transmit only RFC6282 compression formats.

So for receiving this, it's okay. But for compression, since we don't have some interface to make this configurable we should use RFC 6282.

Quoted 8 lines

Quoted 7 lines

Quoted 1 line

So I will ensure UDP is compressed by 6282. Then I was going to start out by just compressing ICMPv6 with GHC and monitor how much data is saved by using GHC. Later on we will implement a mechanism of configuring what gets compressed and by which compression method.

Okay, you mean that you will leave UDP compression by 6282 but insert a receive handling (decompression) for UDP GHC?

RFC6282 doesn't describe any compression/decompression(or uncompression) format for ICMPv6, so we could handle there compression and uncompression. I understand now you did it that way, or?

For the moment I will assume all ICMPv6 traffic is compressed and decompressed with GHC as this will be the only Next Header Compression format. In future we need something better. We also need a method of knowing what compression formats a device supports. I can see a list of compression formats which could also be a list by protocol. Then when sending to a device you would select the highest ranking supported compression format for that device.

Is "what compression methods like a device to use" part of any RFC? Is there something which I don't know? I mean, okay you can do that in any application layer in userspace. But I don't see that we need something like this in kernelspace. I know there is no suggestion that you want to implement something like this in kernelspace, but I want to clarify this.

Application layer in userspace means, use some own coap(or whatever) based protocol and setup the right compressions via userspace by some application.

- Alex

Quoted 2 lines

The GHC spec states that a device indicates it's GHC capability using a 6LoWPAN Capability Indication Option (6CIO), this is an ND option. As far as I can see there is no type assigned yet by IANA so I was wondering if we should have this as an experimental configuration item in the kernel?

Yes, please make a bool into net/6lowpan/Kconfig and add support for drafts only if selected.

In code you simple need to use "if (IS_ENABLED(CONFIG_FOO))" to registration the nhc format into the nhc framework/layer or not.

Replace FOO with a propber 6LOWPAN_NHC_DRAFTS or something else. You can write in the help what exactly this means.

Hi Alex,

Quoted 6 lines

Quoted 2 lines

The GHC spec states that a device indicates it's GHC capability using a 6LoWPAN Capability Indication Option (6CIO), this is an ND option. As far as I can see there is no type assigned yet by IANA so I was wondering if we should have this as an experimental configuration item in the kernel?

Yes, please make a bool into net/6lowpan/Kconfig and add support for drafts only if selected.

Quoted 5 lines

In code you simple need to use "if (IS_ENABLED(CONFIG_FOO))" to registration the nhc format into the nhc framework/layer or not.

Replace FOO with a propber 6LOWPAN_NHC_DRAFTS or something else. You can write in the help what exactly this means.

or you just create /sys/kernel/debug/6lowpan/foo and use debugfs to toggle experimental options on/off at runtime. Especially if you are dealing with not yet assigned types, you can also have an entry that allows you to define the type.

Hi Martin,

On Mon, Sep 08, 2014 at 07:13:02PM +0100, Martin Townsend wrote:
...

Quoted 15 lines

Quoted 8 lines

Quoted 6 lines

Hi,

I'll respin and include the memory leak fix and this patch and a couple of others I have and send as a series to bluetooth. What bluetooth git repository should I base the series on?

What's the state about to fix this bad issue? :-)

Quoted 7 lines

I didn't saw any new patches because of this.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines

Sorry for the delay, been on hols for a few days, I will create the patches tomorrow if I get time.

I have also implemented Generic Header Compression[0] which is still in draft at the moment but I can't imagine it will change much before being released. I'm going to integrate it into our linux repository this week. Would you be interested in putting it in linux-wpan or linux-wpan-next or would you prefer to wait until it gets it RFC status?

no, for me it's okay to have this mainline. I heard that a draft becomes no RFC when nobody implements it.

But another point, I want to avoid that everything is putted into the iphc file for next header compression. Did you saw the patches for the next header compression layer?

This is a layer to separate next header implementation, like IPv6 it does for next header [0]. [0] shows the registration of the next header protocols.

We should have something similar and I post some months a RFC about that. I got some review notes and need to rework it, working branch is at [1].

Also I added that we drop packets for IPv6 Header Extensions, described at [2]. We currently no support this kind of next header compression and if we don't check on it, we send garbage to IPv6 layer. (That's of course one more argument to implement draft next header compression formats).

For now, we should make it like this:

- First you send patches for the fixes.
- Then I will send patches for the nhc layer.
- You send patches to adding the your next header compression formats. based on the nhc layer.

Is that okay for you? It should easily for you to add your header compression formats. (Filling some struct with macro and implement some callbacks).

This is all related stuff to the GENERIC 6LOWPAN branch, all which belongs to IPHC and IPHC contains next header compression. As conclusion this should be go to bluetooth/bluetooth-next.

Bug fixes to bluetooth, new features to bluetooth-next.

btw:
I working on the 802.15.4 rework. I am sure you know that I want to make a rework of the 802.15.4 stack. Very busy at the moment, but this should not affect the net/6lowpan branch. Only the net/ieee802.15.4/6lowpan.

I will take some time, so after you send fixes for this I will rebase/rework the nhc layer patches to try to bring it mainline. Then you can make your work on it.

- Alex

[0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/net/ipv6/af_inet6.c?id=refs/tags/v3.17-rc4#n822
[1] https://github.com/linux-wpan/linux-wpan-next/tree/nhc_layer
[2] http://tools.ietf.org/html/rfc6282#section-4.2
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html