| Message ID | 1492759292-31031-1-git-send-email-sysugaozhenyu@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
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; > } >
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
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 > >
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. 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(-)