| Message ID | 1452606524-84399-1-git-send-email-sugesh.chandran@intel.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi, We found that there is a serious performance drop in OVS userspace-datapath after the commit "tunneling: add IPv6 support to netdev_tunnel_config(commit ID :- 3ae91c019019889fbe93aa7dfd6e3747da362b24)" in the OVS tree. The PHY-PHY throughput dropped almost 25% for 64 byte packets due to this check-in. The following patch is for fixing it. Let me know your comments/suggestion on it , if any?? Regards _Sugesh > -----Original Message----- > From: Chandran, Sugesh > Sent: Tuesday, January 12, 2016 1:49 PM > To: dev@openvswitch.org; cascardo@redhat.com; jbenc@redhat.com > Cc: Chandran, Sugesh <sugesh.chandran@intel.com> > Subject: [PATCH] ipv6 tunneling: Fix for performance drop introduced by ipv6 > tunnel support. > > Adding a new field called protocol in flow tunnel structure to verify the > validity of tunnel metadata. This field avoids the need of resetting and > validating the entire ipv4/ipv6 tunnel destination address which caused a > serious performance drop. > > Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com> > --- > lib/flow.c | 8 +++---- > lib/match.c | 12 +++++------ > lib/meta-flow.c | 11 ++++++++-- > lib/netdev-vport.c | 5 ++--- > lib/odp-util.c | 10 ++++----- > lib/packets.c | 8 ++++++- > lib/packets.h | 50 ++++++++++++++++++++++++++++++++++++++- > ----- > ofproto/ofproto-dpif-ipfix.c | 3 ++- > ofproto/ofproto-dpif-rid.c | 3 +-- > ofproto/ofproto-dpif-sflow.c | 6 +++--- > ofproto/tunnel.c | 14 ++++++++----- > 11 files changed, 92 insertions(+), 38 deletions(-) > > diff --git a/lib/flow.c b/lib/flow.c > index 5668d0c..34cee55 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -818,15 +818,15 @@ flow_get_metadata(const struct flow *flow, struct > match *flow_metadata) > if (flow->tunnel.ip_src) { > match_set_tun_src(flow_metadata, flow->tunnel.ip_src); > } > - if (flow->tunnel.ip_dst) { > + if (flow->tunnel.protocol == FLOW_TUNNEL_IPV4) { > match_set_tun_dst(flow_metadata, flow->tunnel.ip_dst); > } > + else if (flow->tunnel.protocol == FLOW_TUNNEL_IPV6) { > + match_set_tun_ipv6_dst(flow_metadata, &flow->tunnel.ipv6_dst); > + } > if (ipv6_addr_is_set(&flow->tunnel.ipv6_src)) { > match_set_tun_ipv6_src(flow_metadata, &flow->tunnel.ipv6_src); > } > - if (ipv6_addr_is_set(&flow->tunnel.ipv6_dst)) { > - match_set_tun_ipv6_dst(flow_metadata, &flow->tunnel.ipv6_dst); > - } > if (flow->tunnel.gbp_id != htons(0)) { > match_set_tun_gbp_id(flow_metadata, flow->tunnel.gbp_id); > } > diff --git a/lib/match.c b/lib/match.c > index 95d34bc..97a623e 100644 > --- a/lib/match.c > +++ b/lib/match.c > @@ -186,8 +186,8 @@ match_set_tun_dst(struct match *match, ovs_be32 > dst) void match_set_tun_dst_masked(struct match *match, ovs_be32 dst, > ovs_be32 mask) { > - match->wc.masks.tunnel.ip_dst = mask; > - match->flow.tunnel.ip_dst = dst & mask; > + set_ipv4_dst_tnl(&match->wc.masks.tunnel, mask); > + set_ipv4_dst_tnl(&match->flow.tunnel, dst & mask); > } > > void > @@ -208,16 +208,16 @@ match_set_tun_ipv6_src_masked(struct match > *match, const struct in6_addr *src, void match_set_tun_ipv6_dst(struct > match *match, const struct in6_addr *dst) { > - match->flow.tunnel.ipv6_dst = *dst; > - match->wc.masks.tunnel.ipv6_dst = in6addr_exact; > + set_ipv6_dst_tnl(&match->flow.tunnel, *dst); > + set_ipv6_dst_tnl(&match->wc.masks.tunnel, in6addr_exact); > } > > void > match_set_tun_ipv6_dst_masked(struct match *match, const struct > in6_addr *dst, > const struct in6_addr *mask) { > - match->flow.tunnel.ipv6_dst = ipv6_addr_bitand(dst, mask); > - match->wc.masks.tunnel.ipv6_dst = *mask; > + set_ipv6_dst_tnl(&match->flow.tunnel, ipv6_addr_bitand(dst, mask)); > + set_ipv6_dst_tnl(&match->wc.masks.tunnel, *mask); > } > > void > diff --git a/lib/meta-flow.c b/lib/meta-flow.c index 6bd0b99..0826d1d 100644 > --- a/lib/meta-flow.c > +++ b/lib/meta-flow.c > @@ -1180,13 +1180,13 @@ mf_set_flow_value(const struct mf_field *mf, > flow->tunnel.ip_src = value->be32; > break; > case MFF_TUN_DST: > - flow->tunnel.ip_dst = value->be32; > + set_ipv4_dst_tnl(&flow->tunnel, value->be32); > break; > case MFF_TUN_IPV6_SRC: > flow->tunnel.ipv6_src = value->ipv6; > break; > case MFF_TUN_IPV6_DST: > - flow->tunnel.ipv6_dst = value->ipv6; > + set_ipv6_dst_tnl(&flow->tunnel, value->ipv6); > break; > case MFF_TUN_FLAGS: > flow->tunnel.flags = (flow->tunnel.flags & ~FLOW_TNL_PUB_F_MASK) | > @@ -1503,6 +1503,13 @@ mf_set_wild(const struct mf_field *mf, struct > match *match, char **err_str) > sizeof match->wc.masks.tunnel.ipv6_dst); > memset(&match->flow.tunnel.ipv6_dst, 0, > sizeof match->flow.tunnel.ipv6_dst); > + /* What if flow have a valid ipv4 tunnel address?? > + * Reset the flag only if thats not the case. > + */ > + match->wc.masks.tunnel.protocol = (match->wc.masks.tunnel.protocol > == > + FLOW_TUNNEL_IPV4)? > + match->wc.masks.tunnel.protocol: > + FLOW_TUNNEL_UNSPEC; > break; > case MFF_TUN_FLAGS: > match_set_tun_flags_masked(match, 0, 0); diff --git a/lib/netdev- > vport.c b/lib/netdev-vport.c index 88f5022..d3477c3 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -919,19 +919,18 @@ ip_extract_tnl_md(struct dp_packet *packet, > struct flow_tnl *tnl, > ip_dst = get_16aligned_be32(&ip->ip_dst); > > tnl->ip_src = ip_src; > - tnl->ip_dst = ip_dst; > tnl->ip_tos = ip->ip_tos; > tnl->ip_ttl = ip->ip_ttl; > + set_ipv4_dst_tnl(tnl, ip_dst); > > *hlen += IP_HEADER_LEN; > > } else if (IP_VER(ip->ip_ihl_ver) == 6) { > > memcpy(tnl->ipv6_src.s6_addr, ip6->ip6_src.be16, sizeof ip6->ip6_src); > - memcpy(tnl->ipv6_dst.s6_addr, ip6->ip6_dst.be16, sizeof ip6->ip6_dst); > tnl->ip_tos = 0; > tnl->ip_ttl = ip6->ip6_hlim; > - > + set_ipv6_dst_tnl(tnl, *(struct in6_addr *)&ip6->ip6_dst); > *hlen += IPV6_HEADER_LEN; > > } else { > diff --git a/lib/odp-util.c b/lib/odp-util.c index f16e113..26c3727 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -510,7 +510,7 @@ format_odp_tnl_push_header(struct ds *ds, struct > ovs_action_push_tnl *data) > gnh->oam ? "oam," : "", > gnh->critical ? "crit," : "", > ntohl(get_16aligned_be32(&gnh->vni)) >> 8); > - > + > if (gnh->opt_len) { > ds_put_cstr(ds, ",options("); > format_geneve_opts(gnh->options, NULL, gnh->opt_len * 4, @@ - > 1864,13 +1864,13 @@ odp_tun_key_from_attr__(const struct nlattr *attr, > tun->ip_src = nl_attr_get_be32(a); > break; > case OVS_TUNNEL_KEY_ATTR_IPV4_DST: > - tun->ip_dst = nl_attr_get_be32(a); > + set_ipv4_dst_tnl(tun, nl_attr_get_be32(a)); > break; > case OVS_TUNNEL_KEY_ATTR_IPV6_SRC: > tun->ipv6_src = nl_attr_get_in6_addr(a); > break; > case OVS_TUNNEL_KEY_ATTR_IPV6_DST: > - tun->ipv6_dst = nl_attr_get_in6_addr(a); > + set_ipv6_dst_tnl(tun, nl_attr_get_in6_addr(a)); > break; > case OVS_TUNNEL_KEY_ATTR_TOS: > tun->ip_tos = nl_attr_get_u8(a); @@ -1961,13 +1961,13 @@ > tun_key_to_attr(struct ofpbuf *a, const struct flow_tnl *tun_key, > if (tun_key->ip_src) { > nl_msg_put_be32(a, OVS_TUNNEL_KEY_ATTR_IPV4_SRC, tun_key- > >ip_src); > } > - if (tun_key->ip_dst) { > + if (tun_key->protocol == FLOW_TUNNEL_IPV4) { > nl_msg_put_be32(a, OVS_TUNNEL_KEY_ATTR_IPV4_DST, tun_key- > >ip_dst); > } > if (ipv6_addr_is_set(&tun_key->ipv6_src)) { > nl_msg_put_in6_addr(a, OVS_TUNNEL_KEY_ATTR_IPV6_SRC, > &tun_key->ipv6_src); > } > - if (ipv6_addr_is_set(&tun_key->ipv6_dst)) { > + if (tun_key->protocol == FLOW_TUNNEL_IPV6) { > nl_msg_put_in6_addr(a, OVS_TUNNEL_KEY_ATTR_IPV6_DST, > &tun_key->ipv6_dst); > } > if (tun_key->ip_tos) { > diff --git a/lib/packets.c b/lib/packets.c index d82341d..ed6c5ad 100644 > --- a/lib/packets.c > +++ b/lib/packets.c > @@ -39,7 +39,13 @@ const struct in6_addr in6addr_all_hosts = > IN6ADDR_ALL_HOSTS_INIT; struct in6_addr flow_tnl_dst(const struct > flow_tnl *tnl) { > - return tnl->ip_dst ? in6_addr_mapped_ipv4(tnl->ip_dst) : tnl->ipv6_dst; > + if(tnl->protocol == FLOW_TUNNEL_IPV4) { > + return in6_addr_mapped_ipv4(tnl->ip_dst); > + } > + else if(tnl->protocol == FLOW_TUNNEL_IPV6) { > + return tnl->ipv6_dst; > + } > + return in6addr_any; > } > > struct in6_addr > diff --git a/lib/packets.h b/lib/packets.h index 834e8a4..53a6ec0 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -35,8 +35,21 @@ > struct dp_packet; > struct ds; > > +/* Each type tunnel uses one bitfield. all bits are unset for > +unconfigured > + * tunnel address.*/ > +enum flow_tnl_proto_type { > + FLOW_TUNNEL_UNSPEC = 0, > + FLOW_TUNNEL_IPV4 = 1, //1st bit for ipv4 tunnel > + FLOW_TUNNEL_IPV6 = 2 //2nd bit for ipv6 tunnel }; > + > /* Tunnel information used in flow key and metadata. */ struct flow_tnl { > + /* The tunnel destination ip/ipv6 address are not initializing anywhere > due to > + * the performance constrains. the enum 'flow_tnl_proto_type' is used to > + * validate the destination addresses. > + */ > + enum flow_tnl_proto_type protocol; > ovs_be32 ip_dst; > struct in6_addr ipv6_dst; > ovs_be32 ip_src; > @@ -49,7 +62,7 @@ struct flow_tnl { > ovs_be16 tp_dst; > ovs_be16 gbp_id; > uint8_t gbp_flags; > - uint8_t pad1[5]; /* Pad to 64 bits. */ > + uint8_t pad1[1]; /* Pad to 64 bits. */ > struct tun_metadata metadata; > }; > > @@ -76,10 +89,36 @@ struct flow_tnl { > > static inline bool ipv6_addr_is_set(const struct in6_addr *addr); > > +static inline void > +set_ipv4_dst_tnl(struct flow_tnl *tnl, ovs_be32 ip_dst) { > + if(tnl) { > + tnl->ip_dst = ip_dst; > + if(tnl->ip_dst) { > + tnl->protocol = FLOW_TUNNEL_IPV4; > + } > + else { > + tnl->protocol = FLOW_TUNNEL_UNSPEC; > + } > + } > +} > + > +static inline void > +set_ipv6_dst_tnl(struct flow_tnl *tnl, struct in6_addr ipv6_dst) { > + if(tnl) { > + tnl->ipv6_dst = ipv6_dst; > + if(ipv6_addr_is_set(&tnl->ipv6_dst)) { > + tnl->protocol = FLOW_TUNNEL_IPV6; > + } > + else { > + tnl->protocol = FLOW_TUNNEL_UNSPEC; > + } > + } > +} > + > static inline bool > flow_tnl_dst_is_set(const struct flow_tnl *tnl) { > - return tnl->ip_dst || ipv6_addr_is_set(&tnl->ipv6_dst); > + return (tnl->protocol != FLOW_TUNNEL_UNSPEC); > } > > struct in6_addr flow_tnl_dst(const struct flow_tnl *tnl); @@ -90,8 +129,8 > @@ static inline size_t flow_tnl_size(const struct flow_tnl *src) { > if (!flow_tnl_dst_is_set(src)) { > - /* Covers ip_dst and ipv6_dst only. */ > - return offsetof(struct flow_tnl, ip_src); > + /* Covers protocol flag only. */ > + return offsetof(struct flow_tnl, ip_dst); > } > if (src->flags & FLOW_TNL_F_UDPIF) { > /* Datapath format, cover all options we have. */ @@ -157,8 +196,7 @@ > pkt_metadata_init(struct pkt_metadata *md, odp_port_t port) > * we can just zero out ip_dst and the rest of the data will never be > * looked at. */ > memset(md, 0, offsetof(struct pkt_metadata, in_port)); > - md->tunnel.ip_dst = 0; > - md->tunnel.ipv6_dst = in6addr_any; > + md->tunnel.protocol = FLOW_TUNNEL_UNSPEC; > > md->in_port.odp_port = port; > } > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index > a610c53..e00dac7 100644 > --- a/ofproto/ofproto-dpif-ipfix.c > +++ b/ofproto/ofproto-dpif-ipfix.c > @@ -1721,7 +1721,8 @@ dpif_ipfix_bridge_sample(struct dpif_ipfix *di, > const struct dp_packet *packet, > * of matched packets. */ > packet_delta_count = UINT32_MAX / di->bridge_exporter.probability; > if (di->bridge_exporter.options->enable_tunnel_sampling) { > - if (output_odp_port == ODPP_NONE && flow->tunnel.ip_dst) { > + if (output_odp_port == ODPP_NONE && > + flow->tunnel.protocol == FLOW_TUNNEL_IPV4) { > /* Input tunnel. */ > tunnel_key = &flow->tunnel; > tunnel_port = dpif_ipfix_find_port(di, input_odp_port); diff --git > a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c index > d142933..396489c 100644 > --- a/ofproto/ofproto-dpif-rid.c > +++ b/ofproto/ofproto-dpif-rid.c > @@ -297,8 +297,7 @@ uint32_t > recirc_alloc_id(struct ofproto_dpif *ofproto) { > struct flow_tnl tunnel; > - tunnel.ip_dst = htonl(0); > - tunnel.ipv6_dst = in6addr_any; > + tunnel.protocol = FLOW_TUNNEL_UNSPEC; > struct recirc_state state = { > .table_id = TBL_INTERNAL, > .ofproto = ofproto, > diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c > index f11699c..66686de 100644 > --- a/ofproto/ofproto-dpif-sflow.c > +++ b/ofproto/ofproto-dpif-sflow.c > @@ -899,7 +899,7 @@ sflow_read_tnl_push_action(const struct nlattr *attr, > /* IPv4 */ > /* Cannot assume alignment so just use memcpy. */ > sflow_actions->tunnel.ip_src = get_16aligned_be32(&ip->ip_src); > - sflow_actions->tunnel.ip_dst = get_16aligned_be32(&ip->ip_dst); > + set_ipv4_dst_tnl(&sflow_actions->tunnel, > + get_16aligned_be32(&ip->ip_dst)); > sflow_actions->tunnel.ip_tos = ip->ip_tos; > sflow_actions->tunnel.ip_ttl = ip->ip_ttl; > /* The tnl_push action can supply the ip_protocol too. */ @@ -991,7 > +991,7 @@ sflow_read_set_action(const struct nlattr *attr, > sflow_actions->tunnel.ip_src = key->ipv4_src; > } > if (key->ipv4_dst) { > - sflow_actions->tunnel.ip_dst = key->ipv4_dst; > + set_ipv4_dst_tnl(&sflow_actions->tunnel, > + key->ipv4_dst); > } > if (key->ipv4_proto) { > sflow_actions->tunnel_ipproto = key->ipv4_proto; @@ -1287,7 > +1287,7 @@ dpif_sflow_received(struct dpif_sflow *ds, const struct > dp_packet *packet, > fs.output = cookie->sflow.output; > > /* Input tunnel. */ > - if (flow->tunnel.ip_dst) { > + if (flow->tunnel.protocol == FLOW_TUNNEL_IPV4) { > memset(&tnlInElem, 0, sizeof(tnlInElem)); > tnlInElem.tag = SFLFLOW_EX_IPV4_TUNNEL_INGRESS; > tnlInProto = dpif_sflow_tunnel_proto(in_dsp->tunnel_type); > diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c index 5cf6c75..95f6df3 > 100644 > --- a/ofproto/tunnel.c > +++ b/ofproto/tunnel.c > @@ -362,12 +362,13 @@ tnl_wc_init(struct flow *flow, struct flow_wildcards > *wc) { > if (tnl_port_should_receive(flow)) { > wc->masks.tunnel.tun_id = OVS_BE64_MAX; > - if (flow->tunnel.ip_dst) { > + if (flow->tunnel.protocol == FLOW_TUNNEL_IPV4) { > wc->masks.tunnel.ip_src = OVS_BE32_MAX; > - wc->masks.tunnel.ip_dst = OVS_BE32_MAX; > - } else { > + set_ipv4_dst_tnl(&wc->masks.tunnel, OVS_BE32_MAX); > + } > + else if(flow->tunnel.protocol == FLOW_TUNNEL_IPV6) { > wc->masks.tunnel.ipv6_src = in6addr_exact; > - wc->masks.tunnel.ipv6_dst = in6addr_exact; > + set_ipv6_dst_tnl(&wc->masks.tunnel, in6addr_exact); > } > wc->masks.tunnel.flags = (FLOW_TNL_F_DONT_FRAGMENT | > FLOW_TNL_F_CSUM | @@ -424,7 +425,10 @@ > tnl_port_send(const struct ofport_dpif *ofport, struct flow *flow, > if (!cfg->ip_dst_flow) { > flow->tunnel.ip_dst = in6_addr_get_mapped_ipv4(&tnl_port- > >match.ipv6_dst); > if (!flow->tunnel.ip_dst) { > - flow->tunnel.ipv6_dst = tnl_port->match.ipv6_dst; > + set_ipv6_dst_tnl(&flow->tunnel, tnl_port->match.ipv6_dst); > + } > + else { > + set_ipv4_dst_tnl(&flow->tunnel, flow->tunnel.ip_dst); > } > } > flow->pkt_mark = tnl_port->match.pkt_mark; > -- > 1.9.1
On 12 January 2016 at 05:52, Chandran, Sugesh <sugesh.chandran@intel.com> wrote: > Hi, > We found that there is a serious performance drop in OVS userspace-datapath after the commit "tunneling: add IPv6 support to netdev_tunnel_config(commit ID :- 3ae91c019019889fbe93aa7dfd6e3747da362b24)" in the OVS tree. The PHY-PHY throughput dropped almost 25% for 64 byte packets due to this check-in. > The following patch is for fixing it. > Let me know your comments/suggestion on it , if any?? Hi Sugesh, Just so you know, we've recently adopted the linux-style "fixes" commit message tag which you use to provide the above information to other developers: Fixes: 3ae91c019019 ("tunneling: add IPv6 support to netdev_tunnel_config") You can place these above the "Signed-off-by" line in your commit message. This helps committers to identify which releases the bug affects and how far back the fix should go (v2.5? v2.4?). A description and how to generate it are here: https://github.com/openvswitch/ovs/commit/5a6b18a979ddda3f9cd8ff774397a9ad02bdc20f
diff --git a/lib/flow.c b/lib/flow.c index 5668d0c..34cee55 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -818,15 +818,15 @@ flow_get_metadata(const struct flow *flow, struct match *flow_metadata) if (flow->tunnel.ip_src) { match_set_tun_src(flow_metadata, flow->tunnel.ip_src); } - if (flow->tunnel.ip_dst) { + if (flow->tunnel.protocol == FLOW_TUNNEL_IPV4) { match_set_tun_dst(flow_metadata, flow->tunnel.ip_dst); } + else if (flow->tunnel.protocol == FLOW_TUNNEL_IPV6) { + match_set_tun_ipv6_dst(flow_metadata, &flow->tunnel.ipv6_dst); + } if (ipv6_addr_is_set(&flow->tunnel.ipv6_src)) { match_set_tun_ipv6_src(flow_metadata, &flow->tunnel.ipv6_src); } - if (ipv6_addr_is_set(&flow->tunnel.ipv6_dst)) { - match_set_tun_ipv6_dst(flow_metadata, &flow->tunnel.ipv6_dst); - } if (flow->tunnel.gbp_id != htons(0)) { match_set_tun_gbp_id(flow_metadata, flow->tunnel.gbp_id); } diff --git a/lib/match.c b/lib/match.c index 95d34bc..97a623e 100644 --- a/lib/match.c +++ b/lib/match.c @@ -186,8 +186,8 @@ match_set_tun_dst(struct match *match, ovs_be32 dst) void match_set_tun_dst_masked(struct match *match, ovs_be32 dst, ovs_be32 mask) { - match->wc.masks.tunnel.ip_dst = mask; - match->flow.tunnel.ip_dst = dst & mask; + set_ipv4_dst_tnl(&match->wc.masks.tunnel, mask); + set_ipv4_dst_tnl(&match->flow.tunnel, dst & mask); } void @@ -208,16 +208,16 @@ match_set_tun_ipv6_src_masked(struct match *match, const struct in6_addr *src, void match_set_tun_ipv6_dst(struct match *match, const struct in6_addr *dst) { - match->flow.tunnel.ipv6_dst = *dst; - match->wc.masks.tunnel.ipv6_dst = in6addr_exact; + set_ipv6_dst_tnl(&match->flow.tunnel, *dst); + set_ipv6_dst_tnl(&match->wc.masks.tunnel, in6addr_exact); } void match_set_tun_ipv6_dst_masked(struct match *match, const struct in6_addr *dst, const struct in6_addr *mask) { - match->flow.tunnel.ipv6_dst = ipv6_addr_bitand(dst, mask); - match->wc.masks.tunnel.ipv6_dst = *mask; + set_ipv6_dst_tnl(&match->flow.tunnel, ipv6_addr_bitand(dst, mask)); + set_ipv6_dst_tnl(&match->wc.masks.tunnel, *mask); } void diff --git a/lib/meta-flow.c b/lib/meta-flow.c index 6bd0b99..0826d1d 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c @@ -1180,13 +1180,13 @@ mf_set_flow_value(const struct mf_field *mf, flow->tunnel.ip_src = value->be32; break; case MFF_TUN_DST: - flow->tunnel.ip_dst = value->be32; + set_ipv4_dst_tnl(&flow->tunnel, value->be32); break; case MFF_TUN_IPV6_SRC: flow->tunnel.ipv6_src = value->ipv6; break; case MFF_TUN_IPV6_DST: - flow->tunnel.ipv6_dst = value->ipv6; + set_ipv6_dst_tnl(&flow->tunnel, value->ipv6); break; case MFF_TUN_FLAGS: flow->tunnel.flags = (flow->tunnel.flags & ~FLOW_TNL_PUB_F_MASK) | @@ -1503,6 +1503,13 @@ mf_set_wild(const struct mf_field *mf, struct match *match, char **err_str) sizeof match->wc.masks.tunnel.ipv6_dst); memset(&match->flow.tunnel.ipv6_dst, 0, sizeof match->flow.tunnel.ipv6_dst); + /* What if flow have a valid ipv4 tunnel address?? + * Reset the flag only if thats not the case. + */ + match->wc.masks.tunnel.protocol = (match->wc.masks.tunnel.protocol == + FLOW_TUNNEL_IPV4)? + match->wc.masks.tunnel.protocol: + FLOW_TUNNEL_UNSPEC; break; case MFF_TUN_FLAGS: match_set_tun_flags_masked(match, 0, 0); diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 88f5022..d3477c3 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -919,19 +919,18 @@ ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, ip_dst = get_16aligned_be32(&ip->ip_dst); tnl->ip_src = ip_src; - tnl->ip_dst = ip_dst; tnl->ip_tos = ip->ip_tos; tnl->ip_ttl = ip->ip_ttl; + set_ipv4_dst_tnl(tnl, ip_dst); *hlen += IP_HEADER_LEN; } else if (IP_VER(ip->ip_ihl_ver) == 6) { memcpy(tnl->ipv6_src.s6_addr, ip6->ip6_src.be16, sizeof ip6->ip6_src); - memcpy(tnl->ipv6_dst.s6_addr, ip6->ip6_dst.be16, sizeof ip6->ip6_dst); tnl->ip_tos = 0; tnl->ip_ttl = ip6->ip6_hlim; - + set_ipv6_dst_tnl(tnl, *(struct in6_addr *)&ip6->ip6_dst); *hlen += IPV6_HEADER_LEN; } else { diff --git a/lib/odp-util.c b/lib/odp-util.c index f16e113..26c3727 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -510,7 +510,7 @@ format_odp_tnl_push_header(struct ds *ds, struct ovs_action_push_tnl *data) gnh->oam ? "oam," : "", gnh->critical ? "crit," : "", ntohl(get_16aligned_be32(&gnh->vni)) >> 8); - + if (gnh->opt_len) { ds_put_cstr(ds, ",options("); format_geneve_opts(gnh->options, NULL, gnh->opt_len * 4, @@ -1864,13 +1864,13 @@ odp_tun_key_from_attr__(const struct nlattr *attr, tun->ip_src = nl_attr_get_be32(a); break; case OVS_TUNNEL_KEY_ATTR_IPV4_DST: - tun->ip_dst = nl_attr_get_be32(a); + set_ipv4_dst_tnl(tun, nl_attr_get_be32(a)); break; case OVS_TUNNEL_KEY_ATTR_IPV6_SRC: tun->ipv6_src = nl_attr_get_in6_addr(a); break; case OVS_TUNNEL_KEY_ATTR_IPV6_DST: - tun->ipv6_dst = nl_attr_get_in6_addr(a); + set_ipv6_dst_tnl(tun, nl_attr_get_in6_addr(a)); break; case OVS_TUNNEL_KEY_ATTR_TOS: tun->ip_tos = nl_attr_get_u8(a); @@ -1961,13 +1961,13 @@ tun_key_to_attr(struct ofpbuf *a, const struct flow_tnl *tun_key, if (tun_key->ip_src) { nl_msg_put_be32(a, OVS_TUNNEL_KEY_ATTR_IPV4_SRC, tun_key->ip_src); } - if (tun_key->ip_dst) { + if (tun_key->protocol == FLOW_TUNNEL_IPV4) { nl_msg_put_be32(a, OVS_TUNNEL_KEY_ATTR_IPV4_DST, tun_key->ip_dst); } if (ipv6_addr_is_set(&tun_key->ipv6_src)) { nl_msg_put_in6_addr(a, OVS_TUNNEL_KEY_ATTR_IPV6_SRC, &tun_key->ipv6_src); } - if (ipv6_addr_is_set(&tun_key->ipv6_dst)) { + if (tun_key->protocol == FLOW_TUNNEL_IPV6) { nl_msg_put_in6_addr(a, OVS_TUNNEL_KEY_ATTR_IPV6_DST, &tun_key->ipv6_dst); } if (tun_key->ip_tos) { diff --git a/lib/packets.c b/lib/packets.c index d82341d..ed6c5ad 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -39,7 +39,13 @@ const struct in6_addr in6addr_all_hosts = IN6ADDR_ALL_HOSTS_INIT; struct in6_addr flow_tnl_dst(const struct flow_tnl *tnl) { - return tnl->ip_dst ? in6_addr_mapped_ipv4(tnl->ip_dst) : tnl->ipv6_dst; + if(tnl->protocol == FLOW_TUNNEL_IPV4) { + return in6_addr_mapped_ipv4(tnl->ip_dst); + } + else if(tnl->protocol == FLOW_TUNNEL_IPV6) { + return tnl->ipv6_dst; + } + return in6addr_any; } struct in6_addr diff --git a/lib/packets.h b/lib/packets.h index 834e8a4..53a6ec0 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -35,8 +35,21 @@ struct dp_packet; struct ds; +/* Each type tunnel uses one bitfield. all bits are unset for unconfigured + * tunnel address.*/ +enum flow_tnl_proto_type { + FLOW_TUNNEL_UNSPEC = 0, + FLOW_TUNNEL_IPV4 = 1, //1st bit for ipv4 tunnel + FLOW_TUNNEL_IPV6 = 2 //2nd bit for ipv6 tunnel +}; + /* Tunnel information used in flow key and metadata. */ struct flow_tnl { + /* The tunnel destination ip/ipv6 address are not initializing anywhere due to + * the performance constrains. the enum 'flow_tnl_proto_type' is used to + * validate the destination addresses. + */ + enum flow_tnl_proto_type protocol; ovs_be32 ip_dst; struct in6_addr ipv6_dst; ovs_be32 ip_src; @@ -49,7 +62,7 @@ struct flow_tnl { ovs_be16 tp_dst; ovs_be16 gbp_id; uint8_t gbp_flags; - uint8_t pad1[5]; /* Pad to 64 bits. */ + uint8_t pad1[1]; /* Pad to 64 bits. */ struct tun_metadata metadata; }; @@ -76,10 +89,36 @@ struct flow_tnl { static inline bool ipv6_addr_is_set(const struct in6_addr *addr); +static inline void +set_ipv4_dst_tnl(struct flow_tnl *tnl, ovs_be32 ip_dst) { + if(tnl) { + tnl->ip_dst = ip_dst; + if(tnl->ip_dst) { + tnl->protocol = FLOW_TUNNEL_IPV4; + } + else { + tnl->protocol = FLOW_TUNNEL_UNSPEC; + } + } +} + +static inline void +set_ipv6_dst_tnl(struct flow_tnl *tnl, struct in6_addr ipv6_dst) { + if(tnl) { + tnl->ipv6_dst = ipv6_dst; + if(ipv6_addr_is_set(&tnl->ipv6_dst)) { + tnl->protocol = FLOW_TUNNEL_IPV6; + } + else { + tnl->protocol = FLOW_TUNNEL_UNSPEC; + } + } +} + static inline bool flow_tnl_dst_is_set(const struct flow_tnl *tnl) { - return tnl->ip_dst || ipv6_addr_is_set(&tnl->ipv6_dst); + return (tnl->protocol != FLOW_TUNNEL_UNSPEC); } struct in6_addr flow_tnl_dst(const struct flow_tnl *tnl); @@ -90,8 +129,8 @@ static inline size_t flow_tnl_size(const struct flow_tnl *src) { if (!flow_tnl_dst_is_set(src)) { - /* Covers ip_dst and ipv6_dst only. */ - return offsetof(struct flow_tnl, ip_src); + /* Covers protocol flag only. */ + return offsetof(struct flow_tnl, ip_dst); } if (src->flags & FLOW_TNL_F_UDPIF) { /* Datapath format, cover all options we have. */ @@ -157,8 +196,7 @@ pkt_metadata_init(struct pkt_metadata *md, odp_port_t port) * we can just zero out ip_dst and the rest of the data will never be * looked at. */ memset(md, 0, offsetof(struct pkt_metadata, in_port)); - md->tunnel.ip_dst = 0; - md->tunnel.ipv6_dst = in6addr_any; + md->tunnel.protocol = FLOW_TUNNEL_UNSPEC; md->in_port.odp_port = port; } diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index a610c53..e00dac7 100644 --- a/ofproto/ofproto-dpif-ipfix.c +++ b/ofproto/ofproto-dpif-ipfix.c @@ -1721,7 +1721,8 @@ dpif_ipfix_bridge_sample(struct dpif_ipfix *di, const struct dp_packet *packet, * of matched packets. */ packet_delta_count = UINT32_MAX / di->bridge_exporter.probability; if (di->bridge_exporter.options->enable_tunnel_sampling) { - if (output_odp_port == ODPP_NONE && flow->tunnel.ip_dst) { + if (output_odp_port == ODPP_NONE && + flow->tunnel.protocol == FLOW_TUNNEL_IPV4) { /* Input tunnel. */ tunnel_key = &flow->tunnel; tunnel_port = dpif_ipfix_find_port(di, input_odp_port); diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c index d142933..396489c 100644 --- a/ofproto/ofproto-dpif-rid.c +++ b/ofproto/ofproto-dpif-rid.c @@ -297,8 +297,7 @@ uint32_t recirc_alloc_id(struct ofproto_dpif *ofproto) { struct flow_tnl tunnel; - tunnel.ip_dst = htonl(0); - tunnel.ipv6_dst = in6addr_any; + tunnel.protocol = FLOW_TUNNEL_UNSPEC; struct recirc_state state = { .table_id = TBL_INTERNAL, .ofproto = ofproto, diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c index f11699c..66686de 100644 --- a/ofproto/ofproto-dpif-sflow.c +++ b/ofproto/ofproto-dpif-sflow.c @@ -899,7 +899,7 @@ sflow_read_tnl_push_action(const struct nlattr *attr, /* IPv4 */ /* Cannot assume alignment so just use memcpy. */ sflow_actions->tunnel.ip_src = get_16aligned_be32(&ip->ip_src); - sflow_actions->tunnel.ip_dst = get_16aligned_be32(&ip->ip_dst); + set_ipv4_dst_tnl(&sflow_actions->tunnel, get_16aligned_be32(&ip->ip_dst)); sflow_actions->tunnel.ip_tos = ip->ip_tos; sflow_actions->tunnel.ip_ttl = ip->ip_ttl; /* The tnl_push action can supply the ip_protocol too. */ @@ -991,7 +991,7 @@ sflow_read_set_action(const struct nlattr *attr, sflow_actions->tunnel.ip_src = key->ipv4_src; } if (key->ipv4_dst) { - sflow_actions->tunnel.ip_dst = key->ipv4_dst; + set_ipv4_dst_tnl(&sflow_actions->tunnel, key->ipv4_dst); } if (key->ipv4_proto) { sflow_actions->tunnel_ipproto = key->ipv4_proto; @@ -1287,7 +1287,7 @@ dpif_sflow_received(struct dpif_sflow *ds, const struct dp_packet *packet, fs.output = cookie->sflow.output; /* Input tunnel. */ - if (flow->tunnel.ip_dst) { + if (flow->tunnel.protocol == FLOW_TUNNEL_IPV4) { memset(&tnlInElem, 0, sizeof(tnlInElem)); tnlInElem.tag = SFLFLOW_EX_IPV4_TUNNEL_INGRESS; tnlInProto = dpif_sflow_tunnel_proto(in_dsp->tunnel_type); diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c index 5cf6c75..95f6df3 100644 --- a/ofproto/tunnel.c +++ b/ofproto/tunnel.c @@ -362,12 +362,13 @@ tnl_wc_init(struct flow *flow, struct flow_wildcards *wc) { if (tnl_port_should_receive(flow)) { wc->masks.tunnel.tun_id = OVS_BE64_MAX; - if (flow->tunnel.ip_dst) { + if (flow->tunnel.protocol == FLOW_TUNNEL_IPV4) { wc->masks.tunnel.ip_src = OVS_BE32_MAX; - wc->masks.tunnel.ip_dst = OVS_BE32_MAX; - } else { + set_ipv4_dst_tnl(&wc->masks.tunnel, OVS_BE32_MAX); + } + else if(flow->tunnel.protocol == FLOW_TUNNEL_IPV6) { wc->masks.tunnel.ipv6_src = in6addr_exact; - wc->masks.tunnel.ipv6_dst = in6addr_exact; + set_ipv6_dst_tnl(&wc->masks.tunnel, in6addr_exact); } wc->masks.tunnel.flags = (FLOW_TNL_F_DONT_FRAGMENT | FLOW_TNL_F_CSUM | @@ -424,7 +425,10 @@ tnl_port_send(const struct ofport_dpif *ofport, struct flow *flow, if (!cfg->ip_dst_flow) { flow->tunnel.ip_dst = in6_addr_get_mapped_ipv4(&tnl_port->match.ipv6_dst); if (!flow->tunnel.ip_dst) { - flow->tunnel.ipv6_dst = tnl_port->match.ipv6_dst; + set_ipv6_dst_tnl(&flow->tunnel, tnl_port->match.ipv6_dst); + } + else { + set_ipv4_dst_tnl(&flow->tunnel, flow->tunnel.ip_dst); } } flow->pkt_mark = tnl_port->match.pkt_mark;
Adding a new field called protocol in flow tunnel structure to verify the validity of tunnel metadata. This field avoids the need of resetting and validating the entire ipv4/ipv6 tunnel destination address which caused a serious performance drop. Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com> --- lib/flow.c | 8 +++---- lib/match.c | 12 +++++------ lib/meta-flow.c | 11 ++++++++-- lib/netdev-vport.c | 5 ++--- lib/odp-util.c | 10 ++++----- lib/packets.c | 8 ++++++- lib/packets.h | 50 ++++++++++++++++++++++++++++++++++++++------ ofproto/ofproto-dpif-ipfix.c | 3 ++- ofproto/ofproto-dpif-rid.c | 3 +-- ofproto/ofproto-dpif-sflow.c | 6 +++--- ofproto/tunnel.c | 14 ++++++++----- 11 files changed, 92 insertions(+), 38 deletions(-)