[ovs-dev,v1] flow.c: Refactor the key_extract function in parsing frame.

Submitted by Gao Zhenyu on April 21, 2017, 7:21 a.m.

Details

Message ID 1492759292-31031-1-git-send-email-sysugaozhenyu@gmail.com
State Deferred
Headers show

Commit Message

Gao Zhenyu April 21, 2017, 7:21 a.m.
1. Consume switch/case to judge type of frame instead of using if/else.
2. Add parse_ipv4hdr for ipv4 frame header parsing.

Signed-off-by: Zhenyu Gao <sysugaozhenyu@gmail.com>
---
 datapath/flow.c | 230 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 117 insertions(+), 113 deletions(-)

Comments

Greg Rose April 21, 2017, 2:56 p.m.
On Fri, 2017-04-21 at 00:21 -0700, Zhenyu Gao wrote:
> 1. Consume switch/case to judge type of frame instead of using if/else.
> 2. Add parse_ipv4hdr for ipv4 frame header parsing.
> 

I think this patch addresses too many issues and should be broken up
into at least two different patches with appropriate subjects and commit
comments for each.

Thanks,

- Greg

> Signed-off-by: Zhenyu Gao <sysugaozhenyu@gmail.com>
> ---
>  datapath/flow.c | 230 ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 117 insertions(+), 113 deletions(-)
> 
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2bc1ad0..0b35de6 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
>  				  sizeof(struct icmphdr));
>  }
>  
> +/**
> +  * Parse ipv4 header from an Ethernet frame.
> +  * Return ipv4 header length if successful, otherwise a negative errno value.
> +  */
> +static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> +	int err;
> +	struct iphdr *nh;
> +	__be16 offset;
> +
> +	err = check_iphdr(skb);
> +	if (unlikely(err))
> +		return err;
> +
> +	nh = ip_hdr(skb);
> +	key->ipv4.addr.src = nh->saddr;
> +	key->ipv4.addr.dst = nh->daddr;
> +
> +	key->ip.proto = nh->protocol;
> +	key->ip.tos = nh->tos;
> +	key->ip.ttl = nh->ttl;
> +
> +	offset = nh->frag_off & htons(IP_OFFSET);
> +	if (offset) {
> +		key->ip.frag = OVS_FRAG_TYPE_LATER;
> +	} else {
> +		if (nh->frag_off & htons(IP_MF) ||
> +				skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
> +			key->ip.frag = OVS_FRAG_TYPE_FIRST;
> +		} else {
> +			key->ip.frag = OVS_FRAG_TYPE_NONE;
> +		}
> +	}
> +	return ip_hdrlen(skb);
> +}
> +
> +/**
> +  * Parse ipv6 header from an Ethernet frame.
> +  * Return ipv6 header length if successful, otherwise a negative errno value.
> +  */
>  static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
>  {
>  	unsigned int nh_ofs = skb_network_offset(skb);
> @@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
>  		else
>  			key->ip.frag = OVS_FRAG_TYPE_FIRST;
>  	} else {
> -		key->ip.frag = OVS_FRAG_TYPE_NONE;
> +		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> +			key->ip.frag = OVS_FRAG_TYPE_FIRST;
> +		else
> +			key->ip.frag = OVS_FRAG_TYPE_NONE;
>  	}
>  
>  	/* Delayed handling of error in ipv6_skip_exthdr() as it
> @@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>  	key->eth.type = skb->protocol;
>  
>  	/* Network layer. */
> -	if (key->eth.type == htons(ETH_P_IP)) {
> -		struct iphdr *nh;
> -		__be16 offset;
> +	switch(key->eth.type) {
> +	case htons(ETH_P_IP):
> +	case htons(ETH_P_IPV6): {
> +		int nh_len;
> +		if (key->eth.type == htons(ETH_P_IP)) {
> +			nh_len = parse_ipv4hdr(skb, key);
> +		} else {
> +			nh_len = parse_ipv6hdr(skb, key);
> +		}
>  
> -		error = check_iphdr(skb);
> -		if (unlikely(error)) {
> -			memset(&key->ip, 0, sizeof(key->ip));
> -			memset(&key->ipv4, 0, sizeof(key->ipv4));
> -			if (error == -EINVAL) {
> +		if (unlikely(nh_len < 0)) {
> +			switch (nh_len) {
> +			case -EINVAL:
> +				memset(&key->ip, 0, sizeof(key->ip));
> +				if (key->eth.type == htons(ETH_P_IP)) {
> +					memset(&key->ipv4.addr, 0, sizeof(key->ipv4.addr));
> +				} else {
> +					memset(&key->ipv6.addr, 0, sizeof(key->ipv6.addr));
> +				}
> +				/* fall-through */
> +			case -EPROTO:
>  				skb->transport_header = skb->network_header;
>  				error = 0;
> +				break;
> +			default:
> +				error = nh_len;
>  			}
>  			return error;
>  		}
>  
> -		nh = ip_hdr(skb);
> -		key->ipv4.addr.src = nh->saddr;
> -		key->ipv4.addr.dst = nh->daddr;
> -
> -		key->ip.proto = nh->protocol;
> -		key->ip.tos = nh->tos;
> -		key->ip.ttl = nh->ttl;
> -
> -		offset = nh->frag_off & htons(IP_OFFSET);
> -		if (offset) {
> -			key->ip.frag = OVS_FRAG_TYPE_LATER;
> +		if (key->ip.frag == OVS_FRAG_TYPE_LATER) {
>  			return 0;
>  		}
> -		if (nh->frag_off & htons(IP_MF) ||
> -			skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> -			key->ip.frag = OVS_FRAG_TYPE_FIRST;
> -		else
> -			key->ip.frag = OVS_FRAG_TYPE_NONE;
>  
>  		/* Transport layer. */
> -		if (key->ip.proto == IPPROTO_TCP) {
> +		switch(key->ip.proto) {
> +		case IPPROTO_TCP:
>  			if (tcphdr_ok(skb)) {
>  				struct tcphdr *tcp = tcp_hdr(skb);
>  				key->tp.src = tcp->source;
> @@ -605,8 +649,8 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>  			} else {
>  				memset(&key->tp, 0, sizeof(key->tp));
>  			}
> -
> -		} else if (key->ip.proto == IPPROTO_UDP) {
> +			break;
> +		case IPPROTO_UDP:
>  			if (udphdr_ok(skb)) {
>  				struct udphdr *udp = udp_hdr(skb);
>  				key->tp.src = udp->source;
> @@ -614,7 +658,8 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>  			} else {
>  				memset(&key->tp, 0, sizeof(key->tp));
>  			}
> -		} else if (key->ip.proto == IPPROTO_SCTP) {
> +			break;
> +		case IPPROTO_SCTP:
>  			if (sctphdr_ok(skb)) {
>  				struct sctphdr *sctp = sctp_hdr(skb);
>  				key->tp.src = sctp->source;
> @@ -622,7 +667,8 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>  			} else {
>  				memset(&key->tp, 0, sizeof(key->tp));
>  			}
> -		} else if (key->ip.proto == IPPROTO_ICMP) {
> +			break;
> +		case IPPROTO_ICMP:
>  			if (icmphdr_ok(skb)) {
>  				struct icmphdr *icmp = icmp_hdr(skb);
>  				/* The ICMP type and code fields use the 16-bit
> @@ -634,10 +680,23 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>  			} else {
>  				memset(&key->tp, 0, sizeof(key->tp));
>  			}
> +			break;
> +		case NEXTHDR_ICMP:
> +			if (icmp6hdr_ok(skb)) {
> +				error = parse_icmpv6(skb, key, nh_len);
> +				if (error)
> +					return error;
> +			} else {
> +				memset(&key->tp, 0, sizeof(key->tp));
> +			}
> +			break;
> +		default:
> +			break;
>  		}
> -
> -	} else if (key->eth.type == htons(ETH_P_ARP) ||
> -		   key->eth.type == htons(ETH_P_RARP)) {
> +		break;
> +	}
> +	case htons(ETH_P_ARP):
> +	case htons(ETH_P_RARP): {
>  		struct arp_eth_header *arp;
>  		bool arp_available = arphdr_ok(skb);
>  
> @@ -663,94 +722,39 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>  			memset(&key->ip, 0, sizeof(key->ip));
>  			memset(&key->ipv4, 0, sizeof(key->ipv4));
>  		}
> -	} else if (eth_p_mpls(key->eth.type)) {
> -		size_t stack_len = MPLS_HLEN;
> -
> -		/* In the presence of an MPLS label stack the end of the L2
> -		 * header and the beginning of the L3 header differ.
> -		 *
> -		 * Advance network_header to the beginning of the L3
> -		 * header. mac_len corresponds to the end of the L2 header.
> -		 */
> -		while (1) {
> -			__be32 lse;
> +		break;
> +	}
> +	default:
> +		if (eth_p_mpls(key->eth.type)) {
> +			size_t stack_len = MPLS_HLEN;
> +
> +			/* In the presence of an MPLS label stack the end of the L2
> +			 * header and the beginning of the L3 header differ.
> +			 *
> +			 * Advance network_header to the beginning of the L3
> +			 * header. mac_len corresponds to the end of the L2 header.
> +			 */
> +			while (1) {
> +				__be32 lse;
>  
> -			error = check_header(skb, skb->mac_len + stack_len);
> -			if (unlikely(error))
> -				return 0;
> +				error = check_header(skb, skb->mac_len + stack_len);
> +				if (unlikely(error))
> +					return 0;
>  
> -			memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
> +				memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
>  
> -			if (stack_len == MPLS_HLEN)
> -				memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
> +				if (stack_len == MPLS_HLEN)
> +					memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
>  
> -			skb_set_network_header(skb, skb->mac_len + stack_len);
> -			if (lse & htonl(MPLS_LS_S_MASK))
> -				break;
> +				skb_set_network_header(skb, skb->mac_len + stack_len);
> +				if (lse & htonl(MPLS_LS_S_MASK))
> +					break;
>  
> -			stack_len += MPLS_HLEN;
> -		}
> -	} else if (key->eth.type == htons(ETH_P_IPV6)) {
> -		int nh_len;             /* IPv6 Header + Extensions */
> -
> -		nh_len = parse_ipv6hdr(skb, key);
> -		if (unlikely(nh_len < 0)) {
> -			switch (nh_len) {
> -			case -EINVAL:
> -				memset(&key->ip, 0, sizeof(key->ip));
> -				memset(&key->ipv6.addr, 0, sizeof(key->ipv6.addr));
> -				/* fall-through */
> -			case -EPROTO:
> -				skb->transport_header = skb->network_header;
> -				error = 0;
> -				break;
> -			default:
> -				error = nh_len;
> -			}
> -			return error;
> -		}
> -
> -		if (key->ip.frag == OVS_FRAG_TYPE_LATER)
> -			return 0;
> -		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> -			key->ip.frag = OVS_FRAG_TYPE_FIRST;
> -
> -		/* Transport layer. */
> -		if (key->ip.proto == NEXTHDR_TCP) {
> -			if (tcphdr_ok(skb)) {
> -				struct tcphdr *tcp = tcp_hdr(skb);
> -				key->tp.src = tcp->source;
> -				key->tp.dst = tcp->dest;
> -				key->tp.flags = TCP_FLAGS_BE16(tcp);
> -			} else {
> -				memset(&key->tp, 0, sizeof(key->tp));
> -			}
> -		} else if (key->ip.proto == NEXTHDR_UDP) {
> -			if (udphdr_ok(skb)) {
> -				struct udphdr *udp = udp_hdr(skb);
> -				key->tp.src = udp->source;
> -				key->tp.dst = udp->dest;
> -			} else {
> -				memset(&key->tp, 0, sizeof(key->tp));
> -			}
> -		} else if (key->ip.proto == NEXTHDR_SCTP) {
> -			if (sctphdr_ok(skb)) {
> -				struct sctphdr *sctp = sctp_hdr(skb);
> -				key->tp.src = sctp->source;
> -				key->tp.dst = sctp->dest;
> -			} else {
> -				memset(&key->tp, 0, sizeof(key->tp));
> -			}
> -		} else if (key->ip.proto == NEXTHDR_ICMP) {
> -			if (icmp6hdr_ok(skb)) {
> -				error = parse_icmpv6(skb, key, nh_len);
> -				if (error)
> -					return error;
> -			} else {
> -				memset(&key->tp, 0, sizeof(key->tp));
> +				stack_len += MPLS_HLEN;
>  			}
>  		}
>  	}
> +
>  	return 0;
>  }
>
Jarno Rajahalme April 21, 2017, 8:24 p.m.
As a policy, Linux kernel datapath changes other than backports need to go to upstream Linux first, new features to net-next tree, and bug fixes to net tree. See the documentation file ‘backporting-patches.rst’ in directory 'Documentation/internals/contributing/‘ of the OVS tree for more detailed description of the process.

Also, the commit message should contain a clear motivation for the change. Changes that enhance readability may adversely affect datapath performance, so having a report on performance testing would be helpful in determining whether to apply the change.

Regards,

  Jarno

> On Apr 21, 2017, at 12:21 AM, Zhenyu Gao <sysugaozhenyu@gmail.com> wrote:
> 
> 1. Consume switch/case to judge type of frame instead of using if/else.
> 2. Add parse_ipv4hdr for ipv4 frame header parsing.
> 
> Signed-off-by: Zhenyu Gao <sysugaozhenyu@gmail.com>
> ---
> datapath/flow.c | 230 ++++++++++++++++++++++++++++----------------------------
> 1 file changed, 117 insertions(+), 113 deletions(-)
> 
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2bc1ad0..0b35de6 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
> 				  sizeof(struct icmphdr));
> }
> 
> +/**
> +  * Parse ipv4 header from an Ethernet frame.
> +  * Return ipv4 header length if successful, otherwise a negative errno value.
> +  */
> +static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> +	int err;
> +	struct iphdr *nh;
> +	__be16 offset;
> +
> +	err = check_iphdr(skb);
> +	if (unlikely(err))
> +		return err;
> +
> +	nh = ip_hdr(skb);
> +	key->ipv4.addr.src = nh->saddr;
> +	key->ipv4.addr.dst = nh->daddr;
> +
> +	key->ip.proto = nh->protocol;
> +	key->ip.tos = nh->tos;
> +	key->ip.ttl = nh->ttl;
> +
> +	offset = nh->frag_off & htons(IP_OFFSET);
> +	if (offset) {
> +		key->ip.frag = OVS_FRAG_TYPE_LATER;
> +	} else {
> +		if (nh->frag_off & htons(IP_MF) ||
> +				skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
> +			key->ip.frag = OVS_FRAG_TYPE_FIRST;
> +		} else {
> +			key->ip.frag = OVS_FRAG_TYPE_NONE;
> +		}
> +	}
> +	return ip_hdrlen(skb);
> +}
> +
> +/**
> +  * Parse ipv6 header from an Ethernet frame.
> +  * Return ipv6 header length if successful, otherwise a negative errno value.
> +  */
> static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
> {
> 	unsigned int nh_ofs = skb_network_offset(skb);
> @@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
> 		else
> 			key->ip.frag = OVS_FRAG_TYPE_FIRST;
> 	} else {
> -		key->ip.frag = OVS_FRAG_TYPE_NONE;
> +		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> +			key->ip.frag = OVS_FRAG_TYPE_FIRST;
> +		else
> +			key->ip.frag = OVS_FRAG_TYPE_NONE;
> 	}
> 
> 	/* Delayed handling of error in ipv6_skip_exthdr() as it
> @@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> 	key->eth.type = skb->protocol;
> 
> 	/* Network layer. */
> -	if (key->eth.type == htons(ETH_P_IP)) {
> -		struct iphdr *nh;
> -		__be16 offset;
> +	switch(key->eth.type) {
> +	case htons(ETH_P_IP):
> +	case htons(ETH_P_IPV6): {
> +		int nh_len;
> +		if (key->eth.type == htons(ETH_P_IP)) {
> +			nh_len = parse_ipv4hdr(skb, key);
> +		} else {
> +			nh_len = parse_ipv6hdr(skb, key);
> +		}
> 
> -		error = check_iphdr(skb);
> -		if (unlikely(error)) {
> -			memset(&key->ip, 0, sizeof(key->ip));
> -			memset(&key->ipv4, 0, sizeof(key->ipv4));
> -			if (error == -EINVAL) {
> +		if (unlikely(nh_len < 0)) {
> +			switch (nh_len) {
> +			case -EINVAL:
> +				memset(&key->ip, 0, sizeof(key->ip));
> +				if (key->eth.type == htons(ETH_P_IP)) {
> +					memset(&key->ipv4.addr, 0, sizeof(key->ipv4.addr));
> +				} else {
> +					memset(&key->ipv6.addr, 0, sizeof(key->ipv6.addr));
> +				}
> +				/* fall-through */
> +			case -EPROTO:
> 				skb->transport_header = skb->network_header;
> 				error = 0;
> +				break;
> +			default:
> +				error = nh_len;
> 			}
> 			return error;
> 		}
> 
> -		nh = ip_hdr(skb);
> -		key->ipv4.addr.src = nh->saddr;
> -		key->ipv4.addr.dst = nh->daddr;
> -
> -		key->ip.proto = nh->protocol;
> -		key->ip.tos = nh->tos;
> -		key->ip.ttl = nh->ttl;
> -
> -		offset = nh->frag_off & htons(IP_OFFSET);
> -		if (offset) {
> -			key->ip.frag = OVS_FRAG_TYPE_LATER;
> +		if (key->ip.frag == OVS_FRAG_TYPE_LATER) {
> 			return 0;
> 		}
> -		if (nh->frag_off & htons(IP_MF) ||
> -			skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> -			key->ip.frag = OVS_FRAG_TYPE_FIRST;
> -		else
> -			key->ip.frag = OVS_FRAG_TYPE_NONE;
> 
> 		/* Transport layer. */
> -		if (key->ip.proto == IPPROTO_TCP) {
> +		switch(key->ip.proto) {
> +		case IPPROTO_TCP:
> 			if (tcphdr_ok(skb)) {
> 				struct tcphdr *tcp = tcp_hdr(skb);
> 				key->tp.src = tcp->source;
> @@ -605,8 +649,8 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> 			} else {
> 				memset(&key->tp, 0, sizeof(key->tp));
> 			}
> -
> -		} else if (key->ip.proto == IPPROTO_UDP) {
> +			break;
> +		case IPPROTO_UDP:
> 			if (udphdr_ok(skb)) {
> 				struct udphdr *udp = udp_hdr(skb);
> 				key->tp.src = udp->source;
> @@ -614,7 +658,8 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> 			} else {
> 				memset(&key->tp, 0, sizeof(key->tp));
> 			}
> -		} else if (key->ip.proto == IPPROTO_SCTP) {
> +			break;
> +		case IPPROTO_SCTP:
> 			if (sctphdr_ok(skb)) {
> 				struct sctphdr *sctp = sctp_hdr(skb);
> 				key->tp.src = sctp->source;
> @@ -622,7 +667,8 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> 			} else {
> 				memset(&key->tp, 0, sizeof(key->tp));
> 			}
> -		} else if (key->ip.proto == IPPROTO_ICMP) {
> +			break;
> +		case IPPROTO_ICMP:
> 			if (icmphdr_ok(skb)) {
> 				struct icmphdr *icmp = icmp_hdr(skb);
> 				/* The ICMP type and code fields use the 16-bit
> @@ -634,10 +680,23 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> 			} else {
> 				memset(&key->tp, 0, sizeof(key->tp));
> 			}
> +			break;
> +		case NEXTHDR_ICMP:
> +			if (icmp6hdr_ok(skb)) {
> +				error = parse_icmpv6(skb, key, nh_len);
> +				if (error)
> +					return error;
> +			} else {
> +				memset(&key->tp, 0, sizeof(key->tp));
> +			}
> +			break;
> +		default:
> +			break;
> 		}
> -
> -	} else if (key->eth.type == htons(ETH_P_ARP) ||
> -		   key->eth.type == htons(ETH_P_RARP)) {
> +		break;
> +	}
> +	case htons(ETH_P_ARP):
> +	case htons(ETH_P_RARP): {
> 		struct arp_eth_header *arp;
> 		bool arp_available = arphdr_ok(skb);
> 
> @@ -663,94 +722,39 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> 			memset(&key->ip, 0, sizeof(key->ip));
> 			memset(&key->ipv4, 0, sizeof(key->ipv4));
> 		}
> -	} else if (eth_p_mpls(key->eth.type)) {
> -		size_t stack_len = MPLS_HLEN;
> -
> -		/* In the presence of an MPLS label stack the end of the L2
> -		 * header and the beginning of the L3 header differ.
> -		 *
> -		 * Advance network_header to the beginning of the L3
> -		 * header. mac_len corresponds to the end of the L2 header.
> -		 */
> -		while (1) {
> -			__be32 lse;
> +		break;
> +	}
> +	default:
> +		if (eth_p_mpls(key->eth.type)) {
> +			size_t stack_len = MPLS_HLEN;
> +
> +			/* In the presence of an MPLS label stack the end of the L2
> +			 * header and the beginning of the L3 header differ.
> +			 *
> +			 * Advance network_header to the beginning of the L3
> +			 * header. mac_len corresponds to the end of the L2 header.
> +			 */
> +			while (1) {
> +				__be32 lse;
> 
> -			error = check_header(skb, skb->mac_len + stack_len);
> -			if (unlikely(error))
> -				return 0;
> +				error = check_header(skb, skb->mac_len + stack_len);
> +				if (unlikely(error))
> +					return 0;
> 
> -			memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
> +				memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
> 
> -			if (stack_len == MPLS_HLEN)
> -				memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
> +				if (stack_len == MPLS_HLEN)
> +					memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
> 
> -			skb_set_network_header(skb, skb->mac_len + stack_len);
> -			if (lse & htonl(MPLS_LS_S_MASK))
> -				break;
> +				skb_set_network_header(skb, skb->mac_len + stack_len);
> +				if (lse & htonl(MPLS_LS_S_MASK))
> +					break;
> 
> -			stack_len += MPLS_HLEN;
> -		}
> -	} else if (key->eth.type == htons(ETH_P_IPV6)) {
> -		int nh_len;             /* IPv6 Header + Extensions */
> -
> -		nh_len = parse_ipv6hdr(skb, key);
> -		if (unlikely(nh_len < 0)) {
> -			switch (nh_len) {
> -			case -EINVAL:
> -				memset(&key->ip, 0, sizeof(key->ip));
> -				memset(&key->ipv6.addr, 0, sizeof(key->ipv6.addr));
> -				/* fall-through */
> -			case -EPROTO:
> -				skb->transport_header = skb->network_header;
> -				error = 0;
> -				break;
> -			default:
> -				error = nh_len;
> -			}
> -			return error;
> -		}
> -
> -		if (key->ip.frag == OVS_FRAG_TYPE_LATER)
> -			return 0;
> -		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> -			key->ip.frag = OVS_FRAG_TYPE_FIRST;
> -
> -		/* Transport layer. */
> -		if (key->ip.proto == NEXTHDR_TCP) {
> -			if (tcphdr_ok(skb)) {
> -				struct tcphdr *tcp = tcp_hdr(skb);
> -				key->tp.src = tcp->source;
> -				key->tp.dst = tcp->dest;
> -				key->tp.flags = TCP_FLAGS_BE16(tcp);
> -			} else {
> -				memset(&key->tp, 0, sizeof(key->tp));
> -			}
> -		} else if (key->ip.proto == NEXTHDR_UDP) {
> -			if (udphdr_ok(skb)) {
> -				struct udphdr *udp = udp_hdr(skb);
> -				key->tp.src = udp->source;
> -				key->tp.dst = udp->dest;
> -			} else {
> -				memset(&key->tp, 0, sizeof(key->tp));
> -			}
> -		} else if (key->ip.proto == NEXTHDR_SCTP) {
> -			if (sctphdr_ok(skb)) {
> -				struct sctphdr *sctp = sctp_hdr(skb);
> -				key->tp.src = sctp->source;
> -				key->tp.dst = sctp->dest;
> -			} else {
> -				memset(&key->tp, 0, sizeof(key->tp));
> -			}
> -		} else if (key->ip.proto == NEXTHDR_ICMP) {
> -			if (icmp6hdr_ok(skb)) {
> -				error = parse_icmpv6(skb, key, nh_len);
> -				if (error)
> -					return error;
> -			} else {
> -				memset(&key->tp, 0, sizeof(key->tp));
> +				stack_len += MPLS_HLEN;
> 			}
> 		}
> 	}
> +
> 	return 0;
> }
> 
> -- 
> 1.9.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Gao Zhenyu April 22, 2017, 2:14 a.m.
Hi Jarno,

Thanks for the suggestion,  I will try to get a performance number and
submit it into Linux first. :)

Thanks
Zhenyu Gao

2017-04-22 4:24 GMT+08:00 Jarno Rajahalme <jarno@ovn.org>:

> As a policy, Linux kernel datapath changes other than backports need to go
> to upstream Linux first, new features to net-next tree, and bug fixes to
> net tree. See the documentation file ‘backporting-patches.rst’ in directory
> 'Documentation/internals/contributing/‘ of the OVS tree for more detailed
> description of the process.
>
> Also, the commit message should contain a clear motivation for the change.
> Changes that enhance readability may adversely affect datapath performance,
> so having a report on performance testing would be helpful in determining
> whether to apply the change.
>
> Regards,
>
>   Jarno
>
> > On Apr 21, 2017, at 12:21 AM, Zhenyu Gao <sysugaozhenyu@gmail.com>
> wrote:
> >
> > 1. Consume switch/case to judge type of frame instead of using if/else.
> > 2. Add parse_ipv4hdr for ipv4 frame header parsing.
> >
> > Signed-off-by: Zhenyu Gao <sysugaozhenyu@gmail.com>
> > ---
> > datapath/flow.c | 230 ++++++++++++++++++++++++++++--
> --------------------------
> > 1 file changed, 117 insertions(+), 113 deletions(-)
> >
> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index 2bc1ad0..0b35de6 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
> >                                 sizeof(struct icmphdr));
> > }
> >
> > +/**
> > +  * Parse ipv4 header from an Ethernet frame.
> > +  * Return ipv4 header length if successful, otherwise a negative errno
> value.
> > +  */
> > +static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
> > +{
> > +     int err;
> > +     struct iphdr *nh;
> > +     __be16 offset;
> > +
> > +     err = check_iphdr(skb);
> > +     if (unlikely(err))
> > +             return err;
> > +
> > +     nh = ip_hdr(skb);
> > +     key->ipv4.addr.src = nh->saddr;
> > +     key->ipv4.addr.dst = nh->daddr;
> > +
> > +     key->ip.proto = nh->protocol;
> > +     key->ip.tos = nh->tos;
> > +     key->ip.ttl = nh->ttl;
> > +
> > +     offset = nh->frag_off & htons(IP_OFFSET);
> > +     if (offset) {
> > +             key->ip.frag = OVS_FRAG_TYPE_LATER;
> > +     } else {
> > +             if (nh->frag_off & htons(IP_MF) ||
> > +                             skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
> > +                     key->ip.frag = OVS_FRAG_TYPE_FIRST;
> > +             } else {
> > +                     key->ip.frag = OVS_FRAG_TYPE_NONE;
> > +             }
> > +     }
> > +     return ip_hdrlen(skb);
> > +}
> > +
> > +/**
> > +  * Parse ipv6 header from an Ethernet frame.
> > +  * Return ipv6 header length if successful, otherwise a negative errno
> value.
> > +  */
> > static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
> > {
> >       unsigned int nh_ofs = skb_network_offset(skb);
> > @@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb,
> struct sw_flow_key *key)
> >               else
> >                       key->ip.frag = OVS_FRAG_TYPE_FIRST;
> >       } else {
> > -             key->ip.frag = OVS_FRAG_TYPE_NONE;
> > +             if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> > +                     key->ip.frag = OVS_FRAG_TYPE_FIRST;
> > +             else
> > +                     key->ip.frag = OVS_FRAG_TYPE_NONE;
> >       }
> >
> >       /* Delayed handling of error in ipv6_skip_exthdr() as it
> > @@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> >       key->eth.type = skb->protocol;
> >
> >       /* Network layer. */
> > -     if (key->eth.type == htons(ETH_P_IP)) {
> > -             struct iphdr *nh;
> > -             __be16 offset;
> > +     switch(key->eth.type) {
> > +     case htons(ETH_P_IP):
> > +     case htons(ETH_P_IPV6): {
> > +             int nh_len;
> > +             if (key->eth.type == htons(ETH_P_IP)) {
> > +                     nh_len = parse_ipv4hdr(skb, key);
> > +             } else {
> > +                     nh_len = parse_ipv6hdr(skb, key);
> > +             }
> >
> > -             error = check_iphdr(skb);
> > -             if (unlikely(error)) {
> > -                     memset(&key->ip, 0, sizeof(key->ip));
> > -                     memset(&key->ipv4, 0, sizeof(key->ipv4));
> > -                     if (error == -EINVAL) {
> > +             if (unlikely(nh_len < 0)) {
> > +                     switch (nh_len) {
> > +                     case -EINVAL:
> > +                             memset(&key->ip, 0, sizeof(key->ip));
> > +                             if (key->eth.type == htons(ETH_P_IP)) {
> > +                                     memset(&key->ipv4.addr, 0,
> sizeof(key->ipv4.addr));
> > +                             } else {
> > +                                     memset(&key->ipv6.addr, 0,
> sizeof(key->ipv6.addr));
> > +                             }
> > +                             /* fall-through */
> > +                     case -EPROTO:
> >                               skb->transport_header =
> skb->network_header;
> >                               error = 0;
> > +                             break;
> > +                     default:
> > +                             error = nh_len;
> >                       }
> >                       return error;
> >               }
> >
> > -             nh = ip_hdr(skb);
> > -             key->ipv4.addr.src = nh->saddr;
> > -             key->ipv4.addr.dst = nh->daddr;
> > -
> > -             key->ip.proto = nh->protocol;
> > -             key->ip.tos = nh->tos;
> > -             key->ip.ttl = nh->ttl;
> > -
> > -             offset = nh->frag_off & htons(IP_OFFSET);
> > -             if (offset) {
> > -                     key->ip.frag = OVS_FRAG_TYPE_LATER;
> > +             if (key->ip.frag == OVS_FRAG_TYPE_LATER) {
> >                       return 0;
> >               }
> > -             if (nh->frag_off & htons(IP_MF) ||
> > -                     skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> > -                     key->ip.frag = OVS_FRAG_TYPE_FIRST;
> > -             else
> > -                     key->ip.frag = OVS_FRAG_TYPE_NONE;
> >
> >               /* Transport layer. */
> > -             if (key->ip.proto == IPPROTO_TCP) {
> > +             switch(key->ip.proto) {
> > +             case IPPROTO_TCP:
> >                       if (tcphdr_ok(skb)) {
> >                               struct tcphdr *tcp = tcp_hdr(skb);
> >                               key->tp.src = tcp->source;
> > @@ -605,8 +649,8 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> >                       } else {
> >                               memset(&key->tp, 0, sizeof(key->tp));
> >                       }
> > -
> > -             } else if (key->ip.proto == IPPROTO_UDP) {
> > +                     break;
> > +             case IPPROTO_UDP:
> >                       if (udphdr_ok(skb)) {
> >                               struct udphdr *udp = udp_hdr(skb);
> >                               key->tp.src = udp->source;
> > @@ -614,7 +658,8 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> >                       } else {
> >                               memset(&key->tp, 0, sizeof(key->tp));
> >                       }
> > -             } else if (key->ip.proto == IPPROTO_SCTP) {
> > +                     break;
> > +             case IPPROTO_SCTP:
> >                       if (sctphdr_ok(skb)) {
> >                               struct sctphdr *sctp = sctp_hdr(skb);
> >                               key->tp.src = sctp->source;
> > @@ -622,7 +667,8 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> >                       } else {
> >                               memset(&key->tp, 0, sizeof(key->tp));
> >                       }
> > -             } else if (key->ip.proto == IPPROTO_ICMP) {
> > +                     break;
> > +             case IPPROTO_ICMP:
> >                       if (icmphdr_ok(skb)) {
> >                               struct icmphdr *icmp = icmp_hdr(skb);
> >                               /* The ICMP type and code fields use the
> 16-bit
> > @@ -634,10 +680,23 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> >                       } else {
> >                               memset(&key->tp, 0, sizeof(key->tp));
> >                       }
> > +                     break;
> > +             case NEXTHDR_ICMP:
> > +                     if (icmp6hdr_ok(skb)) {
> > +                             error = parse_icmpv6(skb, key, nh_len);
> > +                             if (error)
> > +                                     return error;
> > +                     } else {
> > +                             memset(&key->tp, 0, sizeof(key->tp));
> > +                     }
> > +                     break;
> > +             default:
> > +                     break;
> >               }
> > -
> > -     } else if (key->eth.type == htons(ETH_P_ARP) ||
> > -                key->eth.type == htons(ETH_P_RARP)) {
> > +             break;
> > +     }
> > +     case htons(ETH_P_ARP):
> > +     case htons(ETH_P_RARP): {
> >               struct arp_eth_header *arp;
> >               bool arp_available = arphdr_ok(skb);
> >
> > @@ -663,94 +722,39 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> >                       memset(&key->ip, 0, sizeof(key->ip));
> >                       memset(&key->ipv4, 0, sizeof(key->ipv4));
> >               }
> > -     } else if (eth_p_mpls(key->eth.type)) {
> > -             size_t stack_len = MPLS_HLEN;
> > -
> > -             /* In the presence of an MPLS label stack the end of the L2
> > -              * header and the beginning of the L3 header differ.
> > -              *
> > -              * Advance network_header to the beginning of the L3
> > -              * header. mac_len corresponds to the end of the L2 header.
> > -              */
> > -             while (1) {
> > -                     __be32 lse;
> > +             break;
> > +     }
> > +     default:
> > +             if (eth_p_mpls(key->eth.type)) {
> > +                     size_t stack_len = MPLS_HLEN;
> > +
> > +                     /* In the presence of an MPLS label stack the end
> of the L2
> > +                      * header and the beginning of the L3 header
> differ.
> > +                      *
> > +                      * Advance network_header to the beginning of the
> L3
> > +                      * header. mac_len corresponds to the end of the
> L2 header.
> > +                      */
> > +                     while (1) {
> > +                             __be32 lse;
> >
> > -                     error = check_header(skb, skb->mac_len +
> stack_len);
> > -                     if (unlikely(error))
> > -                             return 0;
> > +                             error = check_header(skb, skb->mac_len +
> stack_len);
> > +                             if (unlikely(error))
> > +                                     return 0;
> >
> > -                     memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
> > +                             memcpy(&lse, skb_network_header(skb),
> MPLS_HLEN);
> >
> > -                     if (stack_len == MPLS_HLEN)
> > -                             memcpy(&key->mpls.top_lse, &lse,
> MPLS_HLEN);
> > +                             if (stack_len == MPLS_HLEN)
> > +                                     memcpy(&key->mpls.top_lse, &lse,
> MPLS_HLEN);
> >
> > -                     skb_set_network_header(skb, skb->mac_len +
> stack_len);
> > -                     if (lse & htonl(MPLS_LS_S_MASK))
> > -                             break;
> > +                             skb_set_network_header(skb, skb->mac_len +
> stack_len);
> > +                             if (lse & htonl(MPLS_LS_S_MASK))
> > +                                     break;
> >
> > -                     stack_len += MPLS_HLEN;
> > -             }
> > -     } else if (key->eth.type == htons(ETH_P_IPV6)) {
> > -             int nh_len;             /* IPv6 Header + Extensions */
> > -
> > -             nh_len = parse_ipv6hdr(skb, key);
> > -             if (unlikely(nh_len < 0)) {
> > -                     switch (nh_len) {
> > -                     case -EINVAL:
> > -                             memset(&key->ip, 0, sizeof(key->ip));
> > -                             memset(&key->ipv6.addr, 0,
> sizeof(key->ipv6.addr));
> > -                             /* fall-through */
> > -                     case -EPROTO:
> > -                             skb->transport_header =
> skb->network_header;
> > -                             error = 0;
> > -                             break;
> > -                     default:
> > -                             error = nh_len;
> > -                     }
> > -                     return error;
> > -             }
> > -
> > -             if (key->ip.frag == OVS_FRAG_TYPE_LATER)
> > -                     return 0;
> > -             if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> > -                     key->ip.frag = OVS_FRAG_TYPE_FIRST;
> > -
> > -             /* Transport layer. */
> > -             if (key->ip.proto == NEXTHDR_TCP) {
> > -                     if (tcphdr_ok(skb)) {
> > -                             struct tcphdr *tcp = tcp_hdr(skb);
> > -                             key->tp.src = tcp->source;
> > -                             key->tp.dst = tcp->dest;
> > -                             key->tp.flags = TCP_FLAGS_BE16(tcp);
> > -                     } else {
> > -                             memset(&key->tp, 0, sizeof(key->tp));
> > -                     }
> > -             } else if (key->ip.proto == NEXTHDR_UDP) {
> > -                     if (udphdr_ok(skb)) {
> > -                             struct udphdr *udp = udp_hdr(skb);
> > -                             key->tp.src = udp->source;
> > -                             key->tp.dst = udp->dest;
> > -                     } else {
> > -                             memset(&key->tp, 0, sizeof(key->tp));
> > -                     }
> > -             } else if (key->ip.proto == NEXTHDR_SCTP) {
> > -                     if (sctphdr_ok(skb)) {
> > -                             struct sctphdr *sctp = sctp_hdr(skb);
> > -                             key->tp.src = sctp->source;
> > -                             key->tp.dst = sctp->dest;
> > -                     } else {
> > -                             memset(&key->tp, 0, sizeof(key->tp));
> > -                     }
> > -             } else if (key->ip.proto == NEXTHDR_ICMP) {
> > -                     if (icmp6hdr_ok(skb)) {
> > -                             error = parse_icmpv6(skb, key, nh_len);
> > -                             if (error)
> > -                                     return error;
> > -                     } else {
> > -                             memset(&key->tp, 0, sizeof(key->tp));
> > +                             stack_len += MPLS_HLEN;
> >                       }
> >               }
> >       }
> > +
> >       return 0;
> > }
> >
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>

Patch hide | download patch | download mbox

diff --git a/datapath/flow.c b/datapath/flow.c
index 2bc1ad0..0b35de6 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -250,6 +250,46 @@  static bool icmphdr_ok(struct sk_buff *skb)
 				  sizeof(struct icmphdr));
 }
 
+/**
+  * Parse ipv4 header from an Ethernet frame.
+  * Return ipv4 header length if successful, otherwise a negative errno value.
+  */
+static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
+{
+	int err;
+	struct iphdr *nh;
+	__be16 offset;
+
+	err = check_iphdr(skb);
+	if (unlikely(err))
+		return err;
+
+	nh = ip_hdr(skb);
+	key->ipv4.addr.src = nh->saddr;
+	key->ipv4.addr.dst = nh->daddr;
+
+	key->ip.proto = nh->protocol;
+	key->ip.tos = nh->tos;
+	key->ip.ttl = nh->ttl;
+
+	offset = nh->frag_off & htons(IP_OFFSET);
+	if (offset) {
+		key->ip.frag = OVS_FRAG_TYPE_LATER;
+	} else {
+		if (nh->frag_off & htons(IP_MF) ||
+				skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
+			key->ip.frag = OVS_FRAG_TYPE_FIRST;
+		} else {
+			key->ip.frag = OVS_FRAG_TYPE_NONE;
+		}
+	}
+	return ip_hdrlen(skb);
+}
+
+/**
+  * Parse ipv6 header from an Ethernet frame.
+  * Return ipv6 header length if successful, otherwise a negative errno value.
+  */
 static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
 {
 	unsigned int nh_ofs = skb_network_offset(skb);
@@ -283,7 +323,10 @@  static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
 		else
 			key->ip.frag = OVS_FRAG_TYPE_FIRST;
 	} else {
-		key->ip.frag = OVS_FRAG_TYPE_NONE;
+		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
+			key->ip.frag = OVS_FRAG_TYPE_FIRST;
+		else
+			key->ip.frag = OVS_FRAG_TYPE_NONE;
 	}
 
 	/* Delayed handling of error in ipv6_skip_exthdr() as it
@@ -561,42 +604,43 @@  static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 	key->eth.type = skb->protocol;
 
 	/* Network layer. */
-	if (key->eth.type == htons(ETH_P_IP)) {
-		struct iphdr *nh;
-		__be16 offset;
+	switch(key->eth.type) {
+	case htons(ETH_P_IP):
+	case htons(ETH_P_IPV6): {
+		int nh_len;
+		if (key->eth.type == htons(ETH_P_IP)) {
+			nh_len = parse_ipv4hdr(skb, key);
+		} else {
+			nh_len = parse_ipv6hdr(skb, key);
+		}
 
-		error = check_iphdr(skb);
-		if (unlikely(error)) {
-			memset(&key->ip, 0, sizeof(key->ip));
-			memset(&key->ipv4, 0, sizeof(key->ipv4));
-			if (error == -EINVAL) {
+		if (unlikely(nh_len < 0)) {
+			switch (nh_len) {
+			case -EINVAL:
+				memset(&key->ip, 0, sizeof(key->ip));
+				if (key->eth.type == htons(ETH_P_IP)) {
+					memset(&key->ipv4.addr, 0, sizeof(key->ipv4.addr));
+				} else {
+					memset(&key->ipv6.addr, 0, sizeof(key->ipv6.addr));
+				}
+				/* fall-through */
+			case -EPROTO:
 				skb->transport_header = skb->network_header;
 				error = 0;
+				break;
+			default:
+				error = nh_len;
 			}
 			return error;
 		}
 
-		nh = ip_hdr(skb);
-		key->ipv4.addr.src = nh->saddr;
-		key->ipv4.addr.dst = nh->daddr;
-
-		key->ip.proto = nh->protocol;
-		key->ip.tos = nh->tos;
-		key->ip.ttl = nh->ttl;
-
-		offset = nh->frag_off & htons(IP_OFFSET);
-		if (offset) {
-			key->ip.frag = OVS_FRAG_TYPE_LATER;
+		if (key->ip.frag == OVS_FRAG_TYPE_LATER) {
 			return 0;
 		}
-		if (nh->frag_off & htons(IP_MF) ||
-			skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
-			key->ip.frag = OVS_FRAG_TYPE_FIRST;
-		else
-			key->ip.frag = OVS_FRAG_TYPE_NONE;
 
 		/* Transport layer. */
-		if (key->ip.proto == IPPROTO_TCP) {
+		switch(key->ip.proto) {
+		case IPPROTO_TCP:
 			if (tcphdr_ok(skb)) {
 				struct tcphdr *tcp = tcp_hdr(skb);
 				key->tp.src = tcp->source;
@@ -605,8 +649,8 @@  static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 			} else {
 				memset(&key->tp, 0, sizeof(key->tp));
 			}
-
-		} else if (key->ip.proto == IPPROTO_UDP) {
+			break;
+		case IPPROTO_UDP:
 			if (udphdr_ok(skb)) {
 				struct udphdr *udp = udp_hdr(skb);
 				key->tp.src = udp->source;
@@ -614,7 +658,8 @@  static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 			} else {
 				memset(&key->tp, 0, sizeof(key->tp));
 			}
-		} else if (key->ip.proto == IPPROTO_SCTP) {
+			break;
+		case IPPROTO_SCTP:
 			if (sctphdr_ok(skb)) {
 				struct sctphdr *sctp = sctp_hdr(skb);
 				key->tp.src = sctp->source;
@@ -622,7 +667,8 @@  static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 			} else {
 				memset(&key->tp, 0, sizeof(key->tp));
 			}
-		} else if (key->ip.proto == IPPROTO_ICMP) {
+			break;
+		case IPPROTO_ICMP:
 			if (icmphdr_ok(skb)) {
 				struct icmphdr *icmp = icmp_hdr(skb);
 				/* The ICMP type and code fields use the 16-bit
@@ -634,10 +680,23 @@  static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 			} else {
 				memset(&key->tp, 0, sizeof(key->tp));
 			}
+			break;
+		case NEXTHDR_ICMP:
+			if (icmp6hdr_ok(skb)) {
+				error = parse_icmpv6(skb, key, nh_len);
+				if (error)
+					return error;
+			} else {
+				memset(&key->tp, 0, sizeof(key->tp));
+			}
+			break;
+		default:
+			break;
 		}
-
-	} else if (key->eth.type == htons(ETH_P_ARP) ||
-		   key->eth.type == htons(ETH_P_RARP)) {
+		break;
+	}
+	case htons(ETH_P_ARP):
+	case htons(ETH_P_RARP): {
 		struct arp_eth_header *arp;
 		bool arp_available = arphdr_ok(skb);
 
@@ -663,94 +722,39 @@  static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 			memset(&key->ip, 0, sizeof(key->ip));
 			memset(&key->ipv4, 0, sizeof(key->ipv4));
 		}
-	} else if (eth_p_mpls(key->eth.type)) {
-		size_t stack_len = MPLS_HLEN;
-
-		/* In the presence of an MPLS label stack the end of the L2
-		 * header and the beginning of the L3 header differ.
-		 *
-		 * Advance network_header to the beginning of the L3
-		 * header. mac_len corresponds to the end of the L2 header.
-		 */
-		while (1) {
-			__be32 lse;
+		break;
+	}
+	default:
+		if (eth_p_mpls(key->eth.type)) {
+			size_t stack_len = MPLS_HLEN;
+
+			/* In the presence of an MPLS label stack the end of the L2
+			 * header and the beginning of the L3 header differ.
+			 *
+			 * Advance network_header to the beginning of the L3
+			 * header. mac_len corresponds to the end of the L2 header.
+			 */
+			while (1) {
+				__be32 lse;
 
-			error = check_header(skb, skb->mac_len + stack_len);
-			if (unlikely(error))
-				return 0;
+				error = check_header(skb, skb->mac_len + stack_len);
+				if (unlikely(error))
+					return 0;
 
-			memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
+				memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
 
-			if (stack_len == MPLS_HLEN)
-				memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
+				if (stack_len == MPLS_HLEN)
+					memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
 
-			skb_set_network_header(skb, skb->mac_len + stack_len);
-			if (lse & htonl(MPLS_LS_S_MASK))
-				break;
+				skb_set_network_header(skb, skb->mac_len + stack_len);
+				if (lse & htonl(MPLS_LS_S_MASK))
+					break;
 
-			stack_len += MPLS_HLEN;
-		}
-	} else if (key->eth.type == htons(ETH_P_IPV6)) {
-		int nh_len;             /* IPv6 Header + Extensions */
-
-		nh_len = parse_ipv6hdr(skb, key);
-		if (unlikely(nh_len < 0)) {
-			switch (nh_len) {
-			case -EINVAL:
-				memset(&key->ip, 0, sizeof(key->ip));
-				memset(&key->ipv6.addr, 0, sizeof(key->ipv6.addr));
-				/* fall-through */
-			case -EPROTO:
-				skb->transport_header = skb->network_header;
-				error = 0;
-				break;
-			default:
-				error = nh_len;
-			}
-			return error;
-		}
-
-		if (key->ip.frag == OVS_FRAG_TYPE_LATER)
-			return 0;
-		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
-			key->ip.frag = OVS_FRAG_TYPE_FIRST;
-
-		/* Transport layer. */
-		if (key->ip.proto == NEXTHDR_TCP) {
-			if (tcphdr_ok(skb)) {
-				struct tcphdr *tcp = tcp_hdr(skb);
-				key->tp.src = tcp->source;
-				key->tp.dst = tcp->dest;
-				key->tp.flags = TCP_FLAGS_BE16(tcp);
-			} else {
-				memset(&key->tp, 0, sizeof(key->tp));
-			}
-		} else if (key->ip.proto == NEXTHDR_UDP) {
-			if (udphdr_ok(skb)) {
-				struct udphdr *udp = udp_hdr(skb);
-				key->tp.src = udp->source;
-				key->tp.dst = udp->dest;
-			} else {
-				memset(&key->tp, 0, sizeof(key->tp));
-			}
-		} else if (key->ip.proto == NEXTHDR_SCTP) {
-			if (sctphdr_ok(skb)) {
-				struct sctphdr *sctp = sctp_hdr(skb);
-				key->tp.src = sctp->source;
-				key->tp.dst = sctp->dest;
-			} else {
-				memset(&key->tp, 0, sizeof(key->tp));
-			}
-		} else if (key->ip.proto == NEXTHDR_ICMP) {
-			if (icmp6hdr_ok(skb)) {
-				error = parse_icmpv6(skb, key, nh_len);
-				if (error)
-					return error;
-			} else {
-				memset(&key->tp, 0, sizeof(key->tp));
+				stack_len += MPLS_HLEN;
 			}
 		}
 	}
+
 	return 0;
 }