[ovs-dev] [optimize 26/26] ofproto-dpif: Avoid extra flow copy in xlate_actions() for unneeded warnings.

Ethan Jackson ethan at nicira.com
Wed Apr 18 13:47:17 PDT 2012


Looks good, thanks.

Ethan

On Mon, Apr 16, 2012 at 17:19, Ben Pfaff <blp at nicira.com> wrote:
> The copy of the extra flow copy here was showing up in profiles.  We only
> need this copy if we end up doing a "trace" to warn the user.  Most runs
> won't ever do that, so don't start making copies until we actually hit
> such a case.
>
> This has a small behavioral change in that we'll only get a warning on the
> *second* time we hit the resubmit recursion limit, not on the first.  I
> doubt that's really a problem.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  ofproto/ofproto-dpif.c |   31 ++++++++++++++++++++-----------
>  1 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 44d0207..8aadead 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5297,6 +5297,11 @@ xlate_actions(struct action_xlate_ctx *ctx,
>               const union ofp_action *in, size_t n_in,
>               struct ofpbuf *odp_actions)
>  {
> +    /* Normally false.  Set to true if we ever hit MAX_RESUBMIT_RECURSION, so
> +     * that in the future we always keep a copy of the original flow for
> +     * tracing purposes. */
> +    static bool hit_resubmit_limit;
> +
>     COVERAGE_INC(ofproto_dpif_xlate);
>
>     ofpbuf_clear(odp_actions);
> @@ -5316,7 +5321,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
>     ctx->table_id = 0;
>     ctx->exit = false;
>
> -    if (ctx->ofproto->has_mirrors) {
> +    if (ctx->ofproto->has_mirrors || hit_resubmit_limit) {
>         /* Do this conditionally because the copy is expensive enough that it
>          * shows up in profiles.
>          *
> @@ -5353,21 +5358,25 @@ xlate_actions(struct action_xlate_ctx *ctx,
>         ctx->may_set_up_flow = false;
>     } else {
>         static struct vlog_rate_limit trace_rl = VLOG_RATE_LIMIT_INIT(1, 1);
> -        struct flow original_flow = ctx->flow;
>         ovs_be16 initial_tci = ctx->base_flow.vlan_tci;
>
>         add_sflow_action(ctx);
>         do_xlate_actions(in, n_in, ctx);
>
> -        if (ctx->max_resubmit_trigger && !ctx->resubmit_hook
> -            && !VLOG_DROP_ERR(&trace_rl)) {
> -            struct ds ds = DS_EMPTY_INITIALIZER;
> -
> -            ofproto_trace(ctx->ofproto, &original_flow, ctx->packet,
> -                          initial_tci, &ds);
> -            VLOG_ERR("Trace triggered by excessive resubmit recursion:\n%s",
> -                     ds_cstr(&ds));
> -            ds_destroy(&ds);
> +        if (ctx->max_resubmit_trigger && !ctx->resubmit_hook) {
> +            if (!hit_resubmit_limit) {
> +                /* We didn't record the original flow.  Make sure we do from
> +                 * now on. */
> +                hit_resubmit_limit = true;
> +            } else if (!VLOG_DROP_ERR(&trace_rl)) {
> +                struct ds ds = DS_EMPTY_INITIALIZER;
> +
> +                ofproto_trace(ctx->ofproto, &ctx->orig_flow, ctx->packet,
> +                              initial_tci, &ds);
> +                VLOG_ERR("Trace triggered by excessive resubmit "
> +                         "recursion:\n%s", ds_cstr(&ds));
> +                ds_destroy(&ds);
> +            }
>         }
>
>         if (!connmgr_may_set_up_flow(ctx->ofproto->up.connmgr, &ctx->flow,
> --
> 1.7.9
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list