Message ID | AM2PR07MB1042C843B907895E66FF257D8AEF0@AM2PR07MB1042.eurprd07.prod.outlook.com |
---|---|
State | Changes Requested |
Headers | show |
On 9 May 2017 at 04:46, Zoltán Balogh <zoltan.balogh@ericsson.com> wrote: > Hi, > > Thank you for your comments. Actually, I was wrong in my previous e-mail when I stated that packet is truncated only at the end of the pipeline, when output is applied. The packet size is set according max_len set by truncate when tunnel_push is applied. So the size of packet is correct. > > The root cause of the problem, why stats in underlay bridge is wrong, is that statistics will be incremented by the packet number and packet size set by the upcall_xlate(). > > static void > upcall_xlate(struct udpif *udpif, struct upcall *upcall, > struct ofpbuf *odp_actions, struct flow_wildcards *wc) > { > struct dpif_flow_stats stats; > struct xlate_in xin; > > stats.n_packets = 1; > stats.n_bytes = dp_packet_size(upcall->packet); > stats.used = time_msec(); > stats.tcp_flags = ntohs(upcall->flow->tcp_flags); > ... > } > > Since we excluded recirculation in the "Avoid recirculation" patch, there is no second upcall when packet enters tunnel, but stats created by "first" and only upcall are used for statistics of both integrate and underlay bridges. And that's not correct. > > I completed my old patch to solve this problem by adding two new members to struct rule_dpif and modify stats with their values. Hi Zoltan, thanks again for looking into this. I had trouble trying to review this, in part because I felt like changes to fix multiple issues are being placed into one patch. If there are separate issues being addressed, each issue should be fixed in a separate patch, each with a clear description of the intent of the change (with links to patches that introduced the breakage if possible!). If there is refactoring to do, consider separating the non-functional changes into a separate patch---when looking at the original patch that started this discussion, I found it difficult to review post-merge because the refactoring was included and I couldn't tell if there were actually functional changes. Given that this is taking a bit longer than I thought and we need to take a holistic approach to how all of these interactions should work, I plan to go ahead and apply the revert patch. I look forward to seeing a fresh series proposal that will bring back the benefits of the original patch. More feedback below. > Here comes the patch: > > > Since tunneling and forwarding via patch port uses clone action, the tunneled Where does forwarding via patch port use clone action? > packet will not be parsed by miniflow_extract() and flow data will not be > updated within clone. So when a tunnel header is added to the packet, the MAC > and IP data of struct flow will not be updated according to the newly created > tunnel header. > > This patch stores MAC and IP data of flow before calling > apply_nested_clone_actions() in build_tunnel_send(), then restores the data > after apply_nested_clone_actions() has returned. > > Furthermore, it introduces truncate_packet_size and tunnel_header_size struct > rule_dpif members in order to correct n_bytes statistics of OF flows. > --- > ofproto/ofproto-dpif-xlate.c | 92 ++++++++++++++++++++++++++++++++++++++++++++ > ofproto/ofproto-dpif.c | 14 ++++++- > ofproto/ofproto-dpif.h | 5 +++ > tests/ofproto-dpif.at | 11 +++++- > 4 files changed, 120 insertions(+), 2 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index b308f21..55015d7 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3141,6 +3141,33 @@ tnl_send_arp_request(struct xlate_ctx *ctx, const struct xport *out_dev, > dp_packet_uninit(&packet); > } > > +/* > + * Copy IP data of 'flow->tunnel' into 'flow' and 'base_flow'. > + */ > +static void > +propagate_tunnel_data_to_flow(struct flow *flow, struct flow *base_flow) > +{ > + flow->nw_dst = flow->tunnel.ip_dst; > + flow->nw_src = flow->tunnel.ip_src; > + flow->ipv6_dst = flow->tunnel.ipv6_dst; > + flow->ipv6_src = flow->tunnel.ipv6_src; > + > + flow->nw_tos = flow->tunnel.ip_tos; > + flow->nw_ttl = flow->tunnel.ip_ttl; > + flow->tp_dst = flow->tunnel.tp_dst; > + flow->tp_src = flow->tunnel.tp_src; > + > + base_flow->nw_dst = flow->nw_dst; > + base_flow->nw_src = flow->nw_src; > + base_flow->ipv6_dst = flow->ipv6_dst; > + base_flow->ipv6_src = flow->ipv6_src; > + > + base_flow->nw_tos = flow->nw_tos; > + base_flow->nw_ttl = flow->nw_ttl; > + base_flow->tp_dst = flow->tp_dst; > + base_flow->tp_src = flow->tp_src; > +} > + > static int > build_tunnel_send(struct xlate_ctx *ctx, const struct xport *xport, > const struct flow *flow, odp_port_t tunnel_odp_port) > @@ -3156,6 +3183,11 @@ build_tunnel_send(struct xlate_ctx *ctx, const struct xport *xport, > int err; > char buf_sip6[INET6_ADDRSTRLEN]; > char buf_dip6[INET6_ADDRSTRLEN]; > + /* Structures to backup Ethernet and IP data of flow and base_flow. */ > + struct flow old_flow = ctx->xin->flow; > + struct flow old_base_flow = ctx->base_flow; > + /* Variable to backup actual tunnel header size. */ > + uint64_t old_ths = 0; apply_nested_clone_action() also takes copies of the flow and base_flow, do we need to do it twice in this path? I suspect that this work could benefit from some further refactoring, taking note to have separate patches to refactor code and different patches for functional changes. > > err = tnl_route_lookup_flow(flow, &d_ip6, &s_ip6, &out_dev); > if (err) { > @@ -3216,16 +3248,57 @@ build_tunnel_send(struct xlate_ctx *ctx, const struct xport *xport, > tnl_push_data.tnl_port = odp_to_u32(tunnel_odp_port); > tnl_push_data.out_port = odp_to_u32(out_dev->odp_port); > > + /* After tunnel header has been added, MAC and IP data of flow and > + * base_flow need to be set properly, since there is not recirculation > + * any more when sending packet to tunnel. */ > + if (!eth_addr_is_zero(ctx->xin->flow.dl_dst)) { > + ctx->xin->flow.dl_dst = dmac; > + ctx->base_flow.dl_dst = ctx->xin->flow.dl_dst; > + } What's the expectation if ctx->xin->flow.dl_dst is zero? Is that for L3 tunneled packets? > + > + ctx->xin->flow.dl_src = smac; > + ctx->base_flow.dl_src = ctx->xin->flow.dl_src; > + > + propagate_tunnel_data_to_flow(&ctx->xin->flow, &ctx->base_flow); > + > + if (!tnl_params.is_ipv6) { > + ctx->xin->flow.dl_type = htons(ETH_TYPE_IP); > + } else { > + ctx->xin->flow.dl_type = htons(ETH_TYPE_IPV6); > + } When writing if (!condition) { ... } else { ... }, please consider whether you can swap the conditions and remove the negation. It's less cognitive load if we don't have to negate a negative condition to consider which condition the "else" applies to. (This otherwise looks correct, so just a style request.) > + > + if (tnl_push_data.tnl_type == OVS_VPORT_TYPE_GRE) { > + ctx->xin->flow.nw_proto = IPPROTO_GRE; > + } else { > + ctx->xin->flow.nw_proto = IPPROTO_UDP; > + } > + ctx->base_flow.nw_proto = ctx->xin->flow.nw_proto; Should we use a switch () statement here to ensure that newer tunnel ports are properly handled in this case if/when they are added? What about things like LISP or STT? > + > size_t push_action_size = 0; > size_t clone_ofs = nl_msg_start_nested(ctx->odp_actions, > OVS_ACTION_ATTR_CLONE); > odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data); > push_action_size = ctx->odp_actions->size; > + > + if (ctx->rule) { > + old_ths = ctx->rule->tunnel_header_size; > + ctx->rule->tunnel_header_size = tnl_push_data.header_len; > + } > + > apply_nested_clone_actions(ctx, xport, out_dev); > if (ctx->odp_actions->size > push_action_size) { > /* Update the CLONE action only when combined */ > nl_msg_end_nested(ctx->odp_actions, clone_ofs); > } If there are no actions on the underlay bridge (ie drop), the else condition here should probably reset the nl_msg nesting so that we don't generate any actions to the datapath at all. You may also consider adding a xlate_report() call to explain why the drop is occuring - this should help debugging later on using ofproto/trace. > + > + if (ctx->rule) { > + ctx->rule->tunnel_header_size = old_ths; > + } > + > + /* Restore flow and base_flow data. */ > + ctx->xin->flow = old_flow; > + ctx->base_flow = old_base_flow; > + > return 0; > } > > @@ -3728,6 +3801,10 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, > } > > if (rule) { > + if (ctx->rule) { > + rule->truncated_packet_size = ctx->rule->truncated_packet_size; > + rule->tunnel_header_size = ctx->rule->tunnel_header_size; > + } > /* Fill in the cache entry here instead of xlate_recursively > * to make the reference counting more explicit. We take a > * reference in the lookups above if we are going to cache the > @@ -4602,6 +4679,8 @@ xlate_output_trunc_action(struct xlate_ctx *ctx, > bool support_trunc = ctx->xbridge->support.trunc; > struct ovs_action_trunc *trunc; > char name[OFP10_MAX_PORT_NAME_LEN]; > + /* Variable to backup truncate max len. */ > + uint64_t old_tps = 0; > > switch (port) { > case OFPP_TABLE: > @@ -4634,7 +4713,20 @@ xlate_output_trunc_action(struct xlate_ctx *ctx, > OVS_ACTION_ATTR_TRUNC, > sizeof *trunc); > trunc->max_len = max_len; > + > + /* Update truncate correction. */ > + if (ctx->rule) { > + old_tps = ctx->rule->truncated_packet_size; > + ctx->rule->truncated_packet_size = max_len; > + } > + > xlate_output_action(ctx, port, max_len, false); > + > + /* Restore truncate correction. */ > + if (ctx->rule) { > + ctx->rule->truncated_packet_size = old_tps; > + } > + > if (!support_trunc) { > ctx->xout->slow |= SLOW_ACTION; > } > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index dc5f004..800d0f6 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -3959,8 +3959,18 @@ rule_dpif_credit_stats__(struct rule_dpif *rule, > OVS_REQUIRES(rule->stats_mutex) > { > if (credit_counts) { > + uint64_t stats_n_bytes = 0; > + > + if (rule->truncated_packet_size) { > + stats_n_bytes = MIN(rule->truncated_packet_size, stats->n_bytes); > + } else { > + stats_n_bytes = stats->n_bytes; > + } Is this fixing a separate issue? I'm confused about whether truncated packet stats are being correctly attributed today on master. If so, I think this should split out into a separate patch. You might also consider whether it makes more sense to modify 'stats' earlier in the path so that each rule doesn't need to individually apply the stats adjustment. I could imagine a store/restore of the stats plus modifying for truncation during the xlate_output_trunc_action() processing rather than pushing this complexity all the way into the rule stats attribution. > + > + stats_n_bytes += rule->tunnel_header_size; > + > rule->stats.n_packets += stats->n_packets; > - rule->stats.n_bytes += stats->n_bytes; > + rule->stats.n_bytes += stats_n_bytes; > } > rule->stats.used = MAX(rule->stats.used, stats->used); > } > @@ -4153,6 +4163,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto, > entry->table.match = (rule != NULL); > } > if (rule) { > + rule->truncated_packet_size = 0; > + rule->tunnel_header_size = 0; > goto out; /* Match. */ > } > if (honor_table_miss) { > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h > index a3a6f1f..748df4c 100644 > --- a/ofproto/ofproto-dpif.h > +++ b/ofproto/ofproto-dpif.h > @@ -93,6 +93,11 @@ struct rule_dpif { > * The recirculation id and associated internal flow should > * be freed when the rule is freed */ > uint32_t recirc_id; > + > + /* Variables to be used to correct statistic when packet is sent to tunnel > + * or truncated. */ > + uint64_t truncated_packet_size; > + uint64_t tunnel_header_size; > }; > > struct rule_dpif *rule_dpif_lookup_from_table(struct ofproto_dpif *, > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index e52ab32..1f6cd84 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -6257,6 +6257,15 @@ HEADER > dgramSeqNo=1 > ds=127.0.0.1>2:1000 > fsSeqNo=1 > + tunnel4_out_length=0 > + tunnel4_out_protocol=47 > + tunnel4_out_src=1.1.2.88 > + tunnel4_out_dst=1.1.2.92 > + tunnel4_out_src_port=0 > + tunnel4_out_dst_port=0 > + tunnel4_out_tcp_flags=0 > + tunnel4_out_tos=0 > + tunnel_out_vni=456 > in_vlan=0 > in_priority=0 > out_vlan=0 > @@ -6266,7 +6275,7 @@ HEADER > dropEvents=0 > in_ifindex=2011 > in_format=0 > - out_ifindex=2 > + out_ifindex=1 > out_format=2 > hdr_prot=1 > pkt_len=46 > -- > 2.7.4
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index b308f21..55015d7 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3141,6 +3141,33 @@ tnl_send_arp_request(struct xlate_ctx *ctx, const struct xport *out_dev, dp_packet_uninit(&packet); } +/* + * Copy IP data of 'flow->tunnel' into 'flow' and 'base_flow'. + */ +static void +propagate_tunnel_data_to_flow(struct flow *flow, struct flow *base_flow) +{ + flow->nw_dst = flow->tunnel.ip_dst; + flow->nw_src = flow->tunnel.ip_src; + flow->ipv6_dst = flow->tunnel.ipv6_dst; + flow->ipv6_src = flow->tunnel.ipv6_src; + + flow->nw_tos = flow->tunnel.ip_tos; + flow->nw_ttl = flow->tunnel.ip_ttl; + flow->tp_dst = flow->tunnel.tp_dst; + flow->tp_src = flow->tunnel.tp_src; + + base_flow->nw_dst = flow->nw_dst; + base_flow->nw_src = flow->nw_src; + base_flow->ipv6_dst = flow->ipv6_dst; + base_flow->ipv6_src = flow->ipv6_src; + + base_flow->nw_tos = flow->nw_tos; + base_flow->nw_ttl = flow->nw_ttl; + base_flow->tp_dst = flow->tp_dst; + base_flow->tp_src = flow->tp_src; +} + static int build_tunnel_send(struct xlate_ctx *ctx, const struct xport *xport, const struct flow *flow, odp_port_t tunnel_odp_port) @@ -3156,6 +3183,11 @@ build_tunnel_send(struct xlate_ctx *ctx, const struct xport *xport, int err; char buf_sip6[INET6_ADDRSTRLEN]; char buf_dip6[INET6_ADDRSTRLEN]; + /* Structures to backup Ethernet and IP data of flow and base_flow. */ + struct flow old_flow = ctx->xin->flow; + struct flow old_base_flow = ctx->base_flow; + /* Variable to backup actual tunnel header size. */ + uint64_t old_ths = 0; err = tnl_route_lookup_flow(flow, &d_ip6, &s_ip6, &out_dev); if (err) { @@ -3216,16 +3248,57 @@ build_tunnel_send(struct xlate_ctx *ctx, const struct xport *xport, tnl_push_data.tnl_port = odp_to_u32(tunnel_odp_port); tnl_push_data.out_port = odp_to_u32(out_dev->odp_port); + /* After tunnel header has been added, MAC and IP data of flow and + * base_flow need to be set properly, since there is not recirculation + * any more when sending packet to tunnel. */ + if (!eth_addr_is_zero(ctx->xin->flow.dl_dst)) { + ctx->xin->flow.dl_dst = dmac; + ctx->base_flow.dl_dst = ctx->xin->flow.dl_dst; + } + + ctx->xin->flow.dl_src = smac; + ctx->base_flow.dl_src = ctx->xin->flow.dl_src; + + propagate_tunnel_data_to_flow(&ctx->xin->flow, &ctx->base_flow); + + if (!tnl_params.is_ipv6) { + ctx->xin->flow.dl_type = htons(ETH_TYPE_IP); + } else { + ctx->xin->flow.dl_type = htons(ETH_TYPE_IPV6); + } + + if (tnl_push_data.tnl_type == OVS_VPORT_TYPE_GRE) { + ctx->xin->flow.nw_proto = IPPROTO_GRE; + } else { + ctx->xin->flow.nw_proto = IPPROTO_UDP; + } + ctx->base_flow.nw_proto = ctx->xin->flow.nw_proto; + size_t push_action_size = 0; size_t clone_ofs = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE); odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data); push_action_size = ctx->odp_actions->size; + + if (ctx->rule) { + old_ths = ctx->rule->tunnel_header_size; + ctx->rule->tunnel_header_size = tnl_push_data.header_len; + } + apply_nested_clone_actions(ctx, xport, out_dev); if (ctx->odp_actions->size > push_action_size) { /* Update the CLONE action only when combined */ nl_msg_end_nested(ctx->odp_actions, clone_ofs); } + + if (ctx->rule) { + ctx->rule->tunnel_header_size = old_ths; + } + + /* Restore flow and base_flow data. */ + ctx->xin->flow = old_flow; + ctx->base_flow = old_base_flow; + return 0; } @@ -3728,6 +3801,10 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, } if (rule) { + if (ctx->rule) { + rule->truncated_packet_size = ctx->rule->truncated_packet_size; + rule->tunnel_header_size = ctx->rule->tunnel_header_size; + } /* Fill in the cache entry here instead of xlate_recursively * to make the reference counting more explicit. We take a * reference in the lookups above if we are going to cache the @@ -4602,6 +4679,8 @@ xlate_output_trunc_action(struct xlate_ctx *ctx, bool support_trunc = ctx->xbridge->support.trunc; struct ovs_action_trunc *trunc; char name[OFP10_MAX_PORT_NAME_LEN]; + /* Variable to backup truncate max len. */ + uint64_t old_tps = 0; switch (port) { case OFPP_TABLE: @@ -4634,7 +4713,20 @@ xlate_output_trunc_action(struct xlate_ctx *ctx, OVS_ACTION_ATTR_TRUNC, sizeof *trunc); trunc->max_len = max_len; + + /* Update truncate correction. */ + if (ctx->rule) { + old_tps = ctx->rule->truncated_packet_size; + ctx->rule->truncated_packet_size = max_len; + } + xlate_output_action(ctx, port, max_len, false); + + /* Restore truncate correction. */ + if (ctx->rule) { + ctx->rule->truncated_packet_size = old_tps; + } + if (!support_trunc) { ctx->xout->slow |= SLOW_ACTION; } diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index dc5f004..800d0f6 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3959,8 +3959,18 @@ rule_dpif_credit_stats__(struct rule_dpif *rule, OVS_REQUIRES(rule->stats_mutex) { if (credit_counts) { + uint64_t stats_n_bytes = 0; + + if (rule->truncated_packet_size) { + stats_n_bytes = MIN(rule->truncated_packet_size, stats->n_bytes); + } else { + stats_n_bytes = stats->n_bytes; + } + + stats_n_bytes += rule->tunnel_header_size; + rule->stats.n_packets += stats->n_packets; - rule->stats.n_bytes += stats->n_bytes; + rule->stats.n_bytes += stats_n_bytes; } rule->stats.used = MAX(rule->stats.used, stats->used); } @@ -4153,6 +4163,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto, entry->table.match = (rule != NULL); } if (rule) { + rule->truncated_packet_size = 0; + rule->tunnel_header_size = 0; goto out; /* Match. */ } if (honor_table_miss) { diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index a3a6f1f..748df4c 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -93,6 +93,11 @@ struct rule_dpif { * The recirculation id and associated internal flow should * be freed when the rule is freed */ uint32_t recirc_id; + + /* Variables to be used to correct statistic when packet is sent to tunnel + * or truncated. */ + uint64_t truncated_packet_size; + uint64_t tunnel_header_size; }; struct rule_dpif *rule_dpif_lookup_from_table(struct ofproto_dpif *, diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index e52ab32..1f6cd84 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -6257,6 +6257,15 @@ HEADER dgramSeqNo=1 ds=127.0.0.1>2:1000 fsSeqNo=1 + tunnel4_out_length=0 + tunnel4_out_protocol=47 + tunnel4_out_src=1.1.2.88 + tunnel4_out_dst=1.1.2.92 + tunnel4_out_src_port=0 + tunnel4_out_dst_port=0 + tunnel4_out_tcp_flags=0 + tunnel4_out_tos=0 + tunnel_out_vni=456 in_vlan=0 in_priority=0 out_vlan=0 @@ -6266,7 +6275,7 @@ HEADER dropEvents=0 in_ifindex=2011 in_format=0 - out_ifindex=2 + out_ifindex=1 out_format=2 hdr_prot=1 pkt_len=46