This patch aims to reduce the amount of copying in the receive path when
decompressing by checking for headroom and calling pskb_expand_head if required.
Another benefit of this is that the skb pointer passed to the decompression
routine will be the same skb after decompression. This will be a step towards
freeing the skb within the receive function and not within all the error paths
of the decompression routine.
Bluetooth and 802.15.4 have been changed to ensure that the skb isn't shared
before calling the decompression routine as this is a requirement of
pskb_expand_head.
v1 -> v2
Use const int for new headroom size.
Remove blank line.
Use skb_unshare instead of skb_copy_expand.
Use consume_skb.
v2 -> v3
Remove cases where we are trying to free a NULL skb.
v3 -> v4
Now uses skb_share_check to ensure the skb isn't shared before decompressing.
Martin Townsend (1):
6lowpan: Use pskb_expand_head in IPHC decompression.
net/6lowpan/iphc.c | 51 +++++++++++++++++++++----------------------
net/bluetooth/6lowpan.c | 7 ++++++
net/ieee802154/6lowpan_rtnl.c | 5 +++++
3 files changed, 37 insertions(+), 26 deletions(-)
--
1.9.1
Hi Martin,
code is ok by me.
Signed-off-by: Jukka Rissanen <[email protected]>
On ke, 2014-10-08 at 16:30 +0100, Martin Townsend wrote:
> Hi Alex,
>
> I'll wait for Jukka's ack for the bluetooth side and then send v5 with the removed skb_share_check.
>
> - Martin.
>
Cheers,
Jukka
Hi Alex,
I'll wait for Jukka's ack for the bluetooth side and then send v5 with the removed skb_share_check.
- Martin.
On 08/10/14 16:27, Alexander Aring wrote:
> On Wed, Oct 08, 2014 at 04:09:33PM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> I completely missed it, unless reassembly can result in a shared skb? can you confirm that it doesn't then I'll remove the line.
>>
> No, we manipulate the skb data there. We fetch the fragment header.
> See [0].
>
> This require a unshared buffer between af_802154 and 6lowpan. Also on
> LOWPAN_DISPATCH_IPV6 we fetch the one byte dispatch value.
>
> - Alex
>
> [0] http://lxr.free-electrons.com/source/net/ieee802154/reassembly.c#L328
> --
> 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
On Wed, Oct 08, 2014 at 04:09:33PM +0100, Martin Townsend wrote:
> Hi Alex,
>
> I completely missed it, unless reassembly can result in a shared skb? can you confirm that it doesn't then I'll remove the line.
>
No, we manipulate the skb data there. We fetch the fragment header.
See [0].
This require a unshared buffer between af_802154 and 6lowpan. Also on
LOWPAN_DISPATCH_IPV6 we fetch the one byte dispatch value.
- Alex
[0] http://lxr.free-electrons.com/source/net/ieee802154/reassembly.c#L328
Hi Alex,
I completely missed it, unless reassembly can result in a shared skb? can you confirm that it doesn't then I'll remove the line.
Thanks, Martin.
On 08/10/14 15:59, Alexander Aring wrote:
> Hi Martin,
>
> On Wed, Oct 08, 2014 at 02:46:55PM +0100, Martin Townsend wrote:
>> Currently there are potentially 2 skb_copy_expand calls in IPHC
>> decompression. This patch replaces this with one call to
>> pskb_expand_head. It also checks to see if there is enough headroom
>> first to ensure it's only done if necessary.
>> As pskb_expand_head must only have one reference the calling code
>> now ensures this.
>>
>> Signed-off-by: Martin Townsend <[email protected]>
> ...
>> --- a/net/ieee802154/6lowpan_rtnl.c
>> +++ b/net/ieee802154/6lowpan_rtnl.c
>> @@ -537,6 +537,11 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
>> if (ret == NET_RX_DROP)
>> goto drop;
>> } else {
>> + /* Decompression may use pskb_expand_head so no shared skb's */
>> + skb = skb_share_check(skb, GFP_ATOMIC);
>> + if (!skb)
>> + goto drop;
>> +
> We do this already at [0], is it really necessary do it here again?
>
> This ensures that 6lowpan header replacing doesn't affect the af_802154
> implementation.
>
> - Alex
>
> [0] http://lxr.free-electrons.com/source/net/ieee802154/6lowpan_rtnl.c#L466
> --
> 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 Martin,
On Wed, Oct 08, 2014 at 02:46:55PM +0100, Martin Townsend wrote:
> Currently there are potentially 2 skb_copy_expand calls in IPHC
> decompression. This patch replaces this with one call to
> pskb_expand_head. It also checks to see if there is enough headroom
> first to ensure it's only done if necessary.
> As pskb_expand_head must only have one reference the calling code
> now ensures this.
>
> Signed-off-by: Martin Townsend <[email protected]>
...
> --- a/net/ieee802154/6lowpan_rtnl.c
> +++ b/net/ieee802154/6lowpan_rtnl.c
> @@ -537,6 +537,11 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
> if (ret == NET_RX_DROP)
> goto drop;
> } else {
> + /* Decompression may use pskb_expand_head so no shared skb's */
> + skb = skb_share_check(skb, GFP_ATOMIC);
> + if (!skb)
> + goto drop;
> +
We do this already at [0], is it really necessary do it here again?
This ensures that 6lowpan header replacing doesn't affect the af_802154
implementation.
- Alex
[0] http://lxr.free-electrons.com/source/net/ieee802154/6lowpan_rtnl.c#L466
Currently there are potentially 2 skb_copy_expand calls in IPHC
decompression. This patch replaces this with one call to
pskb_expand_head. It also checks to see if there is enough headroom
first to ensure it's only done if necessary.
As pskb_expand_head must only have one reference the calling code
now ensures this.
Signed-off-by: Martin Townsend <[email protected]>
---
net/6lowpan/iphc.c | 51 +++++++++++++++++++++----------------------
net/bluetooth/6lowpan.c | 7 ++++++
net/ieee802154/6lowpan_rtnl.c | 5 +++++
3 files changed, 37 insertions(+), 26 deletions(-)
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..853b4b8 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
struct net_device *dev, skb_delivery_cb deliver_skb)
{
- struct sk_buff *new;
int stat;
- new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
- GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb_push(new, sizeof(struct ipv6hdr));
- skb_reset_network_header(new);
- skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
+ skb_push(skb, sizeof(struct ipv6hdr));
+ skb_reset_network_header(skb);
+ skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
- new->protocol = htons(ETH_P_IPV6);
- new->pkt_type = PACKET_HOST;
- new->dev = dev;
+ skb->protocol = htons(ETH_P_IPV6);
+ skb->pkt_type = PACKET_HOST;
+ skb->dev = dev;
raw_dump_table(__func__, "raw skb data dump before receiving",
- new->data, new->len);
+ skb->data, skb->len);
- stat = deliver_skb(new, dev);
+ stat = deliver_skb(skb, dev);
- kfree_skb(new);
+ consume_skb(skb);
return stat;
}
@@ -460,7 +452,7 @@ 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;
+ const int needed = sizeof(struct udphdr) + sizeof(hdr);
if (uncompress_udp_header(skb, &uh))
goto drop;
@@ -468,14 +460,13 @@ 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_tailroom(skb), GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb = new;
+ if (skb_headroom(skb) < needed) {
+ err = pskb_expand_head(skb, needed, 0, GFP_ATOMIC);
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
+ }
skb_push(skb, sizeof(struct udphdr));
skb_reset_transport_header(skb);
@@ -485,6 +476,14 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
(u8 *)&uh, sizeof(uh));
hdr.nexthdr = UIP_PROTO_UDP;
+ } else {
+ if (skb_headroom(skb) < sizeof(hdr)) {
+ err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
+ }
}
hdr.payload_len = htons(skb->len);
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index c2e0d14..6643a7c 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
kfree_skb(local_skb);
kfree_skb(skb);
} else {
+ /* Decompression may use pskb_expand_head so no shared skb's */
+ skb = skb_share_check(skb, GFP_ATOMIC);
+ if (!skb) {
+ dev->stats.rx_dropped++;
+ return NET_RX_DROP;
+ }
+
switch (skb->data[0] & 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
local_skb = skb_clone(skb, GFP_ATOMIC);
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index c7e07b8..702ad04 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -537,6 +537,11 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
if (ret == NET_RX_DROP)
goto drop;
} else {
+ /* Decompression may use pskb_expand_head so no shared skb's */
+ skb = skb_share_check(skb, GFP_ATOMIC);
+ if (!skb)
+ goto drop;
+
switch (skb->data[0] & 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
ret = process_data(skb, &hdr);
--
1.9.1