You may want to combine the two patches into one. It is not good to have is_zero mask check scattered over. This will hard to maintain.
On Tue, Aug 13, 2013 at 2:50 PM, <[email protected]> wrote: > From: gyang <gyang@gyang-virtual-machine.(none)> > > With megaflow support, there is API to convert mask to nlattr key based > format. This change introduces API to do the reverse conversion. We > leverage the existing odp_flow_key_to_flow() API to resue the code. > > Signed-off-by: Guolin Yang <[email protected]> > --- > lib/odp-util.c | 257 > +++++++++++++++++++++++++++++++++++++++----------------- > lib/odp-util.h | 2 + > 2 files changed, 184 insertions(+), 75 deletions(-) > > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 78d5a1b..3a1b55c 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -2680,20 +2680,30 @@ check_expectations(uint64_t present_attrs, int > out_of_range_attr, > static bool > parse_ethertype(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], > uint64_t present_attrs, uint64_t *expected_attrs, > - struct flow *flow) > + struct flow *flow, struct flow *src_flow) > { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + bool is_mask = flow != src_flow; > > if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE)) { > flow->dl_type = nl_attr_get_be16(attrs[OVS_KEY_ATTR_ETHERTYPE]); > - if (ntohs(flow->dl_type) < 1536) { > + if (!is_mask && ntohs(flow->dl_type) < 1536) { > Is possible not to hard code 1536? > VLOG_ERR_RL(&rl, "invalid Ethertype %"PRIu16" in flow key", > ntohs(flow->dl_type)); > return false; > } > *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE; > } else { > - flow->dl_type = htons(FLOW_DL_TYPE_NONE); > + if (!is_mask) { > + flow->dl_type = htons(FLOW_DL_TYPE_NONE); > + } else if (ntohs(src_flow->dl_type) < 1536) { > if mask is exact match, we should accept dl_type to be 802.2. > + /* > + * see comments odp_flow_key_from_flow__ > + */ > + VLOG_ERR_RL(&rl, "Alway expect mask for non ethernet II " > + "frame"); > + return false; > + } > } > return true; > } > @@ -2702,38 +2712,63 @@ static enum odp_key_fitness > parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], > uint64_t present_attrs, int out_of_range_attr, > uint64_t expected_attrs, struct flow *flow, > - const struct nlattr *key, size_t key_len) > + const struct nlattr *key, size_t key_len, > + struct flow *src_flow) > { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > - > - if (eth_type_mpls(flow->dl_type)) { > - expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_MPLS); > - > - if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS))) { > - return ODP_FIT_TOO_LITTLE; > + bool is_mask = src_flow != flow; > + > + if (is_mask && flow->dl_type != 0xffff) { > + goto done; > + } > + if (eth_type_mpls(src_flow->dl_type)) { > + if (!is_mask) { > + expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_MPLS); > + > + if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS))) { > + return ODP_FIT_TOO_LITTLE; > Can we combine this check to the end, and even for mask? By the time all netlink attributes are processed expected_attrs == present_attrs should always to be true, right? > + } > + flow->mpls_lse = nl_attr_get_be32(attrs[OVS_KEY_ATTR_MPLS]); > + flow->mpls_depth++; > + } else if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS)) { > + expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_MPLS); > + flow->mpls_lse = nl_attr_get_be32(attrs[OVS_KEY_ATTR_MPLS]); > + /* > + * do we need to set the following? XXX > + */ > + flow->mpls_depth = 0xffff; > + } > + } else if (src_flow->dl_type == htons(ETH_TYPE_IP)) { > + if (!is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV4; > } > - flow->mpls_lse = nl_attr_get_be32(attrs[OVS_KEY_ATTR_MPLS]); > - flow->mpls_depth++; > - } else if (flow->dl_type == htons(ETH_TYPE_IP)) { > - expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV4; > if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_IPV4)) { > const struct ovs_key_ipv4 *ipv4_key; > > + if (is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV4; > + } > + > ipv4_key = nl_attr_get(attrs[OVS_KEY_ATTR_IPV4]); > flow->nw_src = ipv4_key->ipv4_src; > flow->nw_dst = ipv4_key->ipv4_dst; > flow->nw_proto = ipv4_key->ipv4_proto; > flow->nw_tos = ipv4_key->ipv4_tos; > flow->nw_ttl = ipv4_key->ipv4_ttl; > - if (!odp_to_ovs_frag(ipv4_key->ipv4_frag, flow)) { > + if (!is_mask && !odp_to_ovs_frag(ipv4_key->ipv4_frag, flow)) { > if is_mask, should we still set the frag mask bits? > return ODP_FIT_ERROR; > } > } > - } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) { > - expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV6; > + } else if (src_flow->dl_type == htons(ETH_TYPE_IPV6)) { > + if (!is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV6; > + } > if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_IPV6)) { > const struct ovs_key_ipv6 *ipv6_key; > > + if (is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV6; > + } > ipv6_key = nl_attr_get(attrs[OVS_KEY_ATTR_IPV6]); > memcpy(&flow->ipv6_src, ipv6_key->ipv6_src, sizeof > flow->ipv6_src); > memcpy(&flow->ipv6_dst, ipv6_key->ipv6_dst, sizeof > flow->ipv6_dst); > @@ -2741,20 +2776,25 @@ parse_l2_5_onward(const struct nlattr > *attrs[OVS_KEY_ATTR_MAX + 1], > flow->nw_proto = ipv6_key->ipv6_proto; > flow->nw_tos = ipv6_key->ipv6_tclass; > flow->nw_ttl = ipv6_key->ipv6_hlimit; > - if (!odp_to_ovs_frag(ipv6_key->ipv6_frag, flow)) { > + if (!is_mask && !odp_to_ovs_frag(ipv6_key->ipv6_frag, flow)) { > same comment as above. > return ODP_FIT_ERROR; > } > } > - } else if (flow->dl_type == htons(ETH_TYPE_ARP) || > - flow->dl_type == htons(ETH_TYPE_RARP)) { > - expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ARP; > + } else if (src_flow->dl_type == htons(ETH_TYPE_ARP) || > + src_flow->dl_type == htons(ETH_TYPE_RARP)) { > + if (!is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ARP; > + } > if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ARP)) { > const struct ovs_key_arp *arp_key; > > + if (is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ARP; > + } > arp_key = nl_attr_get(attrs[OVS_KEY_ATTR_ARP]); > flow->nw_src = arp_key->arp_sip; > flow->nw_dst = arp_key->arp_tip; > - if (arp_key->arp_op & htons(0xff00)) { > + if (!is_mask && (arp_key->arp_op & htons(0xff00))) { > VLOG_ERR_RL(&rl, "unsupported ARP opcode %"PRIu16" in > flow " > "key", ntohs(arp_key->arp_op)); > return ODP_FIT_ERROR; > @@ -2765,58 +2805,86 @@ parse_l2_5_onward(const struct nlattr > *attrs[OVS_KEY_ATTR_MAX + 1], > } > } > > - if (flow->nw_proto == IPPROTO_TCP > - && (flow->dl_type == htons(ETH_TYPE_IP) || > - flow->dl_type == htons(ETH_TYPE_IPV6)) > - && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) { > - expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_TCP; > + if (is_mask && flow->nw_proto != 0xff) { > + goto done; > + } > + if (src_flow->nw_proto == IPPROTO_TCP > + && (src_flow->dl_type == htons(ETH_TYPE_IP) || > + src_flow->dl_type == htons(ETH_TYPE_IPV6)) > + && !(src_flow->nw_frag & FLOW_NW_FRAG_LATER)) { > + if (!is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_TCP; > + } > if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_TCP)) { > const struct ovs_key_tcp *tcp_key; > > + if (is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_TCP; > + } > tcp_key = nl_attr_get(attrs[OVS_KEY_ATTR_TCP]); > flow->tp_src = tcp_key->tcp_src; > flow->tp_dst = tcp_key->tcp_dst; > } > - } else if (flow->nw_proto == IPPROTO_UDP > - && (flow->dl_type == htons(ETH_TYPE_IP) || > - flow->dl_type == htons(ETH_TYPE_IPV6)) > - && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) { > - expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_UDP; > + } else if (src_flow->nw_proto == IPPROTO_UDP > + && (src_flow->dl_type == htons(ETH_TYPE_IP) || > + src_flow->dl_type == htons(ETH_TYPE_IPV6)) > + && !(src_flow->nw_frag & FLOW_NW_FRAG_LATER)) { > + if (!is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_UDP; > + } > if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_UDP)) { > const struct ovs_key_udp *udp_key; > > + if (is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_UDP; > + } > udp_key = nl_attr_get(attrs[OVS_KEY_ATTR_UDP]); > flow->tp_src = udp_key->udp_src; > flow->tp_dst = udp_key->udp_dst; > } > - } else if (flow->nw_proto == IPPROTO_ICMP > - && flow->dl_type == htons(ETH_TYPE_IP) - && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) { > - expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ICMP; > + } else if (src_flow->nw_proto == IPPROTO_ICMP > + && src_flow->dl_type == htons(ETH_TYPE_IP) > + && !(src_flow->nw_frag & FLOW_NW_FRAG_LATER)) { > + if (!is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ICMP; > + } > if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ICMP)) { > const struct ovs_key_icmp *icmp_key; > > + if (is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ICMP; > + } > icmp_key = nl_attr_get(attrs[OVS_KEY_ATTR_ICMP]); > flow->tp_src = htons(icmp_key->icmp_type); > flow->tp_dst = htons(icmp_key->icmp_code); > } > - } else if (flow->nw_proto == IPPROTO_ICMPV6 > - && flow->dl_type == htons(ETH_TYPE_IPV6) > - && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) { > - expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ICMPV6; > + } else if (src_flow->nw_proto == IPPROTO_ICMPV6 > + && src_flow->dl_type == htons(ETH_TYPE_IPV6) > + && !(src_flow->nw_frag & FLOW_NW_FRAG_LATER)) { > + if (!is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ICMPV6; > + } > if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ICMPV6)) { > const struct ovs_key_icmpv6 *icmpv6_key; > > + if (is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ICMPV6; > + } > icmpv6_key = nl_attr_get(attrs[OVS_KEY_ATTR_ICMPV6]); > flow->tp_src = htons(icmpv6_key->icmpv6_type); > flow->tp_dst = htons(icmpv6_key->icmpv6_code); > > - if (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT) || > - flow->tp_src == htons(ND_NEIGHBOR_ADVERT)) { > - expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ND; > + if (src_flow->tp_src == htons(ND_NEIGHBOR_SOLICIT) || > + src_flow->tp_src == htons(ND_NEIGHBOR_ADVERT)) { > + if (!is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ND; > + } > if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ND)) { > const struct ovs_key_nd *nd_key; > > + if (is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ND; > + } > nd_key = nl_attr_get(attrs[OVS_KEY_ATTR_ND]); > memcpy(&flow->nd_target, nd_key->nd_target, > sizeof flow->nd_target); > @@ -2826,7 +2894,7 @@ parse_l2_5_onward(const struct nlattr > *attrs[OVS_KEY_ATTR_MAX + 1], > } > } > } > - > +done: > return check_expectations(present_attrs, out_of_range_attr, > expected_attrs, > key, key_len); > } > @@ -2836,9 +2904,11 @@ static enum odp_key_fitness > parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], > uint64_t present_attrs, int out_of_range_attr, > uint64_t expected_attrs, struct flow *flow, > - const struct nlattr *key, size_t key_len) > + const struct nlattr *key, size_t key_len, > + struct flow *src_flow) > { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + bool is_mask = src_flow != flow; > > const struct nlattr *encap > = (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ENCAP) > @@ -2848,33 +2918,49 @@ parse_8021q_onward(const struct nlattr > *attrs[OVS_KEY_ATTR_MAX + 1], > ovs_be16 tci; > > /* Calculate fitness of outer attributes. */ > - expected_attrs |= ((UINT64_C(1) << OVS_KEY_ATTR_VLAN) | > - (UINT64_C(1) << OVS_KEY_ATTR_ENCAP)); > + if (!is_mask) { > + expected_attrs |= ((UINT64_C(1) << OVS_KEY_ATTR_VLAN) | > + (UINT64_C(1) << OVS_KEY_ATTR_ENCAP)); > + } else { > + if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN)) { > + expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_VLAN); > + } > + if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ENCAP)) { > + expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_ENCAP); > + } > + } > fitness = check_expectations(present_attrs, out_of_range_attr, > expected_attrs, key, key_len); > > /* Get the VLAN TCI value. */ > - if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN))) { > + if (!is_mask && !(present_attrs & (UINT64_C(1) << > OVS_KEY_ATTR_VLAN))) { > return ODP_FIT_TOO_LITTLE; > } > - tci = nl_attr_get_be16(attrs[OVS_KEY_ATTR_VLAN]); > - if (tci == htons(0)) { > - /* Corner case for a truncated 802.1Q header. */ > - if (fitness == ODP_FIT_PERFECT && nl_attr_get_size(encap)) { > - return ODP_FIT_TOO_MUCH; > - } > + if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN)) { > + tci = nl_attr_get_be16(attrs[OVS_KEY_ATTR_VLAN]); > + if (!is_mask) { > + if (tci == htons(0)) { > + /* Corner case for a truncated 802.1Q header. */ > + if (fitness == ODP_FIT_PERFECT && nl_attr_get_size(encap)) > { > + return ODP_FIT_TOO_MUCH; > + } > + return fitness; > + } else if (!(tci & htons(VLAN_CFI))) { > + VLOG_ERR_RL(&rl, "OVS_KEY_ATTR_VLAN 0x%04"PRIx16" is > nonzero " > + "but CFI bit is not set", ntohs(tci)); > + return ODP_FIT_ERROR; > + } > + } > + /* Set vlan_tci. > + * Remove the TPID from dl_type since it's not the real > Ethertype. */ > + flow->vlan_tci = tci; > + /* for mask, shall we set this to zero? */ > + flow->dl_type = htons(0); > + } > Does this handle the case VLAN attribute is missing from the mask attribute? > + > + if (is_mask && !(present_attrs & (UINT64_C(1) << > OVS_KEY_ATTR_ENCAP))) { > return fitness; > - } else if (!(tci & htons(VLAN_CFI))) { > - VLOG_ERR_RL(&rl, "OVS_KEY_ATTR_VLAN 0x%04"PRIx16" is nonzero " > - "but CFI bit is not set", ntohs(tci)); > - return ODP_FIT_ERROR; > } > - > - /* Set vlan_tci. > - * Remove the TPID from dl_type since it's not the real Ethertype. */ > - flow->vlan_tci = tci; > - flow->dl_type = htons(0); > - > /* Now parse the encapsulated attributes. */ > if (!parse_flow_nlattrs(nl_attr_get(encap), nl_attr_get_size(encap), > attrs, &present_attrs, &out_of_range_attr)) { > @@ -2882,11 +2968,12 @@ parse_8021q_onward(const struct nlattr > *attrs[OVS_KEY_ATTR_MAX + 1], > } > expected_attrs = 0; > > - if (!parse_ethertype(attrs, present_attrs, &expected_attrs, flow)) { > + if (!parse_ethertype(attrs, present_attrs, &expected_attrs, flow, > src_flow)) { > return ODP_FIT_ERROR; > } > encap_fitness = parse_l2_5_onward(attrs, present_attrs, > out_of_range_attr, > - expected_attrs, flow, key, key_len); > + expected_attrs, flow, key, key_len, > + src_flow); > > /* The overall fitness is the worse of the outer and inner > attributes. */ > return MAX(fitness, encap_fitness); > @@ -2907,14 +2994,15 @@ parse_8021q_onward(const struct nlattr > *attrs[OVS_KEY_ATTR_MAX + 1], > * protocol in OVS_KEY_ATTR_IPV4 or OVS_KEY_ATTR_IPV6 is IPPROTO_TCP then > we > * know that a OVS_KEY_ATTR_TCP attribute must appear and that otherwise > it > * must be absent. */ > -enum odp_key_fitness > -odp_flow_key_to_flow(const struct nlattr *key, size_t key_len, > - struct flow *flow) > +static enum odp_key_fitness > +odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len, > + struct flow *flow, struct flow *src_flow) > { > const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1]; > uint64_t expected_attrs; > uint64_t present_attrs; > int out_of_range_attr; > + bool is_mask = src_flow != flow; > > memset(flow, 0, sizeof *flow); > > @@ -2951,7 +3039,7 @@ odp_flow_key_to_flow(const struct nlattr *key, > size_t key_len, > flow->in_port.odp_port > = nl_attr_get_odp_port(attrs[OVS_KEY_ATTR_IN_PORT]); > expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IN_PORT; > - } else { > + } else if (!is_mask) { > flow->in_port.odp_port = ODPP_NONE; > } > > @@ -2962,20 +3050,39 @@ odp_flow_key_to_flow(const struct nlattr *key, > size_t key_len, > eth_key = nl_attr_get(attrs[OVS_KEY_ATTR_ETHERNET]); > memcpy(flow->dl_src, eth_key->eth_src, ETH_ADDR_LEN); > memcpy(flow->dl_dst, eth_key->eth_dst, ETH_ADDR_LEN); > + if (is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERNET; > + } > + } > + if (!is_mask) { > + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERNET; > } > - expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERNET; > > /* Get Ethertype or 802.1Q TPID or FLOW_DL_TYPE_NONE. */ > - if (!parse_ethertype(attrs, present_attrs, &expected_attrs, flow)) { > + if (!parse_ethertype(attrs, present_attrs, &expected_attrs, flow, > src_flow)) { > return ODP_FIT_ERROR; > } > > - if (flow->dl_type == htons(ETH_TYPE_VLAN)) { > + if (src_flow->dl_type == htons(ETH_TYPE_VLAN)) { > return parse_8021q_onward(attrs, present_attrs, out_of_range_attr, > - expected_attrs, flow, key, key_len); > + expected_attrs, flow, key, key_len, > src_flow); > } > return parse_l2_5_onward(attrs, present_attrs, out_of_range_attr, > - expected_attrs, flow, key, key_len); > + expected_attrs, flow, key, key_len, > src_flow); > +} > + > +enum odp_key_fitness > +odp_flow_key_to_flow(const struct nlattr *key, size_t key_len, > + struct flow *flow) > +{ > + return odp_flow_key_to_flow__(key, key_len, flow, flow); > +} > + > +enum odp_key_fitness > +odp_flow_key_to_mask(const struct nlattr *key, size_t key_len, > + struct flow *mask, struct flow *flow) > +{ > + return odp_flow_key_to_flow__(key, key_len, mask, flow); > } > > /* Returns 'fitness' as a string, for use in debug messages. */ > diff --git a/lib/odp-util.h b/lib/odp-util.h > index 7e27888..2ac5e19 100644 > --- a/lib/odp-util.h > +++ b/lib/odp-util.h > @@ -122,6 +122,8 @@ enum odp_key_fitness { > }; > enum odp_key_fitness odp_flow_key_to_flow(const struct nlattr *, size_t, > struct flow *); > +enum odp_key_fitness odp_flow_key_to_mask(const struct nlattr *key, > size_t len, > + struct flow *mask, struct flow > *flow); > const char *odp_key_fitness_to_string(enum odp_key_fitness); > > void commit_odp_tunnel_action(const struct flow *, struct flow *base, > -- > 1.7.9.5 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev >
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
