[ovs-dev] [PATCH 07/26] ofproto-dpif-xlate: Calculate 'ofpacts' in more restricted scope.

[Jarno Rajahalme]

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

> On Jul 29, 2015, at 11:42 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> This moves the calculation of 'ofpacts' closer to its actual use, which
> in my opinion makes the code easier to read.
> 
> This commit also expands the circumstances in which OVS omits sending
> NetFlow records from those where there is exactly one OpenFlow action that
> sends to controller, to those where any OpenFlow action sends to
> controller.  I doubt that this is a big deal.
> 
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> ofproto/ofproto-dpif-xlate.c | 68 ++++++++++++++++++++------------------------
> 1 file changed, 31 insertions(+), 37 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index eb1e17a..e39e46f 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4781,10 +4781,8 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>     ofpbuf_reserve(ctx.odp_actions, NL_A_U32_SIZE);
> 
>     enum slow_path_reason special;
> -    const struct ofpact *ofpacts;
>     struct xport *in_port;
>     struct flow orig_flow;
> -    size_t ofpacts_len;
>     bool tnl_may_send;
>     bool is_icmp;
> 
> @@ -4932,20 +4930,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>     }
>     xout->fail_open = ctx.rule && rule_dpif_is_fail_open(ctx.rule);
> 
> -    if (xin->ofpacts) {
> -        ofpacts = xin->ofpacts;
> -        ofpacts_len = xin->ofpacts_len;
> -    } else if (ctx.rule) {
> -        const struct rule_actions *actions = rule_dpif_get_actions(ctx.rule);
> -
> -        ofpacts = actions->ofpacts;
> -        ofpacts_len = actions->ofpacts_len;
> -
> -        ctx.rule_cookie = rule_dpif_get_flow_cookie(ctx.rule);
> -    } else {
> -        OVS_NOT_REACHED();
> -    }
> -
>     if (mbridge_has_mirrors(xbridge->mbridge)) {
>         /* Do this conditionally because the copy is expensive enough that it
>          * shows up in profiles. */
> @@ -4994,6 +4978,22 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>         }
> 
>         if (tnl_may_send && (!in_port || may_receive(in_port, &ctx))) {
> +            const struct ofpact *ofpacts;
> +            size_t ofpacts_len;
> +
> +            if (xin->ofpacts) {
> +                ofpacts = xin->ofpacts;
> +                ofpacts_len = xin->ofpacts_len;
> +            } else if (ctx.rule) {
> +                const struct rule_actions *actions
> +                    = rule_dpif_get_actions(ctx.rule);
> +                ofpacts = actions->ofpacts;
> +                ofpacts_len = actions->ofpacts_len;
> +                ctx.rule_cookie = rule_dpif_get_flow_cookie(ctx.rule);
> +            } else {
> +                OVS_NOT_REACHED();
> +            }
> +
>             do_xlate_actions(ofpacts, ofpacts_len, &ctx);
> 
>             /* We've let OFPP_NORMAL and the learning action look at the
> @@ -5068,28 +5068,22 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>         }
>     }
> 
> -    /* Do netflow only for packets really received by the bridge. */
> -    if (!xin->recirc && xbridge->netflow) {
> -        /* Only update netflow if we don't have controller flow.  We don't
> -         * report NetFlow expiration messages for such facets because they
> -         * are just part of the control logic for the network, not real
> -         * traffic. */
> -        if (ofpacts_len == 0
> -            || ofpacts->type != OFPACT_CONTROLLER
> -            || ofpact_next(ofpacts) < ofpact_end(ofpacts, ofpacts_len)) {
> -            if (ctx.xin->resubmit_stats) {
> -                netflow_flow_update(xbridge->netflow, flow,
> -                                    xout->nf_output_iface,
> -                                    ctx.xin->resubmit_stats);
> -            }
> -            if (ctx.xin->xcache) {
> -                struct xc_entry *entry;
> +    /* Do netflow only for packets really received by the bridge and not sent
> +     * to the controller.  We consider packets sent to the controller to be
> +     * part of the control plane rather than the data plane. */
> +    if (!xin->recirc && xbridge->netflow && !(xout->slow & SLOW_CONTROLLER)) {
> +        if (ctx.xin->resubmit_stats) {
> +            netflow_flow_update(xbridge->netflow, flow,
> +                                xout->nf_output_iface,
> +                                ctx.xin->resubmit_stats);
> +        }
> +        if (ctx.xin->xcache) {
> +            struct xc_entry *entry;
> 
> -                entry = xlate_cache_add_entry(ctx.xin->xcache, XC_NETFLOW);
> -                entry->u.nf.netflow = netflow_ref(xbridge->netflow);
> -                entry->u.nf.flow = xmemdup(flow, sizeof *flow);
> -                entry->u.nf.iface = xout->nf_output_iface;
> -            }
> +            entry = xlate_cache_add_entry(ctx.xin->xcache, XC_NETFLOW);
> +            entry->u.nf.netflow = netflow_ref(xbridge->netflow);
> +            entry->u.nf.flow = xmemdup(flow, sizeof *flow);
> +            entry->u.nf.iface = xout->nf_output_iface;
>         }
>     }
> 
> -- 
> 2.1.3
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev