* [PATCH v3 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression. @ 2014-10-08 7:35 Martin Townsend 2014-10-08 7:35 ` Martin Townsend 0 siblings, 1 reply; 6+ messages in thread From: Martin Townsend @ 2014-10-08 7:35 UTC (permalink / raw) To: linux-bluetooth, linux-wpan Cc: marcel, alex.aring, jukka.rissanen, Martin Townsend 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 only one reference to the skb exists 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. Martin Townsend (1): 6lowpan: Use pskb_expand_head in IPHC decompression. net/6lowpan/iphc.c | 51 +++++++++++++++++++++---------------------- net/bluetooth/6lowpan.c | 12 ++++++---- net/ieee802154/6lowpan_rtnl.c | 5 +++++ 3 files changed, 38 insertions(+), 30 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression. 2014-10-08 7:35 [PATCH v3 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression Martin Townsend @ 2014-10-08 7:35 ` Martin Townsend 2014-10-08 7:55 ` Alexander Aring 2014-10-08 8:10 ` Alexander Aring 0 siblings, 2 replies; 6+ messages in thread From: Martin Townsend @ 2014-10-08 7:35 UTC (permalink / raw) To: linux-bluetooth, linux-wpan Cc: marcel, alex.aring, jukka.rissanen, Martin Townsend 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 <martin.townsend@xsilon.com> --- net/6lowpan/iphc.c | 51 +++++++++++++++++++++---------------------- net/bluetooth/6lowpan.c | 12 ++++++---- net/ieee802154/6lowpan_rtnl.c | 5 +++++ 3 files changed, 38 insertions(+), 30 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..bd85edf 100644 --- a/net/bluetooth/6lowpan.c +++ b/net/bluetooth/6lowpan.c @@ -343,13 +343,17 @@ 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, ensure skb unique */ + skb = skb_unshare(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); - if (!local_skb) - goto drop; - ret = process_data(local_skb, dev, chan); + ret = process_data(skb, dev, chan); if (ret != NET_RX_SUCCESS) goto drop; diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c index c7e07b8..68287db 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, ensure skb unique */ + skb = skb_unshare(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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression. 2014-10-08 7:35 ` Martin Townsend @ 2014-10-08 7:55 ` Alexander Aring 2014-10-08 8:06 ` Alexander Aring 2014-10-08 8:10 ` Alexander Aring 1 sibling, 1 reply; 6+ messages in thread From: Alexander Aring @ 2014-10-08 7:55 UTC (permalink / raw) To: Martin Townsend Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen, Martin Townsend Hi Martin, On Wed, Oct 08, 2014 at 08:35:45AM +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 <martin.townsend@xsilon.com> > --- > net/6lowpan/iphc.c | 51 +++++++++++++++++++++---------------------- > net/bluetooth/6lowpan.c | 12 ++++++---- > net/ieee802154/6lowpan_rtnl.c | 5 +++++ > 3 files changed, 38 insertions(+), 30 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)); > Just a hint, don't do that in this patch or send a patch series. I would in a future move: skb_push(skb, sizeof(struct ipv6hdr)); skb_reset_network_header(skb); skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr)); into the end of iphc decompression, because we do the same thing for transport header in iphc decompression. > - 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; This we should leave here. Then bluetooth/802.15.4 can set different things here. ... > hdr.payload_len = htons(skb->len); > diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c > index c2e0d14..bd85edf 100644 > --- a/net/bluetooth/6lowpan.c > +++ b/net/bluetooth/6lowpan.c > @@ -343,13 +343,17 @@ 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, ensure skb unique */ > + skb = skb_unshare(skb, GFP_ATOMIC); > + if (!skb) { > + dev->stats.rx_dropped++; > + return NET_RX_DROP; > + } > + What I meant here in my previous review notes is that kfree_skb(skb); was correct but in this case we can't do it, we need a temp variable. skb = skb_unshare(skb, GFP_ATOMIC); here we lost the skb reference from parameter if failed and I don't see that skb_unshare free it on failing (maybe I am wrong here). foo(any name for a tmp skb name, I see a lot of nskb names for a tmp skb name. Maybe this is a usual name convention) = skb_unshare(skb, GFP_ATOMIC); Then we don't lost the "skb" parameter reference and can free it with kfree_skb(skb). If all is fine we can run a skb = foo. I think that skb_unshare and the buffer is NOT shared then (skb == foo), but I am not sure about this. If it's NOT the same then we have some memory leaking here. - Alex ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression. 2014-10-08 7:55 ` Alexander Aring @ 2014-10-08 8:06 ` Alexander Aring 2014-10-08 8:10 ` Martin Townsend 0 siblings, 1 reply; 6+ messages in thread From: Alexander Aring @ 2014-10-08 8:06 UTC (permalink / raw) To: Martin Townsend Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen, Martin Townsend Hi Martin, On Wed, Oct 08, 2014 at 09:54:58AM +0200, Alexander Aring wrote: ... > > here we lost the skb reference from parameter if failed and I don't see > that skb_unshare free it on failing (maybe I am wrong here). > I will do this step by step. 1. call skb_unshare 2. check if cloned 3. call skb_copy 4. call __alloc_skb 5. __alloc_skb return NULL 6. assign to temp nskb; 7. parameter skb will be freed. kfree_skb(skb). 8. skb = nskb; 9. return skb; So I see that kfree_skb(skb) is always called also on failure. So your code seems to be correct. This is a lack of documentation. - Alex ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression. 2014-10-08 8:06 ` Alexander Aring @ 2014-10-08 8:10 ` Martin Townsend 0 siblings, 0 replies; 6+ messages in thread From: Martin Townsend @ 2014-10-08 8:10 UTC (permalink / raw) To: Alexander Aring, Martin Townsend Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen Hi Alex, On 08/10/14 09:06, Alexander Aring wrote: > Hi Martin, > > On Wed, Oct 08, 2014 at 09:54:58AM +0200, Alexander Aring wrote: > ... >> here we lost the skb reference from parameter if failed and I don't see >> that skb_unshare free it on failing (maybe I am wrong here). >> > I will do this step by step. > > 1. call skb_unshare > > 2. check if cloned > > 3. call skb_copy > > 4. call __alloc_skb > > 5. __alloc_skb return NULL > > 6. assign to temp nskb; > > 7. parameter skb will be freed. kfree_skb(skb). > > 8. skb = nskb; > > 9. return skb; > > So I see that kfree_skb(skb) is always called also on failure. Yes this caught me out too :) > So your code seems to be correct. This is a lack of documentation. > > - Alex So is v3 patch good to go? - Martin. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression. 2014-10-08 7:35 ` Martin Townsend 2014-10-08 7:55 ` Alexander Aring @ 2014-10-08 8:10 ` Alexander Aring 1 sibling, 0 replies; 6+ messages in thread From: Alexander Aring @ 2014-10-08 8:10 UTC (permalink / raw) To: Martin Townsend Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen, Martin Townsend On Wed, Oct 08, 2014 at 08:35:45AM +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 <martin.townsend@xsilon.com> Acked-by: Alexander Aring <alex.aring@gmail.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-08 8:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-08 7:35 [PATCH v3 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression Martin Townsend 2014-10-08 7:35 ` Martin Townsend 2014-10-08 7:55 ` Alexander Aring 2014-10-08 8:06 ` Alexander Aring 2014-10-08 8:10 ` Martin Townsend 2014-10-08 8:10 ` Alexander Aring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).