[ovs-dev] [PATCH 2/9] ofproto/trace: Propagate ct_zone in recirculation

Greg Rose gvrose8192 at gmail.com
Thu Aug 31 21:38:35 UTC 2017


On 08/25/2017 03:51 PM, Yi-Hung Wei wrote:
> This patch propagates ct_zone when ofproto/trace automatically runs
> through the recirculation process.
>
> Fixes: e6bc8e749381 ("ofproto/trace: Add support for tracing conntrack recirculation")
> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>
> ---
>   ofproto/ofproto-dpif-trace.c |  4 +++-
>   ofproto/ofproto-dpif-trace.h |  3 ++-
>   ofproto/ofproto-dpif-xlate.c |  7 ++++---
>   tests/ofproto-dpif.at        | 14 +++++++-------
>   4 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index a45c9cfd9619..c3c929520a2d 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -91,7 +91,8 @@ oftrace_node_destroy(struct oftrace_node *node)
>   bool
>   oftrace_add_recirc_node(struct ovs_list *recirc_queue,
>                           enum oftrace_recirc_type type, const struct flow *flow,
> -                        const struct dp_packet *packet, uint32_t recirc_id)
> +                        const struct dp_packet *packet, uint32_t recirc_id,
> +                        const uint16_t zone)

This function is beginning to get a lot of parameters.  As a suggestion perhaps you might create a helper struct
to contain all the parameters and just pass a pointer.  Personally I start looking for ways to cut down on parameter
passing when a function gets to 4 or more parameters.  Again - just a personal predilection.

Otherwise the patch LGTM.

Reviewed-by: Greg Rose <gvrose8192 at gmail.com>

>   {
>       if (!recirc_id_node_find_and_ref(recirc_id)) {
>           return false;
> @@ -104,6 +105,7 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue,
>       node->recirc_id = recirc_id;
>       node->flow = *flow;
>       node->flow.recirc_id = recirc_id;
> +    node->flow.ct_zone = zone;
>       node->packet = packet ? dp_packet_clone(packet) : NULL;
>
>       return true;
> diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h
> index 5178d227ed07..5e51771b19b0 100644
> --- a/ofproto/ofproto-dpif-trace.h
> +++ b/ofproto/ofproto-dpif-trace.h
> @@ -84,6 +84,7 @@ struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type,
>                                       const char *text);
>   bool oftrace_add_recirc_node(struct ovs_list *recirc_queue,
>                                enum oftrace_recirc_type, const struct flow *,
> -                             const struct dp_packet *, uint32_t recirc_id);
> +                             const struct dp_packet *, uint32_t recirc_id,
> +                             const uint16_t zone);
>
>   #endif /* ofproto-dpif-trace.h */
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 9e1f837cb23e..95c4fef0d9b0 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4685,7 +4685,8 @@ finish_freezing(struct xlate_ctx *ctx)
>    * the remainder of the current action list and asynchronously resume pipeline
>    * processing in 'table' with the current metadata and action set. */
>   static void
> -compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table)
> +compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table,
> +                             const uint16_t zone)
>   {
>       uint32_t recirc_id;
>       ctx->freezing = true;
> @@ -4694,7 +4695,7 @@ compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table)
>       if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) {
>           if (oftrace_add_recirc_node(ctx->xin->recirc_queue,
>                                       OFT_RECIRC_CONNTRACK, &ctx->xin->flow,
> -                                    ctx->xin->packet, recirc_id)) {
> +                                    ctx->xin->packet, recirc_id, zone)) {
>               xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to "
>                            "recirculate. The forked pipeline will be resumed at "
>                            "table %u.", table);
> @@ -5764,7 +5765,7 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc)
>
>       if (ofc->recirc_table != NX_CT_RECIRC_NONE) {
>           ctx->conntracked = true;
> -        compose_recirculate_and_fork(ctx, ofc->recirc_table);
> +        compose_recirculate_and_fork(ctx, ofc->recirc_table, zone);
>       }
>
>       /* The ct_* fields are only available in the scope of the 'recirc_table'
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 28a7e827cac2..c76ea4eee1cc 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -9827,21 +9827,21 @@ 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=10,in_port=1,ct_zone=0,ct_state=+trk+new,udp,action=ct(commit,zone=0),2
> +table=1,priority=10,in_port=1,ct_zone=0,ct_state=+trk+est,udp,action=2
> +table=1,priority=10,in_port=2,ct_zone=0,ct_state=+trk+est,udp,action=1
>   table=1,priority=1,action=drop
>   dnl
>   dnl Table 2
>   dnl
> -table=2,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)
> -table=2,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=ct(table=3,zone=2)
> +table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2)
> +table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+est,tcp,action=ct(table=3,zone=2)
>   table=2,priority=1,action=drop
>   dnl
>   dnl Table 3
>   dnl
> -table=3,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=2),4
> -table=3,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=3
> +table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+new,tcp,action=ct(commit,zone=2),4
> +table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+est,tcp,action=3
>   table=2,priority=1,action=drop
>   ])
>
>



More information about the dev mailing list