[ovs-dev] [PATCH 2/2] datapath: Hash only the part of struct sw_flow_key populated by flow_extract().

Jesse Gross jesse at nicira.com
Wed May 11 03:10:08 UTC 2011


On Tue, May 10, 2011 at 2:19 PM, Andrew Evans <aevans at nicira.com> wrote:
> On Mon, 2011-05-02 at 15:37 -0700, Jesse Gross wrote:
>> I think what you had previously is actually along the right lines and
>> we definitely need to include L4 headers in the length.  The one
>> difference is that once we start to parse a header group the length
>> needs to be included even if it ultimately turns out to be invalid.  A
>> header group is something that is implied by a next header in the
>> previous layer (i.e. IPv6 is implied by the EtherType).
>>
>> I think the header groups are:
>> * L1/L2 (tun_id, in_port, dl_tci, dl_src, dl_dst)
>> * IPv4 (nw_proto, nw_tos, ipv4.src, ipv4.dst)
>> * IPv6 (nw_proto, nw_tos, ipv6.src, ipv6.dst)
>> * ARP (nw_proto, nw_tos, arp.sha, arp.tha)
>> * TCP/UDP/ICMP over IPv4 (ipv4.tp.src, ipv4.tp.dst)
>> * TCP/UDP/ICMP over IPv6 (ipv6.tp.src, ipv6.tp.dst)
>> * IPv6 ND (ipv6.nd_target, ipv6.nd_sll, ipv6.nd_tll)
>>
>> If we break it more finely than that, then we run into the problems
>> with invalid packets partially matching flows.  However, by breaking
>> it at the layer boundary we know that flows can't have additional data
>> farther on because that would imply that they have the same current
>> layer headers as us, which are zeroed.
>
> That makes sense to me. Ok, here is an incremental diff that should
> return flow key lengths at the end of these boundaries, even when errors
> occur:

This looks about right to me but there have been so many incrementals
to this patch that I've long since lost track.  Can you send it out in
whole again?

Also, we had talked some about grouping together fields that are
always assigned together, such as IP source and dest addresses so that
the length assignment can use the whole block instead of the last
field.  Did we ever come to a conclusion on that?

>
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 241822a..0f851d6 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -547,7 +554,7 @@ int flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key,
>
>                key_len = SW_FLOW_KEY_OFFSET(ipv6);

Is this right?  It seems like it will include all possible IPv6 fields
and will just get overwritten with the correct value in
parse_ipv6hdr() anyways.

>
> -               nh_len = parse_ipv6hdr(skb, key);
> +               nh_len = parse_ipv6hdr(skb, key, &key_len);
>                if (unlikely(nh_len < 0)) {
>                        if (nh_len == -EINVAL)
>                                skb->transport_header = skb->network_header;



More information about the dev mailing list