On 6/26/17, 11:05 AM, "Darrell Ball" <[email protected]> wrote:
On 6/26/17, 10:18 AM, "Yi-Hung Wei" <[email protected]> wrote:
On Wed, Jun 21, 2017 at 11:58 AM, Darrell Ball <[email protected]> wrote:
>
>
> On 6/21/17, 11:06 AM, "[email protected] on behalf of
Yi-Hung Wei" <[email protected] on behalf of
[email protected]> 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 <[email protected]>
> ---
> 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
> [email protected]
>
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=
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
