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 <[email protected]>
> CC: Ian Campbell <[email protected]>
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.
There's a bitrotted comment in flow_extract() that mentions dl_vlan
and dl_vlan_pcp, which haven't existed for a while.
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);
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);
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.
I didn't see anything else.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
