On Tue, Jun 27, 2017 at 11:11:33AM -0700, Yi-Hung Wei wrote: > Previously, a user need to run ofproto/trace multiple times to derive the > final datapath actions if a flow hit conntrack actions that involves > recirculation. To improve the usability of ofproto/trace, in this patch, > we keep track of the conntrack actions, and automatically run the > recirculation process so that a user only need to execute the ofproto/trace > command once. Currently, this patch sets the default ct_state as > trk and new in the automatic recirculation process. A following patch > will provide an option to customize ct_state. > > Signed-off-by: Yi-Hung Wei <yihung....@gmail.com>
This is pretty cool. Thanks a lot. I made a few changes and applied this to master. I noticed that OFT_RECIRC_INIT wasn't really needed if we changed ofproto_trace() so that it didn't put the initial state into the queue. I changed the 'flow' member of struct oftrace_recirc_node from a pointer to just an inline struct flow. I added a 'packet' member to struct oftrace_recirc_node because it seemed like the packet was an important part of the state that could change over time. I also changed the output format so that, at least to me, it more clearly called attention to what was being traced. I'm appending the changes I made, as an incremental diff. --8<--------------------------cut here-------------------------->8-- diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c index 362f932a23a7..23c35e6ffdb0 100644 --- a/ofproto/ofproto-dpif-trace.c +++ b/ofproto/ofproto-dpif-trace.c @@ -23,7 +23,7 @@ #include "openvswitch/ofp-parse.h" #include "unixctl.h" -static void ofproto_trace(struct ofproto_dpif *, struct flow *, +static void ofproto_trace(struct ofproto_dpif *, const struct flow *, const struct dp_packet *packet, const struct ofpact[], size_t ofpacts_len, struct ds *); @@ -88,13 +88,11 @@ oftrace_node_destroy(struct oftrace_node *node) bool oftrace_add_recirc_node(struct ovs_list *recirc_queue, - enum oftrace_recirc_type type, struct flow *flow, - uint32_t recirc_id) + enum oftrace_recirc_type type, const struct flow *flow, + const struct dp_packet *packet, uint32_t recirc_id) { - if (type != OFT_RECIRC_INIT) { - if (!recirc_id_node_find_and_ref(recirc_id)) { - return false; - } + if (!recirc_id_node_find_and_ref(recirc_id)) { + return false; } struct oftrace_recirc_node *node = xmalloc(sizeof *node); @@ -102,8 +100,9 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue, node->type = type; node->recirc_id = recirc_id; - node->flow = xmemdup(flow, sizeof *flow); - node->flow->recirc_id = recirc_id; + node->flow = *flow; + node->flow.recirc_id = recirc_id; + node->packet = packet ? dp_packet_clone(packet) : NULL; return true; } @@ -111,11 +110,11 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue, static void oftrace_recirc_node_destroy(struct oftrace_recirc_node *node) { - if (node->type != OFT_RECIRC_INIT) { + if (node) { recirc_free_id(node->recirc_id); + dp_packet_delete(node->packet); + free(node); } - free(node->flow); - free(node); } static void @@ -452,7 +451,7 @@ exit: } static void -ofproto_trace__(struct ofproto_dpif *ofproto, struct flow *flow, +ofproto_trace__(struct ofproto_dpif *ofproto, const struct flow *flow, const struct dp_packet *packet, struct ovs_list *recirc_queue, const struct ofpact ofpacts[], size_t ofpacts_len, struct ds *output) @@ -537,30 +536,32 @@ ofproto_trace__(struct ofproto_dpif *ofproto, struct flow *flow, * If 'ofpacts' is nonnull then its 'ofpacts_len' bytes specify the actions to * trace, otherwise the actions are determined by a flow table lookup. */ static void -ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow, +ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow, const struct dp_packet *packet, const struct ofpact ofpacts[], size_t ofpacts_len, struct ds *output) { struct ovs_list recirc_queue = OVS_LIST_INITIALIZER(&recirc_queue); - oftrace_add_recirc_node(&recirc_queue, OFT_RECIRC_INIT, flow, - flow->recirc_id); - - while (!ovs_list_is_empty(&recirc_queue)) { - struct ovs_list *node = ovs_list_pop_front(&recirc_queue); - struct oftrace_recirc_node *recirc_node; - - ASSIGN_CONTAINER(recirc_node, node, node); - + ofproto_trace__(ofproto, flow, packet, &recirc_queue, + ofpacts, ofpacts_len, output); + + struct oftrace_recirc_node *recirc_node; + LIST_FOR_EACH_POP (recirc_node, node, &recirc_queue) { + ds_put_cstr(output, "\n\n"); + ds_put_char_multiple(output, '=', 79); + ds_put_format(output, "\nrecirc(%#"PRIx32")", + recirc_node->recirc_id); if (recirc_node->type == OFT_RECIRC_CONNTRACK) { - recirc_node->flow->ct_state = CS_TRACKED | CS_NEW; - ds_put_cstr(output, "\n\nResume conntrack prcessing with " - "default ct_state=trk|new.\n"); - ds_put_char_multiple(output, '-', 79); - ds_put_char(output, '\n'); + recirc_node->flow.ct_state = CS_TRACKED | CS_NEW; + ds_put_cstr(output, " - resume conntrack processing with " + "default ct_state=trk|new"); } - ofproto_trace__(ofproto, recirc_node->flow, packet, &recirc_queue, - ofpacts, ofpacts_len, output); + ds_put_char(output, '\n'); + ds_put_char_multiple(output, '=', 79); + ds_put_cstr(output, "\n\n"); + + ofproto_trace__(ofproto, &recirc_node->flow, recirc_node->packet, + &recirc_queue, ofpacts, ofpacts_len, output); oftrace_recirc_node_destroy(recirc_node); } } diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h index db3be290e477..4c120a54d4a9 100644 --- a/ofproto/ofproto-dpif-trace.h +++ b/ofproto/ofproto-dpif-trace.h @@ -30,6 +30,7 @@ #include "openvswitch/compiler.h" #include "openvswitch/list.h" +#include "flow.h" /* Type of a node within a trace. */ enum oftrace_node_type { @@ -45,11 +46,8 @@ enum oftrace_node_type { OFT_ERROR, /* An erroneous situation, worth logging. */ }; -/* Type of a node within a recirculation queue. */ +/* Reason why a flow is in a recirculation queue. */ enum oftrace_recirc_type { - /* The initial flow to be traced. */ - OFT_RECIRC_INIT, - /* A flow recirculates because of the following actions. */ OFT_RECIRC_CONNTRACK, OFT_RECIRC_MPLS, OFT_RECIRC_BOND, @@ -70,7 +68,8 @@ struct oftrace_recirc_node { enum oftrace_recirc_type type; uint32_t recirc_id; - struct flow *flow; + struct flow flow; + struct dp_packet *packet; }; void ofproto_dpif_trace_init(void); @@ -78,7 +77,7 @@ void ofproto_dpif_trace_init(void); struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type, const char *text); bool oftrace_add_recirc_node(struct ovs_list *recirc_queue, - enum oftrace_recirc_type, struct flow *, - uint32_t recirc_id); + enum oftrace_recirc_type, const struct flow *, + const struct dp_packet *, uint32_t recirc_id); #endif /* ofproto-dpif-trace.h */ diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 716858e8dd44..c4158dfb6923 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4441,7 +4441,7 @@ compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) { if (oftrace_add_recirc_node(ctx->xin->recirc_queue, OFT_RECIRC_CONNTRACK, &ctx->xin->flow, - recirc_id)) { + ctx->xin->packet, recirc_id)) { xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to " "recirculate. The forked pipeline will be resumed at " "table %u.", table); _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev