The patch pretty much looks good to me. I still don't think we have
the patch port code exactly correct though. Suppose we're neither in
the forwarding state, nor the learning state. It looks to me like
we'll still run through the learning code when sending through the
patch port, though we don't do that for a normal port. I think what
we really want is something more akin to what we do in xlate_actions
where we call may_receive() directly.
Ethan
On Thu, Feb 7, 2013 at 11:04 AM, Ben Pfaff <b...@nicira.com> wrote:
> Until now the flow translation code has done one get_ofp_port() call
> initially to check for special processing, then one for each level of
> action processing. Only one call is actually necessary, though, because
> the in_port of a flow doesn't change in ordinary circumstances, and so this
> commit eliminates the unnecessary calls.
>
> The one case where the in_port can change is when a packet passes through
> a patch port. The implementation here was buggy anyway: when the patch
> port's peer had forwarding disabled by STP, then the code would drop all
> ODP actions, even those that were executed before the packet crossed the
> patch port. This commit fixes that case.
>
> With a complicated flow table involving multiple levels of resubmit, this
> increases flow setup performance by 2-3%.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-dpif.c | 56 ++++++++++++++++++++++++++++-------------------
> 1 files changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index f91d3c3..2c73640 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3318,15 +3318,11 @@ send_packet_in_miss(struct ofproto_dpif *ofproto,
> const struct ofpbuf *packet,
>
> static enum slow_path_reason
> process_special(struct ofproto_dpif *ofproto, const struct flow *flow,
> - const struct ofpbuf *packet)
> + const struct ofport_dpif *ofport, const struct ofpbuf
> *packet)
> {
> - struct ofport_dpif *ofport = get_ofp_port(ofproto, flow->in_port);
> -
> if (!ofport) {
> return 0;
> - }
> -
> - if (ofport->cfm && cfm_should_process_flow(ofport->cfm, flow)) {
> + } else if (ofport->cfm && cfm_should_process_flow(ofport->cfm, flow)) {
> if (packet) {
> cfm_process_heartbeat(ofport->cfm, packet);
> }
> @@ -3342,8 +3338,9 @@ process_special(struct ofproto_dpif *ofproto, const
> struct flow *flow,
> stp_process_packet(ofport, packet);
> }
> return SLOW_STP;
> + } else {
> + return 0;
> }
> - return 0;
> }
>
> static struct flow_miss *
> @@ -5733,6 +5730,7 @@ compose_output_action__(struct action_xlate_ctx *ctx,
> uint16_t ofp_port,
> struct ofport_dpif *peer = ofport_get_peer(ofport);
> struct flow old_flow = ctx->flow;
> const struct ofproto_dpif *peer_ofproto;
> + struct ofport_dpif *in_port;
>
> if (!peer) {
> xlate_report(ctx, "Nonexistent patch port peer");
> @@ -5750,7 +5748,20 @@ compose_output_action__(struct action_xlate_ctx *ctx,
> uint16_t ofp_port,
> ctx->flow.metadata = htonll(0);
> memset(&ctx->flow.tunnel, 0, sizeof ctx->flow.tunnel);
> memset(ctx->flow.regs, 0, sizeof ctx->flow.regs);
> - xlate_table_action(ctx, ctx->flow.in_port, 0, true);
> +
> + in_port = get_ofp_port(ctx->ofproto, ctx->flow.in_port);
> + if (!in_port || stp_forward_in_state(in_port->stp_state)) {
> + xlate_table_action(ctx, ctx->flow.in_port, 0, true);
> + } else {
> + /* Forwarding is disabled by STP. Let OFPP_NORMAL and the
> learning
> + * action look at the packet, then drop it. */
> + struct flow old_base_flow = ctx->base_flow;
> + size_t old_size = ctx->odp_actions->size;
> + xlate_table_action(ctx, ctx->flow.in_port, 0, true);
> + ctx->base_flow = old_base_flow;
> + ctx->odp_actions->size = old_size;
> + }
> +
> ctx->flow = old_flow;
> ctx->ofproto = ofproto_dpif_cast(ofport->up.ofproto);
>
> @@ -6300,16 +6311,9 @@ static void
> do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
> struct action_xlate_ctx *ctx)
> {
> - const struct ofport_dpif *port;
> bool was_evictable = true;
> const struct ofpact *a;
>
> - port = get_ofp_port(ctx->ofproto, ctx->flow.in_port);
> - if (port && !may_receive(port, ctx)) {
> - /* Drop this flow. */
> - return;
> - }
> -
> if (ctx->rule) {
> /* Don't let the rule we're working on get evicted underneath us. */
> was_evictable = ctx->rule->up.evictable;
> @@ -6497,12 +6501,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> }
>
> out:
> - /* We've let OFPP_NORMAL and the learning action look at the packet,
> - * so drop it now if forwarding is disabled. */
> - if (port && !stp_forward_in_state(port->stp_state)) {
> - ofpbuf_clear(ctx->odp_actions);
> - add_sflow_action(ctx);
> - }
> if (ctx->rule) {
> ctx->rule->up.evictable = was_evictable;
> }
> @@ -6565,6 +6563,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
> static bool hit_resubmit_limit;
>
> enum slow_path_reason special;
> + struct ofport_dpif *in_port;
>
> COVERAGE_INC(ofproto_dpif_xlate);
>
> @@ -6618,7 +6617,8 @@ xlate_actions(struct action_xlate_ctx *ctx,
> }
> }
>
> - special = process_special(ctx->ofproto, &ctx->flow, ctx->packet);
> + in_port = get_ofp_port(ctx->ofproto, ctx->flow.in_port);
> + special = process_special(ctx->ofproto, &ctx->flow, in_port,
> ctx->packet);
> if (special) {
> ctx->slow |= special;
> } else {
> @@ -6627,7 +6627,17 @@ xlate_actions(struct action_xlate_ctx *ctx,
> uint32_t local_odp_port;
>
> add_sflow_action(ctx);
> - do_xlate_actions(ofpacts, ofpacts_len, ctx);
> +
> + if (!in_port || may_receive(in_port, ctx)) {
> + do_xlate_actions(ofpacts, ofpacts_len, ctx);
> +
> + /* We've let OFPP_NORMAL and the learning action look at the
> + * packet, so drop it now if forwarding is disabled. */
> + if (in_port && !stp_forward_in_state(in_port->stp_state)) {
> + ofpbuf_clear(ctx->odp_actions);
> + add_sflow_action(ctx);
> + }
> + }
>
> if (ctx->max_resubmit_trigger && !ctx->resubmit_hook) {
> if (!hit_resubmit_limit) {
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
