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

[Yi-Hung Wei]

On Wed, Jun 21, 2017 at 12:47 PM, Joe Stringer <joe at ovn.org> wrote:
> 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.
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.