[ovs-dev] [PATCH v3 2/3] ofproto/trace: Add support for tracing conntrack recirculation
Ben Pfaff
blp at ovn.org
Wed Jul 12 22:46:33 UTC 2017
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.wei at 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);
More information about the dev
mailing list