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

Andrew Evans aevans at nicira.com
Sat Apr 30 00:01:29 UTC 2011


On Fri, 2011-04-29 at 01:22 -0700, Andrew Evans wrote:
> On 4/28/11 1:37 PM, Jesse Gross wrote:
> > On Mon, Apr 25, 2011 at 6:11 PM, Andrew Evans <aevans at nicira.com> wrote:
> >> @@ -789,6 +826,7 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, const struct nlattr *attr)
> >>                    swkey->nw_proto == IPPROTO_UDP ||
> >>                    swkey->nw_proto == IPPROTO_ICMP)
> >>                        return -EINVAL;
> >> +               *key_len = SW_FLOW_KEY_OFFSET(ipv4.dst);
> > 
> > Why are some of these set above and some down here?  It seems that you
> > could just set the length each time the we add a new segment of the
> > flow.  We enforce ordering, so there's no danger of an earlier element
> > overwriting a later one.  If we set the length for each segment, even
> > those where it isn't strictly necessary it would be much easier to
> > verify that the length is set correctly everywhere.  We might also
> > want to put in a warning to check that the key_len is not zero.
> 
> I agree. I'll fix that tomorrow and post an incremental.

Here's that patch. I also changed key_len to a local variable and consolidated
function return to one place. Sorry for the extra noise.

--- cut here ---
diff --git a/datapath/flow.c b/datapath/flow.c
index aa2c740..14c04c9 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -396,7 +396,7 @@ out:
  * Ethernet header
  * @in_port: port number on which @skb was received.
  * @key: output flow key
- * @key_len: length of output flow key
+ * @key_lenp: length of output flow key
  * @is_frag: set to 1 if @skb contains an IPv4 fragment, or to 0 if @skb does
  * not contain an IPv4 packet or if it is not a fragment.
  *
@@ -606,7 +606,7 @@ int flow_cmp(const struct tbl_node *node, void *key2_, int len)
 /**
  * flow_from_nlattrs - parses Netlink attributes into a flow key.
  * @swkey: receives the extracted flow key.
- * @key_len: number of bytes used in @swkey.
+ * @key_lenp: number of bytes used in @swkey.
  * @attr: Netlink attribute holding nested %ODP_KEY_ATTR_* Netlink attribute
  * sequence.
  *
@@ -616,15 +616,16 @@ int flow_cmp(const struct tbl_node *node, void *key2_, int len)
  * [tun_id] in_port ethernet [8021q] [ethertype \
  *              [IPv4 [TCP|UDP|ICMP] | IPv6 [TCP|UDP|ICMPv6 [ND]] | ARP]]
  */
-int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_len,
+int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
 		      const struct nlattr *attr)
 {
+	int error = 0;
+	int key_len = 0;
 	const struct nlattr *nla;
 	u16 prev_type;
 	int rem;
 
 	memset(swkey, 0, sizeof(*swkey));
-	*key_len = 0;
 	swkey->dl_type = htons(ETH_P_802_2);
 
 	prev_type = ODP_KEY_ATTR_UNSPEC;
@@ -659,59 +660,65 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_len,
                 int type = nla_type(nla);
 
                 if (type > ODP_KEY_ATTR_MAX || nla_len(nla) != key_lens[type])
-                        return -EINVAL;
+                        goto invalid;
 
 #define TRANSITION(PREV_TYPE, TYPE) (((PREV_TYPE) << 16) | (TYPE))
 		switch (TRANSITION(prev_type, type)) {
 		case TRANSITION(ODP_KEY_ATTR_UNSPEC, ODP_KEY_ATTR_TUN_ID):
 			swkey->tun_id = nla_get_be64(nla);
+			key_len = SW_FLOW_KEY_OFFSET(tun_id);
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_UNSPEC, ODP_KEY_ATTR_IN_PORT):
 		case TRANSITION(ODP_KEY_ATTR_TUN_ID, ODP_KEY_ATTR_IN_PORT):
 			if (nla_get_u32(nla) >= DP_MAX_PORTS)
-				return -EINVAL;
+				goto invalid;
 			swkey->in_port = nla_get_u32(nla);
+			key_len = SW_FLOW_KEY_OFFSET(in_port);
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_IN_PORT, ODP_KEY_ATTR_ETHERNET):
 			eth_key = nla_data(nla);
 			memcpy(swkey->dl_src, eth_key->eth_src, ETH_ALEN);
 			memcpy(swkey->dl_dst, eth_key->eth_dst, ETH_ALEN);
+			key_len = SW_FLOW_KEY_OFFSET(dl_dst);
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_ETHERNET, ODP_KEY_ATTR_8021Q):
 			q_key = nla_data(nla);
 			/* Only standard 0x8100 VLANs currently supported. */
 			if (q_key->q_tpid != htons(ETH_P_8021Q))
-				return -EINVAL;
+				goto invalid;
 			if (q_key->q_tci & htons(VLAN_TAG_PRESENT))
-				return -EINVAL;
+				goto invalid;
 			swkey->dl_tci = q_key->q_tci | htons(VLAN_TAG_PRESENT);
+			/* key_len already past dl_tci */
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_8021Q, ODP_KEY_ATTR_ETHERTYPE):
 		case TRANSITION(ODP_KEY_ATTR_ETHERNET, ODP_KEY_ATTR_ETHERTYPE):
 			swkey->dl_type = nla_get_be16(nla);
+			key_len = SW_FLOW_KEY_OFFSET(dl_type);
 			if (ntohs(swkey->dl_type) < 1536)
-				return -EINVAL;
+				goto invalid;
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_ETHERTYPE, ODP_KEY_ATTR_IPV4):
 			if (swkey->dl_type != htons(ETH_P_IP))
-				return -EINVAL;
+				goto invalid;
 			ipv4_key = nla_data(nla);
 			swkey->nw_proto = ipv4_key->ipv4_proto;
 			swkey->nw_tos = ipv4_key->ipv4_tos;
 			swkey->ipv4.src = ipv4_key->ipv4_src;
 			swkey->ipv4.dst = ipv4_key->ipv4_dst;
+			key_len = SW_FLOW_KEY_OFFSET(ipv4.dst);
 			if (swkey->nw_tos & INET_ECN_MASK)
-				return -EINVAL;
+				goto invalid;
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_ETHERTYPE, ODP_KEY_ATTR_IPV6):
 			if (swkey->dl_type != htons(ETH_P_IPV6))
-				return -EINVAL;
+				goto invalid;
 			ipv6_key = nla_data(nla);
 			swkey->nw_proto = ipv6_key->ipv6_proto;
 			swkey->nw_tos = ipv6_key->ipv6_tos;
@@ -719,152 +726,155 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_len,
 					sizeof(swkey->ipv6.src));
 			memcpy(&swkey->ipv6.dst, ipv6_key->ipv6_dst,
 					sizeof(swkey->ipv6.dst));
+			key_len = SW_FLOW_KEY_OFFSET(ipv6.dst);
 			if (swkey->nw_tos & INET_ECN_MASK)
-				return -EINVAL;
+				goto invalid;
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_IPV4, ODP_KEY_ATTR_TCP):
 			if (swkey->nw_proto != IPPROTO_TCP)
-				return -EINVAL;
+				goto invalid;
 			tcp_key = nla_data(nla);
 			swkey->ipv4.tp.src = tcp_key->tcp_src;
 			swkey->ipv4.tp.dst = tcp_key->tcp_dst;
-			*key_len = SW_FLOW_KEY_OFFSET(ipv4.tp);
+			key_len = SW_FLOW_KEY_OFFSET(ipv4.tp);
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_IPV6, ODP_KEY_ATTR_TCP):
 			if (swkey->nw_proto != IPPROTO_TCP)
-				return -EINVAL;
+				goto invalid;
 			tcp_key = nla_data(nla);
 			swkey->ipv6.tp.src = tcp_key->tcp_src;
 			swkey->ipv6.tp.dst = tcp_key->tcp_dst;
-			*key_len = SW_FLOW_KEY_OFFSET(ipv6.tp);
+			key_len = SW_FLOW_KEY_OFFSET(ipv6.tp);
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_IPV4, ODP_KEY_ATTR_UDP):
 			if (swkey->nw_proto != IPPROTO_UDP)
-				return -EINVAL;
+				goto invalid;
 			udp_key = nla_data(nla);
 			swkey->ipv4.tp.src = udp_key->udp_src;
 			swkey->ipv4.tp.dst = udp_key->udp_dst;
-			*key_len = SW_FLOW_KEY_OFFSET(ipv4.tp);
+			key_len = SW_FLOW_KEY_OFFSET(ipv4.tp);
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_IPV6, ODP_KEY_ATTR_UDP):
 			if (swkey->nw_proto != IPPROTO_UDP)
-				return -EINVAL;
+				goto invalid;
 			udp_key = nla_data(nla);
 			swkey->ipv6.tp.src = udp_key->udp_src;
 			swkey->ipv6.tp.dst = udp_key->udp_dst;
-			*key_len = SW_FLOW_KEY_OFFSET(ipv6.tp);
+			key_len = SW_FLOW_KEY_OFFSET(ipv6.tp);
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_IPV4, ODP_KEY_ATTR_ICMP):
 			if (swkey->nw_proto != IPPROTO_ICMP)
-				return -EINVAL;
+				goto invalid;
 			icmp_key = nla_data(nla);
 			swkey->ipv4.tp.src = htons(icmp_key->icmp_type);
 			swkey->ipv4.tp.dst = htons(icmp_key->icmp_code);
-			*key_len = SW_FLOW_KEY_OFFSET(ipv4.tp);
+			key_len = SW_FLOW_KEY_OFFSET(ipv4.tp);
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_IPV6, ODP_KEY_ATTR_ICMPV6):
 			if (swkey->nw_proto != IPPROTO_ICMPV6)
-				return -EINVAL;
+				goto invalid;
 			icmpv6_key = nla_data(nla);
 			swkey->ipv6.tp.src = htons(icmpv6_key->icmpv6_type);
 			swkey->ipv6.tp.dst = htons(icmpv6_key->icmpv6_code);
+			key_len = SW_FLOW_KEY_OFFSET(ipv6.tp);
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_ETHERTYPE, ODP_KEY_ATTR_ARP):
 			if (swkey->dl_type != htons(ETH_P_ARP))
-				return -EINVAL;
+				goto invalid;
 			arp_key = nla_data(nla);
 			swkey->ipv4.src = arp_key->arp_sip;
 			swkey->ipv4.dst = arp_key->arp_tip;
+			key_len = SW_FLOW_KEY_OFFSET(ipv4.dst);
 			if (arp_key->arp_op & htons(0xff00))
-				return -EINVAL;
+				goto invalid;
 			swkey->nw_proto = ntohs(arp_key->arp_op);
 			memcpy(swkey->ipv4.arp.sha, arp_key->arp_sha, ETH_ALEN);
 			memcpy(swkey->ipv4.arp.tha, arp_key->arp_tha, ETH_ALEN);
-			*key_len = SW_FLOW_KEY_OFFSET(ipv4.arp);
+			key_len = SW_FLOW_KEY_OFFSET(ipv4.arp);
 			break;
 
 		case TRANSITION(ODP_KEY_ATTR_ICMPV6, ODP_KEY_ATTR_ND):
 			if (swkey->ipv6.tp.src != htons(NDISC_NEIGHBOUR_SOLICITATION)
 			    && swkey->ipv6.tp.src != htons(NDISC_NEIGHBOUR_ADVERTISEMENT))
-				return -EINVAL;
+				goto invalid;
 			nd_key = nla_data(nla);
 			memcpy(&swkey->ipv6.nd.target, nd_key->nd_target,
 					sizeof(swkey->ipv6.nd.target));
 			memcpy(swkey->ipv6.nd.sll, nd_key->nd_sll, ETH_ALEN);
 			memcpy(swkey->ipv6.nd.tll, nd_key->nd_tll, ETH_ALEN);
-			*key_len = SW_FLOW_KEY_OFFSET(ipv6.nd);
+			key_len = SW_FLOW_KEY_OFFSET(ipv6.nd);
 			break;
 
 		default:
-			return -EINVAL;
+			goto invalid;
 		}
 
 		prev_type = type;
 	}
 	if (rem)
-		return -EINVAL;
+		goto invalid;
 
 	switch (prev_type) {
 	case ODP_KEY_ATTR_UNSPEC:
-		return -EINVAL;
+		goto invalid;
 
 	case ODP_KEY_ATTR_TUN_ID:
 	case ODP_KEY_ATTR_IN_PORT:
-		return -EINVAL;
+		goto invalid;
 
 	case ODP_KEY_ATTR_ETHERNET:
 	case ODP_KEY_ATTR_8021Q:
-		*key_len = SW_FLOW_KEY_OFFSET(dl_type);
-		return 0;
+		goto ok;
 
 	case ODP_KEY_ATTR_ETHERTYPE:
 		if (swkey->dl_type == htons(ETH_P_IP) ||
 		    swkey->dl_type == htons(ETH_P_ARP))
-			return -EINVAL;
-		*key_len = SW_FLOW_KEY_OFFSET(dl_type);
-		return 0;
+			goto invalid;
+		goto ok;
 
 	case ODP_KEY_ATTR_IPV4:
 		if (swkey->nw_proto == IPPROTO_TCP ||
 		    swkey->nw_proto == IPPROTO_UDP ||
 		    swkey->nw_proto == IPPROTO_ICMP)
-			return -EINVAL;
-		*key_len = SW_FLOW_KEY_OFFSET(ipv4.dst);
-		return 0;
+			goto invalid;
+		goto ok;
 
 	case ODP_KEY_ATTR_IPV6:
 		if (swkey->nw_proto == IPPROTO_TCP ||
 		    swkey->nw_proto == IPPROTO_UDP ||
 		    swkey->nw_proto == IPPROTO_ICMPV6)
-			return -EINVAL;
-		*key_len = SW_FLOW_KEY_OFFSET(ipv6.dst);
-		return 0;
+			goto invalid;
+		goto ok;
 
 	case ODP_KEY_ATTR_ICMPV6:
 		if (swkey->ipv6.tp.src == htons(NDISC_NEIGHBOUR_SOLICITATION) ||
 		    swkey->ipv6.tp.src == htons(NDISC_NEIGHBOUR_ADVERTISEMENT))
-			return -EINVAL;
-		*key_len = SW_FLOW_KEY_OFFSET(ipv6.tp);
-		return 0;
+			goto invalid;
+		goto ok;
 
 	case ODP_KEY_ATTR_TCP:
 	case ODP_KEY_ATTR_UDP:
 	case ODP_KEY_ATTR_ICMP:
 	case ODP_KEY_ATTR_ARP:
 	case ODP_KEY_ATTR_ND:
-		/* *key_len already set above */
-		return 0;
+		goto ok;
 	}
 
+invalid:
 	WARN_ON_ONCE(1);
-	return -EINVAL;
+	error = -EINVAL;
+
+ok:
+	WARN_ON_ONCE(!key_len && !error);
+	*key_lenp = key_len;
+	return error;
 }
 
 int flow_to_nlattrs(const struct sw_flow_key *swkey, struct sk_buff *skb)
--- cut here ---





More information about the dev mailing list