[ovs-dev] [PATCH] datapath: Pull data into linear area only on demand.

Jesse Gross jesse at nicira.com
Wed May 11 18:37:22 UTC 2011


On Wed, May 11, 2011 at 10:53 AM, Ben Pfaff <blp at nicira.com> wrote:
> On Tue, May 10, 2011 at 02:26:09PM -0700, Jesse Gross wrote:
>> We currently always pull 64 bytes of data (if it exists) into the
>> skb linear data area when parsing flows.  The theory behind this
>> is that the data should always be there and it's enough to parse
>> common flows.  However, this causes a number of problems in
>> different situations.  The first is that it is not enough to handle
>> IPv6 so we must pull additional data anyways.  However, the main
>> problem is that GRO typically allocates a new skb and puts just the
>> headers in there.  For a typical TCP/IPv4 packet there are 54 bytes
>> of headers, which means that we must possibly reallocate and copy
>> on every packet.  In addition, GRO creates frag_lists with this
>> specific geometry in order to allow later segmentation if the packet
>> is forwarded to a device that does not support frag_lists.  When
>> we pull additional data it changes the geometry and causes later
>> problems for the device.  This patch instead incrementally pulls
>> data, which avoids these problems.
>>
>> Signed-off-by: Jesse Gross <jesse at nicira.com>
>> CC: Ian Campbell <Ian.Campbell at citrix.com>
>
> flow_extract() didn't previously assume that any data had been pulled
> into the linear data area.  It now assumes that the skb passed in has
> at least the Ethernet header pulled into the linear data area.  I
> guess this is OK, but I thought I'd point it out anyway.

It's guaranteed that we have at least an Ethernet header here, I'll
add a comment to make that clear.

>
> There's a bitrotted comment in flow_extract() that mentions dl_vlan
> and dl_vlan_pcp, which haven't existed for a while.

I dropped it now.

>
> The reason for the following change to parse_ethertype() isn't obvious
> to me.  Its condition still looks unlikely to me:
>> @@ -291,9 +301,12 @@ static __be16 parse_ethertype(struct sk_buff *skb)
>>       if (ntohs(proto) >= 1536)
>>               return proto;
>>
>> -     if (unlikely(skb->len < sizeof(struct llc_snap_hdr)))
>> +     if (skb->len < sizeof(struct llc_snap_hdr))
>>               return htons(ETH_P_802_2);

This wasn't really an intentional change (I changed this line to
something else and then decided to change it back) but I think it is
actually correct.  These packets are still valid LLC packets and I'm
not sure that we should mark processing paths for valid packets as
unlikely.

> The following __skb_push() call in flow_extract() looks invalid to me
> now.  'eth' needs to be reloaded because there have been intervening
> calls to pskb_may_pull() since it was originally set:
>     __skb_push(skb, skb->data - (unsigned char *)eth);

You're right, I changed it to refresh the pointer.

>
> check_iphdr() can call skb_set_transport_header() to point the L4 header
> to data that has not yet been pulled into the linear data area; that
> is, the L4 header pointer might overrun the end of the linear data
> area.  I don't know whether that's OK.

In order for anything to access the transport header it would need to
check that it is actually present (pulling up to the L4 header will
only ensure that the IP options are there, it says nothing about the
actual L4 header).  If the L4 header is one that we understand then
the parser will pull everything that is necessary, including L3 data.
So the only effect that pulling here has is to ensure that the IP
options are present but we don't do that for TCP options either and
nothing cares about them anyways.

>
> I didn't see anything else.

Thanks, I pushed this.



More information about the dev mailing list