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

[Joe Stringer]

On 21 June 2017 at 11:06, Yi-Hung Wei <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. A following patch
> will provide an option to customize ct_state.
>
> Signed-off-by: Yi-Hung Wei <yihung.wei at 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.