| Message ID | 1498068412-89824-3-git-send-email-yihung.wei@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> 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 est in the automatic recirculation process.
Why is ‘est’ part of the default ?
A following patch
will provide an option to customize ct_state.
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
ofproto/ofproto-dpif-trace.c | 88 +++++++++++++++++++++++++++++++++++++-------
ofproto/ofproto-dpif-trace.h | 22 +++++++++++
ofproto/ofproto-dpif-xlate.c | 24 ++++++++++--
ofproto/ofproto-dpif-xlate.h | 4 ++
tests/ofproto-dpif.at | 35 ++++++++++++++++++
5 files changed, 156 insertions(+), 17 deletions(-)
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
index c7f0e48caa16..d8cdd92493cf 100644
--- a/ofproto/ofproto-dpif-trace.c
+++ b/ofproto/ofproto-dpif-trace.c
@@ -86,6 +86,39 @@ oftrace_node_destroy(struct oftrace_node *node)
}
}
+void
+oftrace_add_recirc_node(struct ovs_list *recirc_queue,
+ enum oftrace_recirc_type type, struct flow *flow,
+ uint32_t recirc_id)
+{
+ struct oftrace_recirc_node *node = xmalloc(sizeof *node);
+ ovs_list_push_back(recirc_queue, &node->node);
+
+ node->type = type;
+ node->recirc_id = recirc_id;
+ node->flow = xmalloc(sizeof *flow);
+ memcpy(node->flow, flow, sizeof *flow);
+ node->flow->recirc_id = recirc_id;
+
+ if (type != OFT_INIT) {
+ struct recirc_id_node *rid_node = CONST_CAST(
+ struct recirc_id_node *, recirc_id_node_find(recirc_id));
+ if (rid_node) {
+ ovs_refcount_try_ref_rcu(&rid_node->refcount);
+ }
+ }
+}
+
+static void
+oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)
+{
+ if (node->type != OFT_INIT) {
+ recirc_free_id(node->recirc_id);
+ }
+ free(node->flow);
+ free(node);
+}
+
static void
oftrace_node_print_details(struct ds *output,
const struct ovs_list *nodes, int level)
@@ -419,20 +452,11 @@ exit:
ofpbuf_uninit(&ofpacts);
}
-/* Implements a "trace" through 'ofproto''s flow table, appending a textual
- * description of the results to 'output'.
- *
- * The trace follows a packet with the specified 'flow' through the flow
- * table. 'packet' may be nonnull to trace an actual packet, with consequent
- * side effects (if it is nonnull then its flow must be '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,
- const struct dp_packet *packet,
- const struct ofpact ofpacts[], size_t ofpacts_len,
- struct ds *output)
+ofproto_trace__(struct ofproto_dpif *ofproto, struct flow *flow,
+ const struct dp_packet *packet, struct ovs_list *recirc_queue,
+ const struct ofpact ofpacts[], size_t ofpacts_len,
+ struct ds *output)
{
struct ofpbuf odp_actions;
ofpbuf_init(&odp_actions, 0);
@@ -447,6 +471,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
xin.ofpacts = ofpacts;
xin.ofpacts_len = ofpacts_len;
xin.trace = &trace;
+ xin.recirc_queue = recirc_queue;
/* Copy initial flow out of xin.flow. It differs from '*flow' because
* xlate_in_init() initializes actset_output to OFPP_UNSET. */
@@ -503,6 +528,43 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
oftrace_node_list_destroy(&trace);
}
+/* Implements a "trace" through 'ofproto''s flow table, appending a textual
+ * description of the results to 'output'.
+ *
+ * The trace follows a packet with the specified 'flow' through the flow
+ * table. 'packet' may be nonnull to trace an actual packet, with consequent
+ * side effects (if it is nonnull then its flow must be '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,
+ 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_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);
+
+ if (recirc_node->type == OFT_CONNTRACK) {
+ recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
+ ds_put_cstr(output, "\n\nResume conntrack prcessing with "
+ "default ct_state=trk|est.\n");
+ ds_put_char_multiple(output, '-', 79);
+ ds_put_char(output, '\n');
+ }
+ ofproto_trace__(ofproto, recirc_node->flow, packet, &recirc_queue,
+ ofpacts, ofpacts_len, output);
+ oftrace_recirc_node_destroy(recirc_node);
+ }
+}
+
void
ofproto_dpif_trace_init(void)
{
diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h
index e3d3b393cec8..f7299fd42848 100644
--- a/ofproto/ofproto-dpif-trace.h
+++ b/ofproto/ofproto-dpif-trace.h
@@ -45,6 +45,16 @@ enum oftrace_node_type {
OFT_ERROR, /* An erroneous situation, worth logging. */
};
+/* Type of a node within a recirculation queue. */
+enum oftrace_recirc_type {
+ /* The initial flow to be traced. */
+ OFT_INIT,
+ /* A flow recirculates because of the following actions. */
+ OFT_CONNTRACK,
+ OFT_MPLS,
+ OFT_BOND,
+};
+
/* A node within a trace. */
struct oftrace_node {
struct ovs_list node; /* In parent. */
@@ -54,9 +64,21 @@ struct oftrace_node {
char *text;
};
+/* A node within a recirculation queue. */
+struct oftrace_recirc_node {
+ struct ovs_list node; /* In recirc_queue. */
+
+ enum oftrace_recirc_type type;
+ uint32_t recirc_id;
+ struct flow *flow;
+};
+
void ofproto_dpif_trace_init(void);
struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,
const char *text);
+void oftrace_add_recirc_node(struct ovs_list *recirc_queue,
+ enum oftrace_recirc_type, struct flow *,
+ uint32_t recirc_id);
#endif /* ofproto-dpif-trace.h */
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 48c4bad4ac0b..dbe10e4a1b37 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4356,9 +4356,14 @@ emit_continuation(struct xlate_ctx *ctx, const struct frozen_state *state)
}
}
-static void
+/* Create a forzen state, and allocate a unique recirc id for the given state.
+ * Returns a non-zero value if recirc id is allocated succesfully. Returns 0
+ * otherwise.
+ **/
+static uint32_t
finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
{
+ uint32_t id = 0;
ovs_assert(ctx->freezing);
struct frozen_state state = {
@@ -4385,11 +4390,11 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
* recirculation context, will be returned if possible.
* The life-cycle of this recirc id is managed by associating it
* with the udpif key ('ukey') created for each new datapath flow. */
- uint32_t id = recirc_alloc_id_ctx(&state);
+ id = recirc_alloc_id_ctx(&state);
if (!id) {
xlate_report_error(ctx, "Failed to allocate recirculation id");
ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
- return;
+ return 0;
}
recirc_refs_add(&ctx->xout->recircs, id);
@@ -4408,6 +4413,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
/* Undo changes done by freezing. */
ctx_cancel_freeze(ctx);
+ return id;
}
/* Called only when we're freezing. */
@@ -4425,8 +4431,17 @@ finish_freezing(struct xlate_ctx *ctx)
static void
compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table)
{
+ uint32_t recirc_id;
ctx->freezing = true;
- finish_freezing__(ctx, table);
+ recirc_id = finish_freezing__(ctx, table);
+
+ if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) {
+ xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to "
+ "recirculation. The forked pipeline will be resumed at "
+ "table %u.", table);
+ oftrace_add_recirc_node(ctx->xin->recirc_queue, OFT_CONNTRACK,
+ &ctx->xin->flow, recirc_id);
+ }
Was there a reason this is not done in finish_freezing__() itself ?
}
static void
@@ -5970,6 +5985,7 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto,
xin->wc = wc;
xin->odp_actions = odp_actions;
xin->in_packet_out = false;
+ xin->recirc_queue = NULL;
/* Do recirc lookup. */
xin->frozen_state = NULL;
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 68e114afb9ae..a86d5c6c9862 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -158,6 +158,10 @@ struct xlate_in {
/* If true, the packet to be translated is from a packet_out msg. */
bool in_packet_out;
+
+ /* ofproto/trace maintains this queue to trace flows that requires
+ * recirculation. */
+ struct ovs_list *recirc_queue;
};
void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index e2228661fd09..c51c689b9f46 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -9710,6 +9710,41 @@ udp,vlan_tci=0x0000,dl_src=50:54:00:00:00:0a,dl_dst=50:54:00:00:00:09,nw_src=10.
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - conntrack - ofproto/trace])
+OVS_VSWITCHD_START
+
+add_of_ports br0 1 2
+
+AT_DATA([flows.txt], [dnl
+dnl Table 0
+dnl
+table=0,priority=100,arp,action=normal
+table=0,priority=10,udp,action=ct(table=1,zone=0)
+table=0,priority=1,action=drop
+dnl
+dnl Table 1
+dnl
+table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2
+table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2
+table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1
+table=1,priority=1,action=drop
+])
+
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=2,udp'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+ [Datapath actions: 1
+])
+
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,udp'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+ [Datapath actions: 2
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto - set mtu])
OVS_VSWITCHD_START
--
2.7.4
_______________________________________________
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=QOZanmWX0YHWiBy9aCXt-fJrmiZ8cArSz7hCi1ycHJ0&s=rrSI6U4YywzDuey9wOT0zollHWtE25fMnp3wfjL_GCE&e=
On 21 June 2017 at 11:06, Yi-Hung Wei <yihung.wei@gmail.com> 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 est in the automatic recirculation process. A following patch > will provide an option to customize ct_state. > > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> > --- Hi YiHung, thanks for the patch! Looks really useful. A few comments below. > ofproto/ofproto-dpif-trace.c | 88 +++++++++++++++++++++++++++++++++++++------- > ofproto/ofproto-dpif-trace.h | 22 +++++++++++ > ofproto/ofproto-dpif-xlate.c | 24 ++++++++++-- > ofproto/ofproto-dpif-xlate.h | 4 ++ > tests/ofproto-dpif.at | 35 ++++++++++++++++++ > 5 files changed, 156 insertions(+), 17 deletions(-) > > diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c > index c7f0e48caa16..d8cdd92493cf 100644 > --- a/ofproto/ofproto-dpif-trace.c > +++ b/ofproto/ofproto-dpif-trace.c > @@ -86,6 +86,39 @@ oftrace_node_destroy(struct oftrace_node *node) > } > } > > +void > +oftrace_add_recirc_node(struct ovs_list *recirc_queue, > + enum oftrace_recirc_type type, struct flow *flow, > + uint32_t recirc_id) > +{ > + struct oftrace_recirc_node *node = xmalloc(sizeof *node); > + ovs_list_push_back(recirc_queue, &node->node); > + > + node->type = type; > + node->recirc_id = recirc_id; > + node->flow = xmalloc(sizeof *flow); > + memcpy(node->flow, flow, sizeof *flow); The above two lines could be an xmemdup() call. > + node->flow->recirc_id = recirc_id; > + > + if (type != OFT_INIT) { > + struct recirc_id_node *rid_node = CONST_CAST( > + struct recirc_id_node *, recirc_id_node_find(recirc_id)); > + if (rid_node) { > + ovs_refcount_try_ref_rcu(&rid_node->refcount); I wonder if there should be a recirc_id_node_find_and_ref() function which does the above, then returns the node if found (or atleast a bool true/false of whether the ref worked)..... > + } > + } > +} > + > +static void > +oftrace_recirc_node_destroy(struct oftrace_recirc_node *node) > +{ > + if (node->type != OFT_INIT) { > + recirc_free_id(node->recirc_id); ...then we can guarantee that the recirc_free_id() call is balanced here. (In other words, what happens if 'rid_node' couldn't be found in the code above? or try_ref_rcu failed?) <snip> > diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h > index e3d3b393cec8..f7299fd42848 100644 > --- a/ofproto/ofproto-dpif-trace.h > +++ b/ofproto/ofproto-dpif-trace.h > @@ -45,6 +45,16 @@ enum oftrace_node_type { > OFT_ERROR, /* An erroneous situation, worth logging. */ > }; > > +/* Type of a node within a recirculation queue. */ > +enum oftrace_recirc_type { > + /* The initial flow to be traced. */ > + OFT_INIT, > + /* A flow recirculates because of the following actions. */ > + OFT_CONNTRACK, > + OFT_MPLS, > + OFT_BOND, > +}; You might consider prefixing this enum with something other than OFT_ to reduce confusion between the enums and usage, since oftrace_node_type already uses the prefix OFT_. ... > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 48c4bad4ac0b..dbe10e4a1b37 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -4356,9 +4356,14 @@ emit_continuation(struct xlate_ctx *ctx, const struct frozen_state *state) > } > } > > -static void > +/* Create a forzen state, and allocate a unique recirc id for the given state. frozen. > + * Returns a non-zero value if recirc id is allocated succesfully. Returns 0 successfully. <snip> > diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h > index 68e114afb9ae..a86d5c6c9862 100644 > --- a/ofproto/ofproto-dpif-xlate.h > +++ b/ofproto/ofproto-dpif-xlate.h > @@ -158,6 +158,10 @@ struct xlate_in { > > /* If true, the packet to be translated is from a packet_out msg. */ > bool in_packet_out; > + > + /* ofproto/trace maintains this queue to trace flows that requires require.
On Wed, Jun 21, 2017 at 11:58 AM, Darrell Ball <dball@vmware.com> wrote: > > > On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> 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 est in the automatic recirculation process. > > Why is ‘est’ part of the default ? Please find my reply in the mail of PATCH 3/3. > > A following patch > will provide an option to customize ct_state. > > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> > --- > ofproto/ofproto-dpif-trace.c | 88 +++++++++++++++++++++++++++++++++++++------- > ofproto/ofproto-dpif-trace.h | 22 +++++++++++ > ofproto/ofproto-dpif-xlate.c | 24 ++++++++++-- > ofproto/ofproto-dpif-xlate.h | 4 ++ > tests/ofproto-dpif.at | 35 ++++++++++++++++++ > 5 files changed, 156 insertions(+), 17 deletions(-) > > diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c > index c7f0e48caa16..d8cdd92493cf 100644 > --- a/ofproto/ofproto-dpif-trace.c > +++ b/ofproto/ofproto-dpif-trace.c > @@ -86,6 +86,39 @@ oftrace_node_destroy(struct oftrace_node *node) > } > } > > +void > +oftrace_add_recirc_node(struct ovs_list *recirc_queue, > + enum oftrace_recirc_type type, struct flow *flow, > + uint32_t recirc_id) > +{ > + struct oftrace_recirc_node *node = xmalloc(sizeof *node); > + ovs_list_push_back(recirc_queue, &node->node); > + > + node->type = type; > + node->recirc_id = recirc_id; > + node->flow = xmalloc(sizeof *flow); > + memcpy(node->flow, flow, sizeof *flow); > + node->flow->recirc_id = recirc_id; > + > + if (type != OFT_INIT) { > + struct recirc_id_node *rid_node = CONST_CAST( > + struct recirc_id_node *, recirc_id_node_find(recirc_id)); > + if (rid_node) { > + ovs_refcount_try_ref_rcu(&rid_node->refcount); > + } > + } > +} > + > +static void > +oftrace_recirc_node_destroy(struct oftrace_recirc_node *node) > +{ > + if (node->type != OFT_INIT) { > + recirc_free_id(node->recirc_id); > + } > + free(node->flow); > + free(node); > +} > + > static void > oftrace_node_print_details(struct ds *output, > const struct ovs_list *nodes, int level) > @@ -419,20 +452,11 @@ exit: > ofpbuf_uninit(&ofpacts); > } > > -/* Implements a "trace" through 'ofproto''s flow table, appending a textual > - * description of the results to 'output'. > - * > - * The trace follows a packet with the specified 'flow' through the flow > - * table. 'packet' may be nonnull to trace an actual packet, with consequent > - * side effects (if it is nonnull then its flow must be '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, > - const struct dp_packet *packet, > - const struct ofpact ofpacts[], size_t ofpacts_len, > - struct ds *output) > +ofproto_trace__(struct ofproto_dpif *ofproto, struct flow *flow, > + const struct dp_packet *packet, struct ovs_list *recirc_queue, > + const struct ofpact ofpacts[], size_t ofpacts_len, > + struct ds *output) > { > struct ofpbuf odp_actions; > ofpbuf_init(&odp_actions, 0); > @@ -447,6 +471,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow, > xin.ofpacts = ofpacts; > xin.ofpacts_len = ofpacts_len; > xin.trace = &trace; > + xin.recirc_queue = recirc_queue; > > /* Copy initial flow out of xin.flow. It differs from '*flow' because > * xlate_in_init() initializes actset_output to OFPP_UNSET. */ > @@ -503,6 +528,43 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow, > oftrace_node_list_destroy(&trace); > } > > +/* Implements a "trace" through 'ofproto''s flow table, appending a textual > + * description of the results to 'output'. > + * > + * The trace follows a packet with the specified 'flow' through the flow > + * table. 'packet' may be nonnull to trace an actual packet, with consequent > + * side effects (if it is nonnull then its flow must be '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, > + 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_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); > + > + if (recirc_node->type == OFT_CONNTRACK) { > + recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED; > + ds_put_cstr(output, "\n\nResume conntrack prcessing with " > + "default ct_state=trk|est.\n"); > + ds_put_char_multiple(output, '-', 79); > + ds_put_char(output, '\n'); > + } > + ofproto_trace__(ofproto, recirc_node->flow, packet, &recirc_queue, > + ofpacts, ofpacts_len, output); > + oftrace_recirc_node_destroy(recirc_node); > + } > +} > + > void > ofproto_dpif_trace_init(void) > { > diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h > index e3d3b393cec8..f7299fd42848 100644 > --- a/ofproto/ofproto-dpif-trace.h > +++ b/ofproto/ofproto-dpif-trace.h > @@ -45,6 +45,16 @@ enum oftrace_node_type { > OFT_ERROR, /* An erroneous situation, worth logging. */ > }; > > +/* Type of a node within a recirculation queue. */ > +enum oftrace_recirc_type { > + /* The initial flow to be traced. */ > + OFT_INIT, > + /* A flow recirculates because of the following actions. */ > + OFT_CONNTRACK, > + OFT_MPLS, > + OFT_BOND, > +}; > + > /* A node within a trace. */ > struct oftrace_node { > struct ovs_list node; /* In parent. */ > @@ -54,9 +64,21 @@ struct oftrace_node { > char *text; > }; > > +/* A node within a recirculation queue. */ > +struct oftrace_recirc_node { > + struct ovs_list node; /* In recirc_queue. */ > + > + enum oftrace_recirc_type type; > + uint32_t recirc_id; > + struct flow *flow; > +}; > + > void ofproto_dpif_trace_init(void); > > struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type, > const char *text); > +void oftrace_add_recirc_node(struct ovs_list *recirc_queue, > + enum oftrace_recirc_type, struct flow *, > + uint32_t recirc_id); > > #endif /* ofproto-dpif-trace.h */ > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 48c4bad4ac0b..dbe10e4a1b37 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -4356,9 +4356,14 @@ emit_continuation(struct xlate_ctx *ctx, const struct frozen_state *state) > } > } > > -static void > +/* Create a forzen state, and allocate a unique recirc id for the given state. > + * Returns a non-zero value if recirc id is allocated succesfully. Returns 0 > + * otherwise. > + **/ > +static uint32_t > finish_freezing__(struct xlate_ctx *ctx, uint8_t table) > { > + uint32_t id = 0; > ovs_assert(ctx->freezing); > > struct frozen_state state = { > @@ -4385,11 +4390,11 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table) > * recirculation context, will be returned if possible. > * The life-cycle of this recirc id is managed by associating it > * with the udpif key ('ukey') created for each new datapath flow. */ > - uint32_t id = recirc_alloc_id_ctx(&state); > + id = recirc_alloc_id_ctx(&state); > if (!id) { > xlate_report_error(ctx, "Failed to allocate recirculation id"); > ctx->error = XLATE_NO_RECIRCULATION_CONTEXT; > - return; > + return 0; > } > recirc_refs_add(&ctx->xout->recircs, id); > > @@ -4408,6 +4413,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table) > > /* Undo changes done by freezing. */ > ctx_cancel_freeze(ctx); > + return id; > } > > /* Called only when we're freezing. */ > @@ -4425,8 +4431,17 @@ finish_freezing(struct xlate_ctx *ctx) > static void > compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) > { > + uint32_t recirc_id; > ctx->freezing = true; > - finish_freezing__(ctx, table); > + recirc_id = finish_freezing__(ctx, table); > + > + if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) { > + xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to " > + "recirculation. The forked pipeline will be resumed at " > + "table %u.", table); > + oftrace_add_recirc_node(ctx->xin->recirc_queue, OFT_CONNTRACK, > + &ctx->xin->flow, recirc_id); > + } > > Was there a reason this is not done in finish_freezing__() itself ? It is because: 1) currently this patch only handles the recirculation of conntrack, maybe it makes sense to do xlate_report() in finish_freezing__() when the other recric cases are all being taking care of similarly later on. 2) The packet processing pipeline forked in conntrack recirc. For the other cases that triggers recirc, it may not fork the packet processing pipeline. > > } > > static void > @@ -5970,6 +5985,7 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto, > xin->wc = wc; > xin->odp_actions = odp_actions; > xin->in_packet_out = false; > + xin->recirc_queue = NULL; > > /* Do recirc lookup. */ > xin->frozen_state = NULL; > diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h > index 68e114afb9ae..a86d5c6c9862 100644 > --- a/ofproto/ofproto-dpif-xlate.h > +++ b/ofproto/ofproto-dpif-xlate.h > @@ -158,6 +158,10 @@ struct xlate_in { > > /* If true, the packet to be translated is from a packet_out msg. */ > bool in_packet_out; > + > + /* ofproto/trace maintains this queue to trace flows that requires > + * recirculation. */ > + struct ovs_list *recirc_queue; > }; > > void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *, > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index e2228661fd09..c51c689b9f46 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -9710,6 +9710,41 @@ udp,vlan_tci=0x0000,dl_src=50:54:00:00:00:0a,dl_dst=50:54:00:00:00:09,nw_src=10. > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([ofproto-dpif - conntrack - ofproto/trace]) > +OVS_VSWITCHD_START > + > +add_of_ports br0 1 2 > + > +AT_DATA([flows.txt], [dnl > +dnl Table 0 > +dnl > +table=0,priority=100,arp,action=normal > +table=0,priority=10,udp,action=ct(table=1,zone=0) > +table=0,priority=1,action=drop > +dnl > +dnl Table 1 > +dnl > +table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2 > +table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2 > +table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1 > +table=1,priority=1,action=drop > +]) > + > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=2,udp'], [0], [stdout]) > +AT_CHECK([tail -1 stdout], [0], > + [Datapath actions: 1 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,udp'], [0], [stdout]) > +AT_CHECK([tail -1 stdout], [0], > + [Datapath actions: 2 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([ofproto - set mtu]) > OVS_VSWITCHD_START > > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=QOZanmWX0YHWiBy9aCXt-fJrmiZ8cArSz7hCi1ycHJ0&s=rrSI6U4YywzDuey9wOT0zollHWtE25fMnp3wfjL_GCE&e= > >
On Wed, Jun 21, 2017 at 12:47 PM, Joe Stringer <joe@ovn.org> wrote: > On 21 June 2017 at 11:06, Yi-Hung Wei <yihung.wei@gmail.com> 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 est in the automatic recirculation process. A following patch >> will provide an option to customize ct_state. >> >> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> >> --- > > Hi YiHung, thanks for the patch! Looks really useful. > > A few comments below. > >> ofproto/ofproto-dpif-trace.c | 88 +++++++++++++++++++++++++++++++++++++------- >> ofproto/ofproto-dpif-trace.h | 22 +++++++++++ >> ofproto/ofproto-dpif-xlate.c | 24 ++++++++++-- >> ofproto/ofproto-dpif-xlate.h | 4 ++ >> tests/ofproto-dpif.at | 35 ++++++++++++++++++ >> 5 files changed, 156 insertions(+), 17 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c >> index c7f0e48caa16..d8cdd92493cf 100644 >> --- a/ofproto/ofproto-dpif-trace.c >> +++ b/ofproto/ofproto-dpif-trace.c >> @@ -86,6 +86,39 @@ oftrace_node_destroy(struct oftrace_node *node) >> } >> } >> >> +void >> +oftrace_add_recirc_node(struct ovs_list *recirc_queue, >> + enum oftrace_recirc_type type, struct flow *flow, >> + uint32_t recirc_id) >> +{ >> + struct oftrace_recirc_node *node = xmalloc(sizeof *node); >> + ovs_list_push_back(recirc_queue, &node->node); >> + >> + node->type = type; >> + node->recirc_id = recirc_id; >> + node->flow = xmalloc(sizeof *flow); >> + memcpy(node->flow, flow, sizeof *flow); > > The above two lines could be an xmemdup() call. Thanks for review. Sure, it make sense to use xmemdup(). > >> + node->flow->recirc_id = recirc_id; >> + >> + if (type != OFT_INIT) { >> + struct recirc_id_node *rid_node = CONST_CAST( >> + struct recirc_id_node *, recirc_id_node_find(recirc_id)); >> + if (rid_node) { >> + ovs_refcount_try_ref_rcu(&rid_node->refcount); > > I wonder if there should be a recirc_id_node_find_and_ref() function > which does the above, then returns the node if found (or atleast a > bool true/false of whether the ref worked)..... Yes, I added bool recirc_id_node_find_and_ref() and make changes accrodingly in v2. > >> + } >> + } >> +} >> + >> +static void >> +oftrace_recirc_node_destroy(struct oftrace_recirc_node *node) >> +{ >> + if (node->type != OFT_INIT) { >> + recirc_free_id(node->recirc_id); > > ...then we can guarantee that the recirc_free_id() call is balanced > here. (In other words, what happens if 'rid_node' couldn't be found in > the code above? or try_ref_rcu failed?) > > <snip> > >> diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h >> index e3d3b393cec8..f7299fd42848 100644 >> --- a/ofproto/ofproto-dpif-trace.h >> +++ b/ofproto/ofproto-dpif-trace.h >> @@ -45,6 +45,16 @@ enum oftrace_node_type { >> OFT_ERROR, /* An erroneous situation, worth logging. */ >> }; >> >> +/* Type of a node within a recirculation queue. */ >> +enum oftrace_recirc_type { >> + /* The initial flow to be traced. */ >> + OFT_INIT, >> + /* A flow recirculates because of the following actions. */ >> + OFT_CONNTRACK, >> + OFT_MPLS, >> + OFT_BOND, >> +}; > > You might consider prefixing this enum with something other than OFT_ > to reduce confusion between the enums and usage, since > oftrace_node_type already uses the prefix OFT_. > > ... Thanks, I replace OFT_ with OFT_RECIRC_ in v2. > >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index 48c4bad4ac0b..dbe10e4a1b37 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -4356,9 +4356,14 @@ emit_continuation(struct xlate_ctx *ctx, const struct frozen_state *state) >> } >> } >> >> -static void >> +/* Create a forzen state, and allocate a unique recirc id for the given state. > > frozen. Done. > >> + * Returns a non-zero value if recirc id is allocated succesfully. Returns 0 > > successfully. Done. > > <snip> > >> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h >> index 68e114afb9ae..a86d5c6c9862 100644 >> --- a/ofproto/ofproto-dpif-xlate.h >> +++ b/ofproto/ofproto-dpif-xlate.h >> @@ -158,6 +158,10 @@ struct xlate_in { >> >> /* If true, the packet to be translated is from a packet_out msg. */ >> bool in_packet_out; >> + >> + /* ofproto/trace maintains this queue to trace flows that requires > > require. Sure.
On 6/26/17, 10:18 AM, "Yi-Hung Wei" <yihung.wei@gmail.com> wrote:
On Wed, Jun 21, 2017 at 11:58 AM, Darrell Ball <dball@vmware.com> wrote:
>
>
> On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> 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 est in the automatic recirculation process.
>
> Why is ‘est’ part of the default ?
Please find my reply in the mail of PATCH 3/3.
>
> A following patch
> will provide an option to customize ct_state.
>
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---
> ofproto/ofproto-dpif-trace.c | 88 +++++++++++++++++++++++++++++++++++++-------
> ofproto/ofproto-dpif-trace.h | 22 +++++++++++
> ofproto/ofproto-dpif-xlate.c | 24 ++++++++++--
> ofproto/ofproto-dpif-xlate.h | 4 ++
> tests/ofproto-dpif.at | 35 ++++++++++++++++++
> 5 files changed, 156 insertions(+), 17 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index c7f0e48caa16..d8cdd92493cf 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -86,6 +86,39 @@ oftrace_node_destroy(struct oftrace_node *node)
> }
> }
>
> +void
> +oftrace_add_recirc_node(struct ovs_list *recirc_queue,
> + enum oftrace_recirc_type type, struct flow *flow,
> + uint32_t recirc_id)
> +{
> + struct oftrace_recirc_node *node = xmalloc(sizeof *node);
> + ovs_list_push_back(recirc_queue, &node->node);
> +
> + node->type = type;
> + node->recirc_id = recirc_id;
> + node->flow = xmalloc(sizeof *flow);
> + memcpy(node->flow, flow, sizeof *flow);
> + node->flow->recirc_id = recirc_id;
> +
> + if (type != OFT_INIT) {
> + struct recirc_id_node *rid_node = CONST_CAST(
> + struct recirc_id_node *, recirc_id_node_find(recirc_id));
> + if (rid_node) {
> + ovs_refcount_try_ref_rcu(&rid_node->refcount);
> + }
> + }
> +}
> +
> +static void
> +oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)
> +{
> + if (node->type != OFT_INIT) {
> + recirc_free_id(node->recirc_id);
> + }
> + free(node->flow);
> + free(node);
> +}
> +
> static void
> oftrace_node_print_details(struct ds *output,
> const struct ovs_list *nodes, int level)
> @@ -419,20 +452,11 @@ exit:
> ofpbuf_uninit(&ofpacts);
> }
>
> -/* Implements a "trace" through 'ofproto''s flow table, appending a textual
> - * description of the results to 'output'.
> - *
> - * The trace follows a packet with the specified 'flow' through the flow
> - * table. 'packet' may be nonnull to trace an actual packet, with consequent
> - * side effects (if it is nonnull then its flow must be '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,
> - const struct dp_packet *packet,
> - const struct ofpact ofpacts[], size_t ofpacts_len,
> - struct ds *output)
> +ofproto_trace__(struct ofproto_dpif *ofproto, struct flow *flow,
> + const struct dp_packet *packet, struct ovs_list *recirc_queue,
> + const struct ofpact ofpacts[], size_t ofpacts_len,
> + struct ds *output)
> {
> struct ofpbuf odp_actions;
> ofpbuf_init(&odp_actions, 0);
> @@ -447,6 +471,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
> xin.ofpacts = ofpacts;
> xin.ofpacts_len = ofpacts_len;
> xin.trace = &trace;
> + xin.recirc_queue = recirc_queue;
>
> /* Copy initial flow out of xin.flow. It differs from '*flow' because
> * xlate_in_init() initializes actset_output to OFPP_UNSET. */
> @@ -503,6 +528,43 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
> oftrace_node_list_destroy(&trace);
> }
>
> +/* Implements a "trace" through 'ofproto''s flow table, appending a textual
> + * description of the results to 'output'.
> + *
> + * The trace follows a packet with the specified 'flow' through the flow
> + * table. 'packet' may be nonnull to trace an actual packet, with consequent
> + * side effects (if it is nonnull then its flow must be '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,
> + 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_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);
> +
> + if (recirc_node->type == OFT_CONNTRACK) {
> + recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
> + ds_put_cstr(output, "\n\nResume conntrack prcessing with "
> + "default ct_state=trk|est.\n");
> + ds_put_char_multiple(output, '-', 79);
> + ds_put_char(output, '\n');
> + }
> + ofproto_trace__(ofproto, recirc_node->flow, packet, &recirc_queue,
> + ofpacts, ofpacts_len, output);
> + oftrace_recirc_node_destroy(recirc_node);
> + }
> +}
> +
> void
> ofproto_dpif_trace_init(void)
> {
> diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h
> index e3d3b393cec8..f7299fd42848 100644
> --- a/ofproto/ofproto-dpif-trace.h
> +++ b/ofproto/ofproto-dpif-trace.h
> @@ -45,6 +45,16 @@ enum oftrace_node_type {
> OFT_ERROR, /* An erroneous situation, worth logging. */
> };
>
> +/* Type of a node within a recirculation queue. */
> +enum oftrace_recirc_type {
> + /* The initial flow to be traced. */
> + OFT_INIT,
> + /* A flow recirculates because of the following actions. */
> + OFT_CONNTRACK,
> + OFT_MPLS,
> + OFT_BOND,
> +};
> +
> /* A node within a trace. */
> struct oftrace_node {
> struct ovs_list node; /* In parent. */
> @@ -54,9 +64,21 @@ struct oftrace_node {
> char *text;
> };
>
> +/* A node within a recirculation queue. */
> +struct oftrace_recirc_node {
> + struct ovs_list node; /* In recirc_queue. */
> +
> + enum oftrace_recirc_type type;
> + uint32_t recirc_id;
> + struct flow *flow;
> +};
> +
> void ofproto_dpif_trace_init(void);
>
> struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,
> const char *text);
> +void oftrace_add_recirc_node(struct ovs_list *recirc_queue,
> + enum oftrace_recirc_type, struct flow *,
> + uint32_t recirc_id);
>
> #endif /* ofproto-dpif-trace.h */
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 48c4bad4ac0b..dbe10e4a1b37 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4356,9 +4356,14 @@ emit_continuation(struct xlate_ctx *ctx, const struct frozen_state *state)
> }
> }
>
> -static void
> +/* Create a forzen state, and allocate a unique recirc id for the given state.
> + * Returns a non-zero value if recirc id is allocated succesfully. Returns 0
> + * otherwise.
> + **/
> +static uint32_t
> finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
> {
> + uint32_t id = 0;
> ovs_assert(ctx->freezing);
>
> struct frozen_state state = {
> @@ -4385,11 +4390,11 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
> * recirculation context, will be returned if possible.
> * The life-cycle of this recirc id is managed by associating it
> * with the udpif key ('ukey') created for each new datapath flow. */
> - uint32_t id = recirc_alloc_id_ctx(&state);
> + id = recirc_alloc_id_ctx(&state);
> if (!id) {
> xlate_report_error(ctx, "Failed to allocate recirculation id");
> ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
> - return;
> + return 0;
> }
> recirc_refs_add(&ctx->xout->recircs, id);
>
> @@ -4408,6 +4413,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
>
> /* Undo changes done by freezing. */
> ctx_cancel_freeze(ctx);
> + return id;
> }
>
> /* Called only when we're freezing. */
> @@ -4425,8 +4431,17 @@ finish_freezing(struct xlate_ctx *ctx)
> static void
> compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table)
> {
> + uint32_t recirc_id;
> ctx->freezing = true;
> - finish_freezing__(ctx, table);
> + recirc_id = finish_freezing__(ctx, table);
> +
> + if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) {
> + xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to "
> + "recirculation. The forked pipeline will be resumed at "
> + "table %u.", table);
> + oftrace_add_recirc_node(ctx->xin->recirc_queue, OFT_CONNTRACK,
> + &ctx->xin->flow, recirc_id);
> + }
>
> Was there a reason this is not done in finish_freezing__() itself ?
It is because:
1) currently this patch only handles the recirculation of conntrack,
maybe it makes sense to do xlate_report() in finish_freezing__() when
the other recric cases are all being taking care of similarly later
on.
So your plan is to put the code in compose_recirculate_and_fork()
and then later add other cases like bonding/mpls and then maybe move
the code to finish_freezing__ with another patch ?
I just thought it is less code changes to put xlate_report() in finish_freezing__
since you don’t have to communicate b/w functions. Maybe, I missed something.
2) The packet processing pipeline forked in conntrack recirc. For the
other cases that triggers recirc, it may not fork the packet
processing pipeline.
I am not quite following here; let me discuss offline and come back to the alias.
>
> }
>
> static void
> @@ -5970,6 +5985,7 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto,
> xin->wc = wc;
> xin->odp_actions = odp_actions;
> xin->in_packet_out = false;
> + xin->recirc_queue = NULL;
>
> /* Do recirc lookup. */
> xin->frozen_state = NULL;
> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
> index 68e114afb9ae..a86d5c6c9862 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -158,6 +158,10 @@ struct xlate_in {
>
> /* If true, the packet to be translated is from a packet_out msg. */
> bool in_packet_out;
> +
> + /* ofproto/trace maintains this queue to trace flows that requires
> + * recirculation. */
> + struct ovs_list *recirc_queue;
> };
>
> void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *,
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index e2228661fd09..c51c689b9f46 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -9710,6 +9710,41 @@ udp,vlan_tci=0x0000,dl_src=50:54:00:00:00:0a,dl_dst=50:54:00:00:00:09,nw_src=10.
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - conntrack - ofproto/trace])
> +OVS_VSWITCHD_START
> +
> +add_of_ports br0 1 2
> +
> +AT_DATA([flows.txt], [dnl
> +dnl Table 0
> +dnl
> +table=0,priority=100,arp,action=normal
> +table=0,priority=10,udp,action=ct(table=1,zone=0)
> +table=0,priority=1,action=drop
> +dnl
> +dnl Table 1
> +dnl
> +table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2
> +table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2
> +table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1
> +table=1,priority=1,action=drop
> +])
> +
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=2,udp'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0],
> + [Datapath actions: 1
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,udp'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0],
> + [Datapath actions: 2
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([ofproto - set mtu])
> OVS_VSWITCHD_START
>
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=QOZanmWX0YHWiBy9aCXt-fJrmiZ8cArSz7hCi1ycHJ0&s=rrSI6U4YywzDuey9wOT0zollHWtE25fMnp3wfjL_GCE&e=
>
>
On 6/26/17, 11:05 AM, "Darrell Ball" <dball@vmware.com> wrote:
On 6/26/17, 10:18 AM, "Yi-Hung Wei" <yihung.wei@gmail.com> wrote:
On Wed, Jun 21, 2017 at 11:58 AM, Darrell Ball <dball@vmware.com> wrote:
>
>
> On 6/21/17, 11:06 AM, "ovs-dev-bounces@openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces@openvswitch.org on behalf of yihung.wei@gmail.com> 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 est in the automatic recirculation process.
>
> Why is ‘est’ part of the default ?
Please find my reply in the mail of PATCH 3/3.
>
> A following patch
> will provide an option to customize ct_state.
>
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---
> ofproto/ofproto-dpif-trace.c | 88 +++++++++++++++++++++++++++++++++++++-------
> ofproto/ofproto-dpif-trace.h | 22 +++++++++++
> ofproto/ofproto-dpif-xlate.c | 24 ++++++++++--
> ofproto/ofproto-dpif-xlate.h | 4 ++
> tests/ofproto-dpif.at | 35 ++++++++++++++++++
> 5 files changed, 156 insertions(+), 17 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index c7f0e48caa16..d8cdd92493cf 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -86,6 +86,39 @@ oftrace_node_destroy(struct oftrace_node *node)
> }
> }
>
> +void
> +oftrace_add_recirc_node(struct ovs_list *recirc_queue,
> + enum oftrace_recirc_type type, struct flow *flow,
> + uint32_t recirc_id)
> +{
> + struct oftrace_recirc_node *node = xmalloc(sizeof *node);
> + ovs_list_push_back(recirc_queue, &node->node);
> +
> + node->type = type;
> + node->recirc_id = recirc_id;
> + node->flow = xmalloc(sizeof *flow);
> + memcpy(node->flow, flow, sizeof *flow);
> + node->flow->recirc_id = recirc_id;
> +
> + if (type != OFT_INIT) {
> + struct recirc_id_node *rid_node = CONST_CAST(
> + struct recirc_id_node *, recirc_id_node_find(recirc_id));
> + if (rid_node) {
> + ovs_refcount_try_ref_rcu(&rid_node->refcount);
> + }
> + }
> +}
> +
> +static void
> +oftrace_recirc_node_destroy(struct oftrace_recirc_node *node)
> +{
> + if (node->type != OFT_INIT) {
> + recirc_free_id(node->recirc_id);
> + }
> + free(node->flow);
> + free(node);
> +}
> +
> static void
> oftrace_node_print_details(struct ds *output,
> const struct ovs_list *nodes, int level)
> @@ -419,20 +452,11 @@ exit:
> ofpbuf_uninit(&ofpacts);
> }
>
> -/* Implements a "trace" through 'ofproto''s flow table, appending a textual
> - * description of the results to 'output'.
> - *
> - * The trace follows a packet with the specified 'flow' through the flow
> - * table. 'packet' may be nonnull to trace an actual packet, with consequent
> - * side effects (if it is nonnull then its flow must be '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,
> - const struct dp_packet *packet,
> - const struct ofpact ofpacts[], size_t ofpacts_len,
> - struct ds *output)
> +ofproto_trace__(struct ofproto_dpif *ofproto, struct flow *flow,
> + const struct dp_packet *packet, struct ovs_list *recirc_queue,
> + const struct ofpact ofpacts[], size_t ofpacts_len,
> + struct ds *output)
> {
> struct ofpbuf odp_actions;
> ofpbuf_init(&odp_actions, 0);
> @@ -447,6 +471,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
> xin.ofpacts = ofpacts;
> xin.ofpacts_len = ofpacts_len;
> xin.trace = &trace;
> + xin.recirc_queue = recirc_queue;
>
> /* Copy initial flow out of xin.flow. It differs from '*flow' because
> * xlate_in_init() initializes actset_output to OFPP_UNSET. */
> @@ -503,6 +528,43 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
> oftrace_node_list_destroy(&trace);
> }
>
> +/* Implements a "trace" through 'ofproto''s flow table, appending a textual
> + * description of the results to 'output'.
> + *
> + * The trace follows a packet with the specified 'flow' through the flow
> + * table. 'packet' may be nonnull to trace an actual packet, with consequent
> + * side effects (if it is nonnull then its flow must be '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,
> + 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_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);
> +
> + if (recirc_node->type == OFT_CONNTRACK) {
> + recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED;
> + ds_put_cstr(output, "\n\nResume conntrack prcessing with "
> + "default ct_state=trk|est.\n");
> + ds_put_char_multiple(output, '-', 79);
> + ds_put_char(output, '\n');
> + }
> + ofproto_trace__(ofproto, recirc_node->flow, packet, &recirc_queue,
> + ofpacts, ofpacts_len, output);
> + oftrace_recirc_node_destroy(recirc_node);
> + }
> +}
> +
> void
> ofproto_dpif_trace_init(void)
> {
> diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h
> index e3d3b393cec8..f7299fd42848 100644
> --- a/ofproto/ofproto-dpif-trace.h
> +++ b/ofproto/ofproto-dpif-trace.h
> @@ -45,6 +45,16 @@ enum oftrace_node_type {
> OFT_ERROR, /* An erroneous situation, worth logging. */
> };
>
> +/* Type of a node within a recirculation queue. */
> +enum oftrace_recirc_type {
> + /* The initial flow to be traced. */
> + OFT_INIT,
> + /* A flow recirculates because of the following actions. */
> + OFT_CONNTRACK,
> + OFT_MPLS,
> + OFT_BOND,
> +};
> +
> /* A node within a trace. */
> struct oftrace_node {
> struct ovs_list node; /* In parent. */
> @@ -54,9 +64,21 @@ struct oftrace_node {
> char *text;
> };
>
> +/* A node within a recirculation queue. */
> +struct oftrace_recirc_node {
> + struct ovs_list node; /* In recirc_queue. */
> +
> + enum oftrace_recirc_type type;
> + uint32_t recirc_id;
> + struct flow *flow;
> +};
> +
> void ofproto_dpif_trace_init(void);
>
> struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,
> const char *text);
> +void oftrace_add_recirc_node(struct ovs_list *recirc_queue,
> + enum oftrace_recirc_type, struct flow *,
> + uint32_t recirc_id);
>
> #endif /* ofproto-dpif-trace.h */
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 48c4bad4ac0b..dbe10e4a1b37 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4356,9 +4356,14 @@ emit_continuation(struct xlate_ctx *ctx, const struct frozen_state *state)
> }
> }
>
> -static void
> +/* Create a forzen state, and allocate a unique recirc id for the given state.
> + * Returns a non-zero value if recirc id is allocated succesfully. Returns 0
> + * otherwise.
> + **/
> +static uint32_t
> finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
> {
> + uint32_t id = 0;
> ovs_assert(ctx->freezing);
>
> struct frozen_state state = {
> @@ -4385,11 +4390,11 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
> * recirculation context, will be returned if possible.
> * The life-cycle of this recirc id is managed by associating it
> * with the udpif key ('ukey') created for each new datapath flow. */
> - uint32_t id = recirc_alloc_id_ctx(&state);
> + id = recirc_alloc_id_ctx(&state);
> if (!id) {
> xlate_report_error(ctx, "Failed to allocate recirculation id");
> ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
> - return;
> + return 0;
> }
> recirc_refs_add(&ctx->xout->recircs, id);
>
> @@ -4408,6 +4413,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table)
>
> /* Undo changes done by freezing. */
> ctx_cancel_freeze(ctx);
> + return id;
> }
>
> /* Called only when we're freezing. */
> @@ -4425,8 +4431,17 @@ finish_freezing(struct xlate_ctx *ctx)
> static void
> compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table)
> {
> + uint32_t recirc_id;
> ctx->freezing = true;
> - finish_freezing__(ctx, table);
> + recirc_id = finish_freezing__(ctx, table);
> +
> + if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) {
> + xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to "
> + "recirculation. The forked pipeline will be resumed at "
> + "table %u.", table);
> + oftrace_add_recirc_node(ctx->xin->recirc_queue, OFT_CONNTRACK,
> + &ctx->xin->flow, recirc_id);
> + }
>
> Was there a reason this is not done in finish_freezing__() itself ?
It is because:
1) currently this patch only handles the recirculation of conntrack,
maybe it makes sense to do xlate_report() in finish_freezing__() when
the other recric cases are all being taking care of similarly later
on.
So your plan is to put the code in compose_recirculate_and_fork()
and then later add other cases like bonding/mpls and then maybe move
the code to finish_freezing__ with another patch ?
I just thought it is less code changes to put xlate_report() in finish_freezing__
since you don’t have to communicate b/w functions. Maybe, I missed something.
2) The packet processing pipeline forked in conntrack recirc. For the
other cases that triggers recirc, it may not fork the packet
processing pipeline.
I am not quite following here; let me discuss offline and come back to the alias.
After discussing offline, I agree it is easier to add xlate_report() in compose_recirculate_and_fork()
because the text message context is easier to determine and further support for bonding/mpls may not
even be available for some time.
>
> }
>
> static void
> @@ -5970,6 +5985,7 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto,
> xin->wc = wc;
> xin->odp_actions = odp_actions;
> xin->in_packet_out = false;
> + xin->recirc_queue = NULL;
>
> /* Do recirc lookup. */
> xin->frozen_state = NULL;
> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
> index 68e114afb9ae..a86d5c6c9862 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -158,6 +158,10 @@ struct xlate_in {
>
> /* If true, the packet to be translated is from a packet_out msg. */
> bool in_packet_out;
> +
> + /* ofproto/trace maintains this queue to trace flows that requires
> + * recirculation. */
> + struct ovs_list *recirc_queue;
> };
>
> void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *,
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index e2228661fd09..c51c689b9f46 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -9710,6 +9710,41 @@ udp,vlan_tci=0x0000,dl_src=50:54:00:00:00:0a,dl_dst=50:54:00:00:00:09,nw_src=10.
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - conntrack - ofproto/trace])
> +OVS_VSWITCHD_START
> +
> +add_of_ports br0 1 2
> +
> +AT_DATA([flows.txt], [dnl
> +dnl Table 0
> +dnl
> +table=0,priority=100,arp,action=normal
> +table=0,priority=10,udp,action=ct(table=1,zone=0)
> +table=0,priority=1,action=drop
> +dnl
> +dnl Table 1
> +dnl
> +table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2
> +table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2
> +table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1
> +table=1,priority=1,action=drop
> +])
> +
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=2,udp'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0],
> + [Datapath actions: 1
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,udp'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0],
> + [Datapath actions: 2
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([ofproto - set mtu])
> OVS_VSWITCHD_START
>
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=QOZanmWX0YHWiBy9aCXt-fJrmiZ8cArSz7hCi1ycHJ0&s=rrSI6U4YywzDuey9wOT0zollHWtE25fMnp3wfjL_GCE&e=
>
>
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c index c7f0e48caa16..d8cdd92493cf 100644 --- a/ofproto/ofproto-dpif-trace.c +++ b/ofproto/ofproto-dpif-trace.c @@ -86,6 +86,39 @@ oftrace_node_destroy(struct oftrace_node *node) } } +void +oftrace_add_recirc_node(struct ovs_list *recirc_queue, + enum oftrace_recirc_type type, struct flow *flow, + uint32_t recirc_id) +{ + struct oftrace_recirc_node *node = xmalloc(sizeof *node); + ovs_list_push_back(recirc_queue, &node->node); + + node->type = type; + node->recirc_id = recirc_id; + node->flow = xmalloc(sizeof *flow); + memcpy(node->flow, flow, sizeof *flow); + node->flow->recirc_id = recirc_id; + + if (type != OFT_INIT) { + struct recirc_id_node *rid_node = CONST_CAST( + struct recirc_id_node *, recirc_id_node_find(recirc_id)); + if (rid_node) { + ovs_refcount_try_ref_rcu(&rid_node->refcount); + } + } +} + +static void +oftrace_recirc_node_destroy(struct oftrace_recirc_node *node) +{ + if (node->type != OFT_INIT) { + recirc_free_id(node->recirc_id); + } + free(node->flow); + free(node); +} + static void oftrace_node_print_details(struct ds *output, const struct ovs_list *nodes, int level) @@ -419,20 +452,11 @@ exit: ofpbuf_uninit(&ofpacts); } -/* Implements a "trace" through 'ofproto''s flow table, appending a textual - * description of the results to 'output'. - * - * The trace follows a packet with the specified 'flow' through the flow - * table. 'packet' may be nonnull to trace an actual packet, with consequent - * side effects (if it is nonnull then its flow must be '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, - const struct dp_packet *packet, - const struct ofpact ofpacts[], size_t ofpacts_len, - struct ds *output) +ofproto_trace__(struct ofproto_dpif *ofproto, struct flow *flow, + const struct dp_packet *packet, struct ovs_list *recirc_queue, + const struct ofpact ofpacts[], size_t ofpacts_len, + struct ds *output) { struct ofpbuf odp_actions; ofpbuf_init(&odp_actions, 0); @@ -447,6 +471,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow, xin.ofpacts = ofpacts; xin.ofpacts_len = ofpacts_len; xin.trace = &trace; + xin.recirc_queue = recirc_queue; /* Copy initial flow out of xin.flow. It differs from '*flow' because * xlate_in_init() initializes actset_output to OFPP_UNSET. */ @@ -503,6 +528,43 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow, oftrace_node_list_destroy(&trace); } +/* Implements a "trace" through 'ofproto''s flow table, appending a textual + * description of the results to 'output'. + * + * The trace follows a packet with the specified 'flow' through the flow + * table. 'packet' may be nonnull to trace an actual packet, with consequent + * side effects (if it is nonnull then its flow must be '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, + 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_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); + + if (recirc_node->type == OFT_CONNTRACK) { + recirc_node->flow->ct_state = CS_ESTABLISHED | CS_TRACKED; + ds_put_cstr(output, "\n\nResume conntrack prcessing with " + "default ct_state=trk|est.\n"); + ds_put_char_multiple(output, '-', 79); + ds_put_char(output, '\n'); + } + ofproto_trace__(ofproto, recirc_node->flow, packet, &recirc_queue, + ofpacts, ofpacts_len, output); + oftrace_recirc_node_destroy(recirc_node); + } +} + void ofproto_dpif_trace_init(void) { diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h index e3d3b393cec8..f7299fd42848 100644 --- a/ofproto/ofproto-dpif-trace.h +++ b/ofproto/ofproto-dpif-trace.h @@ -45,6 +45,16 @@ enum oftrace_node_type { OFT_ERROR, /* An erroneous situation, worth logging. */ }; +/* Type of a node within a recirculation queue. */ +enum oftrace_recirc_type { + /* The initial flow to be traced. */ + OFT_INIT, + /* A flow recirculates because of the following actions. */ + OFT_CONNTRACK, + OFT_MPLS, + OFT_BOND, +}; + /* A node within a trace. */ struct oftrace_node { struct ovs_list node; /* In parent. */ @@ -54,9 +64,21 @@ struct oftrace_node { char *text; }; +/* A node within a recirculation queue. */ +struct oftrace_recirc_node { + struct ovs_list node; /* In recirc_queue. */ + + enum oftrace_recirc_type type; + uint32_t recirc_id; + struct flow *flow; +}; + void ofproto_dpif_trace_init(void); struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type, const char *text); +void oftrace_add_recirc_node(struct ovs_list *recirc_queue, + enum oftrace_recirc_type, struct flow *, + uint32_t recirc_id); #endif /* ofproto-dpif-trace.h */ diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 48c4bad4ac0b..dbe10e4a1b37 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4356,9 +4356,14 @@ emit_continuation(struct xlate_ctx *ctx, const struct frozen_state *state) } } -static void +/* Create a forzen state, and allocate a unique recirc id for the given state. + * Returns a non-zero value if recirc id is allocated succesfully. Returns 0 + * otherwise. + **/ +static uint32_t finish_freezing__(struct xlate_ctx *ctx, uint8_t table) { + uint32_t id = 0; ovs_assert(ctx->freezing); struct frozen_state state = { @@ -4385,11 +4390,11 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table) * recirculation context, will be returned if possible. * The life-cycle of this recirc id is managed by associating it * with the udpif key ('ukey') created for each new datapath flow. */ - uint32_t id = recirc_alloc_id_ctx(&state); + id = recirc_alloc_id_ctx(&state); if (!id) { xlate_report_error(ctx, "Failed to allocate recirculation id"); ctx->error = XLATE_NO_RECIRCULATION_CONTEXT; - return; + return 0; } recirc_refs_add(&ctx->xout->recircs, id); @@ -4408,6 +4413,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table) /* Undo changes done by freezing. */ ctx_cancel_freeze(ctx); + return id; } /* Called only when we're freezing. */ @@ -4425,8 +4431,17 @@ finish_freezing(struct xlate_ctx *ctx) static void compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) { + uint32_t recirc_id; ctx->freezing = true; - finish_freezing__(ctx, table); + recirc_id = finish_freezing__(ctx, table); + + if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) { + xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to " + "recirculation. The forked pipeline will be resumed at " + "table %u.", table); + oftrace_add_recirc_node(ctx->xin->recirc_queue, OFT_CONNTRACK, + &ctx->xin->flow, recirc_id); + } } static void @@ -5970,6 +5985,7 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto, xin->wc = wc; xin->odp_actions = odp_actions; xin->in_packet_out = false; + xin->recirc_queue = NULL; /* Do recirc lookup. */ xin->frozen_state = NULL; diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h index 68e114afb9ae..a86d5c6c9862 100644 --- a/ofproto/ofproto-dpif-xlate.h +++ b/ofproto/ofproto-dpif-xlate.h @@ -158,6 +158,10 @@ struct xlate_in { /* If true, the packet to be translated is from a packet_out msg. */ bool in_packet_out; + + /* ofproto/trace maintains this queue to trace flows that requires + * recirculation. */ + struct ovs_list *recirc_queue; }; void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *, diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index e2228661fd09..c51c689b9f46 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -9710,6 +9710,41 @@ udp,vlan_tci=0x0000,dl_src=50:54:00:00:00:0a,dl_dst=50:54:00:00:00:09,nw_src=10. OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - conntrack - ofproto/trace]) +OVS_VSWITCHD_START + +add_of_ports br0 1 2 + +AT_DATA([flows.txt], [dnl +dnl Table 0 +dnl +table=0,priority=100,arp,action=normal +table=0,priority=10,udp,action=ct(table=1,zone=0) +table=0,priority=1,action=drop +dnl +dnl Table 1 +dnl +table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2 +table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2 +table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1 +table=1,priority=1,action=drop +]) + +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) + +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=2,udp'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], + [Datapath actions: 1 +]) + +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,udp'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], + [Datapath actions: 2 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto - set mtu]) OVS_VSWITCHD_START
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 est in the automatic recirculation process. A following patch will provide an option to customize ct_state. Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> --- ofproto/ofproto-dpif-trace.c | 88 +++++++++++++++++++++++++++++++++++++------- ofproto/ofproto-dpif-trace.h | 22 +++++++++++ ofproto/ofproto-dpif-xlate.c | 24 ++++++++++-- ofproto/ofproto-dpif-xlate.h | 4 ++ tests/ofproto-dpif.at | 35 ++++++++++++++++++ 5 files changed, 156 insertions(+), 17 deletions(-)