With small nits below:
Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
On Nov 3, 2014, at 5:14 PM, Ben Pfaff <b...@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 <b...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
