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
v4 -> v5
Remove skb_share_check from 802.15.4 lowpan_rcv as it's already done at the
top of the function.
Martin Townsend (1):
6lowpan: Use pskb_expand_head in IPHC decompression.
net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
net/bluetooth/6lowpan.c | 7 +++++++
2 files changed, 32 insertions(+), 26 deletions(-)
--
1.9.1
On Mon, Oct 13, 2014 at 10:28:44AM +0100, Martin Townsend wrote:
...
> >> If it's ok with everyone I'll change pskb_expand_head to skb_cow or skb_cow_head and leave the bluetooth code as it is with this v5 patch.
> > For me it's confusing, you do that on skb, but we never used skb
> > afterwards (maybe on freeing) but we use local_skb, the local_skb should
> > always be cloned because we running skb_clone before.
> skb_clone does ensure that the skb isn't shared anymore so I think it will be safe to remove the skb_share_check. Or I can move it to the start of the function like we do for 802.15.4?
We need to clarify the following points:
skb_is_shared:
It's used twice in some packet handler functions or elsewhere.
now a skb_clone:
A skb clone is only a copy of struct sk_buff, so we can safely run
skb_pull and skb_push, skb->dev = foo. Simple for modify skb attributes.
now a skb_copy:
It's a complete private copy with private data buffer which allow us to
make skb->data[#] = foobar;
Now skb_share_check do:
if skb_is_shared true, then make a clone. This doesn't allow to
manipulate the data, but skb_cow make a copy of the private data so this
is okay for IPHC call. Means after skb_cow, we have something like
skb_copy, but skb_cow should be faster. ;-)
Also skb_share_check checks if skb_is_shared is false, then we don't
need a skb_clone, because it's not shared, then we are allow to
manipulate skb attributes, because nobody else use it.
I would add skb_share_check at beginning of this function like 802.15.4
but the IPV6 DISPATCH of bluetooth do a complete copy of the data buffer
which seems not necessary, but this is another issue and maybe I think
that the IPV6 DISPATCH value isn't needed by bluetooth 6lowpan. <-- But I
am not sure about this. I also can't see that bluetooth 6lowpan runs
skb_pull for the one byte dispatch value. Jukka need to check that, if
he has time and feel like to doing it.
- Alex
Hi Alex,
On 13/10/14 09:55, Alexander Aring wrote:
> Hi Martin,
>
> On Mon, Oct 13, 2014 at 09:44:42AM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> On 11/10/14 08:36, Alexander Aring wrote:
>>> Hi Martin,
>>>
>>> This is only a summarize what we should now to try to do here for next
>>> version.
>>>
>>> I know... this patch ends in a global debate on principles how we deal
>>> with the replacement of 6LoWPAN -> IPv6 header. Current behaviour is
>>> more a hack.
>>>
>>> On Thu, Oct 09, 2014 at 08:46:34AM +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]>
>>>> ---
>>>> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
>>>> net/bluetooth/6lowpan.c | 7 +++++++
>>>> 2 files changed, 32 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;
>>>> + }
>>>> + }
>>> use skb_cow_head here.
>> I don't know much about skb_cow_head but from the description it says that only the head is writable which I assume is the newly requested headroom? I imagine this is for the usual case where you are adding headers and you go through the protocol layers. For 6lowpan aren't we removing one header and replacing with a decompressed header which would suggest that we are going to modify the data (which would contain the 6lowpan header).
>>
>> skb_cow seems a good fit to me though.
> okay, then use skb_cow here. I will remember that we could possible
> maybe use "skb_cow_head" here. But yea I mean we running lot of pulls
> and move the head pointer, then push now data on it, this would
> overwrite the pulled data and other skb's may not pull here. If the data
> is shared there, this would crash.
>
>>>>
>>>> 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;
>>>> + }
>>>> + }
>>> same here.
>>>
>>>> }
>>>>
>>>> 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;
>>>> + }
>>>> +
>>> I see now that case LOWPAN_DISPATCH_IPHC: runs a "local_skb =
>>> skb_clone(skb, GFP_ATOMIC);" and send the local_skb to process_data. So
>>> is the share_check really needed? In my opinion there are several things
>>> wrong.
>>>
>>> What bluetooth should do here is:
>>>
>>> Always run skb = skb_share_check(skb, GFP_ATOMIC); at first of this function.
>>>
>>> In if (skb->data[0] == LOWPAN_DISPATCH_IPV6):
>>>
>>> - only run a skb_pull (doesn't change the data buffer, only skb attribute)
>>> for the one byte dispatch.
>>> - run other things like skb_reset_network_header, etc...
>>>
>>> -> Currently this works because we run a complete skb_copy_expand here.
>>> But this is not needed, we need a clone because we don't modify the
>>> data buffer.
>>>
>>> -> To Jukka: I don't see a skb_pull for the one byte dispatch value?
>>> What's happend here? Also IPv6 dispatch is part of rfc4944 and btle
>>> 6LoWPAN is only realted to rfc6282 (IPHC). I think you don't need handling
>>> for this here.
>>>
>>>
>>>
>>> In "case LOWPAN_DISPATCH_IPHC:" we should remove the skb_clone, because
>>> this is already handled by "skb_share_check" then.
>> Not necessarily, if the skb isn't shared, ie no-one has called skb_get on it then the clone will not happen. I tried removing the skb_clone as in theory psk_expand_head should handle cloned skb's but I think it caused the oops that Jukka was seeing. Because of this I only want to make changes absolutely necessary to get pskb_expand_head or skb_cow working as I can't test the bluetooth code and have to rely on Jukka.
>>
>> If it's ok with everyone I'll change pskb_expand_head to skb_cow or skb_cow_head and leave the bluetooth code as it is with this v5 patch.
> For me it's confusing, you do that on skb, but we never used skb
> afterwards (maybe on freeing) but we use local_skb, the local_skb should
> always be cloned because we running skb_clone before.
skb_clone does ensure that the skb isn't shared anymore so I think it will be safe to remove the skb_share_check. Or I can move it to the start of the function like we do for 802.15.4?
>
> skb_share_check is a function which checks is the skb shared, then clone
> it. But we don't do anything with skb afterwards.
>
> - 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
-Martin.
Hi Martin,
On Mon, Oct 13, 2014 at 09:44:42AM +0100, Martin Townsend wrote:
> Hi Alex,
>
> On 11/10/14 08:36, Alexander Aring wrote:
> > Hi Martin,
> >
> > This is only a summarize what we should now to try to do here for next
> > version.
> >
> > I know... this patch ends in a global debate on principles how we deal
> > with the replacement of 6LoWPAN -> IPv6 header. Current behaviour is
> > more a hack.
> >
> > On Thu, Oct 09, 2014 at 08:46:34AM +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]>
> >> ---
> >> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
> >> net/bluetooth/6lowpan.c | 7 +++++++
> >> 2 files changed, 32 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;
> >> + }
> >> + }
> > use skb_cow_head here.
> I don't know much about skb_cow_head but from the description it says that only the head is writable which I assume is the newly requested headroom? I imagine this is for the usual case where you are adding headers and you go through the protocol layers. For 6lowpan aren't we removing one header and replacing with a decompressed header which would suggest that we are going to modify the data (which would contain the 6lowpan header).
>
> skb_cow seems a good fit to me though.
okay, then use skb_cow here. I will remember that we could possible
maybe use "skb_cow_head" here. But yea I mean we running lot of pulls
and move the head pointer, then push now data on it, this would
overwrite the pulled data and other skb's may not pull here. If the data
is shared there, this would crash.
> >
> >>
> >> 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;
> >> + }
> >> + }
> > same here.
> >
> >> }
> >>
> >> 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;
> >> + }
> >> +
> > I see now that case LOWPAN_DISPATCH_IPHC: runs a "local_skb =
> > skb_clone(skb, GFP_ATOMIC);" and send the local_skb to process_data. So
> > is the share_check really needed? In my opinion there are several things
> > wrong.
> >
> > What bluetooth should do here is:
> >
> > Always run skb = skb_share_check(skb, GFP_ATOMIC); at first of this function.
> >
> > In if (skb->data[0] == LOWPAN_DISPATCH_IPV6):
> >
> > - only run a skb_pull (doesn't change the data buffer, only skb attribute)
> > for the one byte dispatch.
> > - run other things like skb_reset_network_header, etc...
> >
> > -> Currently this works because we run a complete skb_copy_expand here.
> > But this is not needed, we need a clone because we don't modify the
> > data buffer.
> >
> > -> To Jukka: I don't see a skb_pull for the one byte dispatch value?
> > What's happend here? Also IPv6 dispatch is part of rfc4944 and btle
> > 6LoWPAN is only realted to rfc6282 (IPHC). I think you don't need handling
> > for this here.
> >
> >
> >
> > In "case LOWPAN_DISPATCH_IPHC:" we should remove the skb_clone, because
> > this is already handled by "skb_share_check" then.
> Not necessarily, if the skb isn't shared, ie no-one has called skb_get on it then the clone will not happen. I tried removing the skb_clone as in theory psk_expand_head should handle cloned skb's but I think it caused the oops that Jukka was seeing. Because of this I only want to make changes absolutely necessary to get pskb_expand_head or skb_cow working as I can't test the bluetooth code and have to rely on Jukka.
>
> If it's ok with everyone I'll change pskb_expand_head to skb_cow or skb_cow_head and leave the bluetooth code as it is with this v5 patch.
For me it's confusing, you do that on skb, but we never used skb
afterwards (maybe on freeing) but we use local_skb, the local_skb should
always be cloned because we running skb_clone before.
skb_share_check is a function which checks is the skb shared, then clone
it. But we don't do anything with skb afterwards.
- Alex
Hi Alex,
On 11/10/14 08:36, Alexander Aring wrote:
> Hi Martin,
>
> This is only a summarize what we should now to try to do here for next
> version.
>
> I know... this patch ends in a global debate on principles how we deal
> with the replacement of 6LoWPAN -> IPv6 header. Current behaviour is
> more a hack.
>
> On Thu, Oct 09, 2014 at 08:46:34AM +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]>
>> ---
>> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
>> net/bluetooth/6lowpan.c | 7 +++++++
>> 2 files changed, 32 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;
>> + }
>> + }
> use skb_cow_head here.
I don't know much about skb_cow_head but from the description it says that only the head is writable which I assume is the newly requested headroom? I imagine this is for the usual case where you are adding headers and you go through the protocol layers. For 6lowpan aren't we removing one header and replacing with a decompressed header which would suggest that we are going to modify the data (which would contain the 6lowpan header).
skb_cow seems a good fit to me though.
>
>>
>> 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;
>> + }
>> + }
> same here.
>
>> }
>>
>> 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;
>> + }
>> +
> I see now that case LOWPAN_DISPATCH_IPHC: runs a "local_skb =
> skb_clone(skb, GFP_ATOMIC);" and send the local_skb to process_data. So
> is the share_check really needed? In my opinion there are several things
> wrong.
>
> What bluetooth should do here is:
>
> Always run skb = skb_share_check(skb, GFP_ATOMIC); at first of this function.
>
> In if (skb->data[0] == LOWPAN_DISPATCH_IPV6):
>
> - only run a skb_pull (doesn't change the data buffer, only skb attribute)
> for the one byte dispatch.
> - run other things like skb_reset_network_header, etc...
>
> -> Currently this works because we run a complete skb_copy_expand here.
> But this is not needed, we need a clone because we don't modify the
> data buffer.
>
> -> To Jukka: I don't see a skb_pull for the one byte dispatch value?
> What's happend here? Also IPv6 dispatch is part of rfc4944 and btle
> 6LoWPAN is only realted to rfc6282 (IPHC). I think you don't need handling
> for this here.
>
>
>
> In "case LOWPAN_DISPATCH_IPHC:" we should remove the skb_clone, because
> this is already handled by "skb_share_check" then.
Not necessarily, if the skb isn't shared, ie no-one has called skb_get on it then the clone will not happen. I tried removing the skb_clone as in theory psk_expand_head should handle cloned skb's but I think it caused the oops that Jukka was seeing. Because of this I only want to make changes absolutely necessary to get pskb_expand_head or skb_cow working as I can't test the bluetooth code and have to rely on Jukka.
If it's ok with everyone I'll change pskb_expand_head to skb_cow or skb_cow_head and leave the bluetooth code as it is with this v5 patch.
>
> - 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
- Martin
Hi Martin,
This is only a summarize what we should now to try to do here for next
version.
I know... this patch ends in a global debate on principles how we deal
with the replacement of 6LoWPAN -> IPv6 header. Current behaviour is
more a hack.
On Thu, Oct 09, 2014 at 08:46:34AM +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]>
> ---
> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
> net/bluetooth/6lowpan.c | 7 +++++++
> 2 files changed, 32 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;
> + }
> + }
use skb_cow_head here.
>
> 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;
> + }
> + }
same here.
> }
>
> 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;
> + }
> +
I see now that case LOWPAN_DISPATCH_IPHC: runs a "local_skb =
skb_clone(skb, GFP_ATOMIC);" and send the local_skb to process_data. So
is the share_check really needed? In my opinion there are several things
wrong.
What bluetooth should do here is:
Always run skb = skb_share_check(skb, GFP_ATOMIC); at first of this function.
In if (skb->data[0] == LOWPAN_DISPATCH_IPV6):
- only run a skb_pull (doesn't change the data buffer, only skb attribute)
for the one byte dispatch.
- run other things like skb_reset_network_header, etc...
-> Currently this works because we run a complete skb_copy_expand here.
But this is not needed, we need a clone because we don't modify the
data buffer.
-> To Jukka: I don't see a skb_pull for the one byte dispatch value?
What's happend here? Also IPv6 dispatch is part of rfc4944 and btle
6LoWPAN is only realted to rfc6282 (IPHC). I think you don't need handling
for this here.
In "case LOWPAN_DISPATCH_IPHC:" we should remove the skb_clone, because
this is already handled by "skb_share_check" then.
- Alex
Hi Martin,
On Fri, Oct 10, 2014 at 09:41:16PM +0200, Alexander Aring wrote:
...
> > 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);
>
>
> Now we come to this function.
>
> There exist two global rules here:
>
> 1.
>
> If we change only the skb pointers head/tail with e.g. skb_push then we
> need a cloned skb only. We don't change the skb data buffer here.
>
> We need this in LOWPAN_DISPATCH_IPV6, here we run only one skb_pull for
> one byte, and the FRAGN and FRAG1 dispatches in 802.15.4 6LoWPAN.
>
> skb_share_check do this, we have surely a clone afterwards and the
> sk_buff attributes are not changed elsewhere, which means we are
> allowed to manipulate the skb attributes.
>
> 2.
>
> If we touch the data buffer, then we need a skb_copy. We need this at
> LOWPAN_DISPATCH_IPHC because here we replacing the data buffer.
>
> skb_unshare do this for us.
>
This is not correct. I mean the chain of thought is correct. But a
private copy of data buffer is already done by pskb_expand_head, which
is used before modify the data buffer. If we should not run it, then it
would be correct. This handling is done by decompression IPHC.
So we don't need skb_unshare. Now it's very interesting if we can use
skb_cow_head, when replacing transport/network header, because we only
need to modify the headroom. Otherwise we should use skb_cow.
- Alex
Hi Martin,
I reconsider these steps which we do here now and saw some new
improvements/issues.
On Thu, Oct 09, 2014 at 08:46:34AM +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]>
> ---
> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
> net/bluetooth/6lowpan.c | 7 +++++++
> 2 files changed, 32 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;
> + }
> + }
What we here try to do is a usual sk_buff principle.
There exist a function skb_cow [0]:
<qoute>
static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom,
int cloned)
{
int delta = 0;
if (headroom > skb_headroom(skb))
delta = headroom - skb_headroom(skb);
if (delta || cloned)
return pskb_expand_head(skb, ALIGN(delta, NET_SKB_PAD), 0,
GFP_ATOMIC);
return 0;
}
</qoute>
This is the wrapper call which is called by skb_cow. I see here much
similarity and also more performance. It calculates a delta size and do
also a NET_SKB_PAD which is a align to the cache_lines and I think they
try do some "false sharing" here. [1]
We should use this function, if you agree here.
There exist also some skb_cow_head function [2].
code commentar:
<qoute>
It should be used when you only need to push on some header and do not
need to modify the data.
</qoute>
I am not sure about if we can use skb_cow_head here, because we modify
the data. I mean we run skb_pull and then skb_cow_head and run skb_push
with different data buffer. (IPv6 header).
Currently I am not sure if this works.
> }
>
> 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);
Now we come to this function.
There exist two global rules here:
1.
If we change only the skb pointers head/tail with e.g. skb_push then we
need a cloned skb only. We don't change the skb data buffer here.
We need this in LOWPAN_DISPATCH_IPV6, here we run only one skb_pull for
one byte, and the FRAGN and FRAG1 dispatches in 802.15.4 6LoWPAN.
skb_share_check do this, we have surely a clone afterwards and the
sk_buff attributes are not changed elsewhere, which means we are
allowed to manipulate the skb attributes.
2.
If we touch the data buffer, then we need a skb_copy. We need this at
LOWPAN_DISPATCH_IPHC because here we replacing the data buffer.
skb_unshare do this for us.
If we can use skb_cow_head like above, I think then we don't need a
skb_unshare here. But I am not 100% sure if we really can use
skb_cow_head here. We don't need skb_unshare then because, skb_cow_head
makes the head writeable only, this should something similar like a
private copy of headroom then.
Summary:
We need always a skb_share_check at begin of evaluate the dispatch, for
the LOWPAN_DISPATCH_IPHC we need a skb_unshare before. That's fine for
now, maybe later we can do some tricks with skb_cow_head.
skb_share_check:
We always have a cloned skb and we can manipulate sk_buff attributes.
skb_unshare:
We have a private copy of data buffer and can replace 6LoWPAN header
with IPv6 header.
Do you agree here?
- Alex
[0] http://lxr.free-electrons.com/source/include/linux/skbuff.h#L2316
[1] http://en.wikipedia.org/wiki/False_sharing
[2] http://lxr.free-electrons.com/source/include/linux/skbuff.h#L2331
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 +++++++
2 files changed, 32 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);
--
1.9.1