Thanks for pointing out the nits. I fixed them and I'll apply this in
a minute.
On Tue, Nov 04, 2014 at 09:41:38AM -0800, Jarno Rajahalme wrote:
> 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
