[ovs-dev] [rfc 2/6] ovs-datapath: Open vSwitch Datapath

Jesse Gross jesse at nicira.com
Thu Aug 26 04:33:04 UTC 2010


On Wed, Aug 25, 2010 at 12:53 AM, Stephen Hemminger
<shemminger at vyatta.com> wrote:
> On Wed, 25 Aug 2010 13:08:03 +0900
> Simon Horman <horms at verge.net.au> wrote:
>
>> +static struct sk_buff *make_writable(struct sk_buff *skb, unsigned
>> min_headroom, gfp_t gfp)
>> +{
>> +     if (skb_cloned(skb)) {
>> +             struct sk_buff *nskb;
>> +             unsigned headroom = max(min_headroom, skb_headroom(skb));
>> +
>> +             nskb = skb_copy_expand(skb, headroom, skb_tailroom(skb),
>> gfp);
>> +             if (nskb) {
>> +                     set_skb_csum_bits(skb, nskb);
>> +                     kfree_skb(skb);
>> +                     return nskb;
>> +             }
>> +     } else {
>> +             unsigned int hdr_len = (skb_transport_offset(skb)
>> +                                     + sizeof(struct tcphdr));
>> +             if (pskb_may_pull(skb, min(hdr_len, skb->len)))
>> +                     return skb;
>> +     }
>> +     kfree_skb(skb);
>> +     return NULL;
>> +}
>
> 1. Isn't the same as skb_cow_data()
> 2. If not why not make it generic
> 3. Do you need the gfp argument? Most of the skb rework routines just
>   use GPF_ATOMIC

Yes, this function is both redundant and inefficient.  I've been
meaning to replace it for a while but haven't had time.

As it currently exists, it is very close to skb_cow_data().  However,
both functions actually do more work than is necessary.

The second half of this function (the !skb_cloned() portion) is not
needed since any action that changes the packet needs to ensure that
the relevant fields actually exist in the packet and that happens
using pskb_may_pull(), so the check here isn't needed.

Given that, we know that the data to be touched is in the linear data
area and we just need to copy that if it is cloned (skb_copy_expand()
is overkill).  skb_cow() is really what we want here and is basically
a drop-in replacement.  The only thing that I don't like about it is
that it rounds up headroom to NET_SKB_PAD before it decides whether to
copy, which can potentially lead to unnecessary copies if
skb_headroom() is greater than headroom but less than NET_SKB_PAD.

I agree that it is reasonable to use GFP_ATOMIC in this function (and
elsewhere in this file).  The percentage of packets coming down from
userspace just isn't high enough to be worth passing around the
argument.




More information about the dev mailing list