[ovs-dev] [PATCH 2/3] ofproto-dpif: Make ofproto/trace a bit more like real packet translation.

Jarno Rajahalme jrajahalme at nicira.com
Tue Nov 4 17:41:38 UTC 2014


With small nits below:

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

On Nov 3, 2014, at 5:14 PM, Ben Pfaff <blp at nicira.com> wrote:

> Until now, ofproto/trace has looked up the flow itself.  xlate_actions()
> can do the flow lookup internally and, since that is what happens when a
> packet arrives, having it do its own packet lookup makes a lot of sense.
> 
> I noticed this in connection with the actset_output field, which
> xlate_actions() should set to OFPP_UNSET at the beginning of translation
> before looking up the flow.  ofproto/trace didn't do that, so it looked
> up a rule with actset_output=0 instead.  By having xlate_actions() do the
> lookup, the behavior can be consistent and correct.
> 
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> ofproto/ofproto-dpif-xlate.c |   18 +++++++++-
> ofproto/ofproto-dpif.c       |   77 +++++++++++++++---------------------------
> 2 files changed, 45 insertions(+), 50 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 8a8eb92..a6bde1d 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2747,7 +2747,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
>         ctx->xin->flow.in_port.ofp_port = old_in_port;
> 
>         if (ctx->xin->resubmit_hook) {

It seems that only trace sets the resubmit hook, so this should be annotated with OVS_UNLIKELY as well?

> -            ctx->xin->resubmit_hook(ctx->xin, rule, ctx->recurse);
> +            ctx->xin->resubmit_hook(ctx->xin, rule, ctx->recurse + 1);
>         }
> 
>         switch (verdict) {
> @@ -4256,6 +4256,18 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>                                         !xin->skip_wildcards ? wc : NULL,
>                                         &rule, ctx.xin->xcache != NULL,
>                                         ctx.xin->resubmit_stats);
> +        if (OVS_UNLIKELY(ctx.xin->report_hook)) {
> +            if (rule == ctx.xbridge->miss_rule) {
> +                xlate_report(&ctx, "No match, flow generates \"packet in\"s.");
> +            } else if (rule == ctx.xbridge->no_packet_in_rule) {
> +                xlate_report(&ctx, "No match, packets dropped because "
> +                             "OFPPC_NO_PACKET_IN is set on in_port.");
> +            } else if (rule == ctx.xbridge->drop_frags_rule) {
> +                xlate_report(&ctx, "Packets dropped because they are IP "
> +                             "fragments and the fragment handling mode is "
> +                             "\"drop\".");
> +            }
> +        }
>         if (ctx.xin->resubmit_stats) {
>             rule_dpif_credit_stats(rule, ctx.xin->resubmit_stats);
>         }
> @@ -4266,6 +4278,10 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>             entry->u.rule = rule;
>         }
>         ctx.rule = rule;
> +
> +        if (ctx.xin->resubmit_hook) {

Same here.

> +            ctx.xin->resubmit_hook(ctx.xin, rule, 0);
> +        }
>     }
>     xout->fail_open = ctx.rule && rule_dpif_is_fail_open(ctx.rule);
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 744850c..a4f10e4 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4703,7 +4703,6 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
>               const struct ofpact ofpacts[], size_t ofpacts_len,
>               struct ds *ds)
> {
> -    struct rule_dpif *rule;
>     struct trace_ctx trace;
> 
>     ds_put_format(ds, "Bridge: %s\n", ofproto->up.name);
> @@ -4712,65 +4711,45 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
>     ds_put_char(ds, '\n');
> 
>     flow_wildcards_init_catchall(&trace.wc);
> -    if (ofpacts) {
> -        rule = NULL;
> -    } else {
> -        rule_dpif_lookup(ofproto, flow, &trace.wc, &rule, false, NULL);
> -
> -        trace_format_rule(ds, 0, rule);
> -        if (rule == ofproto->miss_rule) {
> -            ds_put_cstr(ds, "\nNo match, flow generates \"packet in\"s.\n");
> -        } else if (rule == ofproto->no_packet_in_rule) {
> -            ds_put_cstr(ds, "\nNo match, packets dropped because "
> -                        "OFPPC_NO_PACKET_IN is set on in_port.\n");
> -        } else if (rule == ofproto->drop_frags_rule) {
> -            ds_put_cstr(ds, "\nPackets dropped because they are IP fragments "
> -                        "and the fragment handling mode is \"drop\".\n");
> -        }
> -    }
> 
> -    if (rule || ofpacts) {
> -        trace.result = ds;
> -        trace.key = flow; /* Original flow key, used for megaflow. */
> -        trace.flow = *flow; /* May be modified by actions. */
> -        xlate_in_init(&trace.xin, ofproto, flow, flow->in_port.ofp_port, rule,
> -                      ntohs(flow->tcp_flags), packet);
> -        if (ofpacts) {
> -            trace.xin.ofpacts = ofpacts;
> -            trace.xin.ofpacts_len = ofpacts_len;
> -        }
> -        trace.xin.resubmit_hook = trace_resubmit;
> -        trace.xin.report_hook = trace_report;
> +    trace.result = ds;
> +    trace.key = flow; /* Original flow key, used for megaflow. */
> +    trace.flow = *flow; /* May be modified by actions. */
> +    xlate_in_init(&trace.xin, ofproto, flow, flow->in_port.ofp_port, NULL,
> +                  ntohs(flow->tcp_flags), packet);
> +    trace.xin.ofpacts = ofpacts;
> +    trace.xin.ofpacts_len = ofpacts_len;
> +    trace.xin.resubmit_hook = trace_resubmit;
> +    trace.xin.report_hook = trace_report;
> 
> -        xlate_actions(&trace.xin, &trace.xout);
> +    xlate_actions(&trace.xin, &trace.xout);
> 
> -        ds_put_char(ds, '\n');
> -        trace_format_flow(ds, 0, "Final flow", &trace);
> -        trace_format_megaflow(ds, 0, "Megaflow", &trace);
> +    ds_put_char(ds, '\n');
> +    trace_format_flow(ds, 0, "Final flow", &trace);
> +    trace_format_megaflow(ds, 0, "Megaflow", &trace);
> 
> -        ds_put_cstr(ds, "Datapath actions: ");
> -        format_odp_actions(ds, ofpbuf_data(trace.xout.odp_actions),
> -                           ofpbuf_size(trace.xout.odp_actions));
> +    ds_put_cstr(ds, "Datapath actions: ");
> +    format_odp_actions(ds, ofpbuf_data(trace.xout.odp_actions),
> +                       ofpbuf_size(trace.xout.odp_actions));
> 
> -        if (trace.xout.slow) {
> -            enum slow_path_reason slow;
> +    if (trace.xout.slow) {
> +        enum slow_path_reason slow;
> 
> -            ds_put_cstr(ds, "\nThis flow is handled by the userspace "
> -                        "slow path because it:");
> +        ds_put_cstr(ds, "\nThis flow is handled by the userspace "
> +                    "slow path because it:");
> 
> -            slow = trace.xout.slow;
> -            while (slow) {
> -                enum slow_path_reason bit = rightmost_1bit(slow);
> +        slow = trace.xout.slow;
> +        while (slow) {
> +            enum slow_path_reason bit = rightmost_1bit(slow);
> 
> -                ds_put_format(ds, "\n\t- %s.",
> -                              slow_path_reason_to_explanation(bit));
> +            ds_put_format(ds, "\n\t- %s.",
> +                          slow_path_reason_to_explanation(bit));
> 
> -                slow &= ~bit;
> -            }
> +            slow &= ~bit;
>         }
> -
> -        xlate_out_uninit(&trace.xout);
>     }
> +
> +    xlate_out_uninit(&trace.xout);
> }
> 
> /* Store the current ofprotos in 'ofproto_shash'.  Returns a sorted list
> -- 
> 1.7.10.4
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list