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

[Andrew Evans]

On Tue, 2011-05-10 at 20:10 -0700, Jesse Gross wrote:
> 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?

Yes, I'll do that once we've resolved the following.

> 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?

I backed out the changes that grouped fields into structs because I
thought you wanted to be consistent with other structs in the datapath,
which don't nest structs except as members of a union. I do still like
the idea of grouping related fields together, since I think it makes it
less likely that future changes to the flow key struct will break flow
key length computation. Do you want me to group them? That's an easy
change to make. Are there other structs you think should be similarly
grouped for the sake of consistency?

> >
> > 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.

Thanks, that was an oversight on my part. I went back and forth a few
times on whether parse_ipv6hdr() should report flow key length, so that
was left over from a prior iteration where it didn't.

Sanity-checking all the flow key length computations again, I noticed
two others that were overly pessimistic. Here's a diff for the one you
caught plus those other two:

diff --git a/datapath/flow.c b/datapath/flow.c
index 0f851d6..a5d0b79 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -526,8 +526,6 @@ int flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key,
 	} else if (key->dl_type == htons(ETH_P_ARP) && arphdr_ok(skb)) {
 		struct arp_eth_header *arp;
 
-		key_len = SW_FLOW_KEY_OFFSET(ipv4);
-
 		arp = (struct arp_eth_header *)skb_network_header(skb);
 
 		if (arp->ar_hrd == htons(ARPHRD_ETHER)
@@ -552,8 +550,6 @@ int flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key,
 	} else if (key->dl_type == htons(ETH_P_IPV6)) {
 		int nh_len;             /* IPv6 Header + Extensions */
 
-		key_len = SW_FLOW_KEY_OFFSET(ipv6);
-
 		nh_len = parse_ipv6hdr(skb, key, &key_len);
 		if (unlikely(nh_len < 0)) {
 			if (nh_len == -EINVAL)
@@ -716,7 +712,7 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_ETHERTYPE, ODP_KEY_ATTR_IPV6):
-			key_len = SW_FLOW_KEY_OFFSET(ipv6);
+			key_len = SW_FLOW_KEY_OFFSET(ipv6.dst);
 			if (swkey->dl_type != htons(ETH_P_IPV6))
 				goto invalid;
 			ipv6_key = nla_data(nla);