[ovs-dev] [PATCH 2/3] ofproto/trace: Add support for tracing conntrack recirculation

Yi-Hung Wei yihung.wei at gmail.com
Mon Jun 26 17:18:25 UTC 2017


On Wed, Jun 21, 2017 at 11:58 AM, Darrell Ball <dball at vmware.com> wrote:
>
>
> On 6/21/17, 11:06 AM, "ovs-dev-bounces at openvswitch.org on behalf of Yi-Hung Wei" <ovs-dev-bounces at openvswitch.org on behalf of yihung.wei at 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 at 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 at 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=
>
>


More information about the dev mailing list