[ovs-dev] [PATCH] ofproto-dpif: Reduce number of get_ofp_port() calls during flow xlate.

Ben Pfaff blp at nicira.com
Mon Feb 11 22:34:30 UTC 2013


I think you're right.

I changed
        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;
        }
to
        in_port = get_ofp_port(ctx->ofproto, ctx->flow.in_port);
        if (!in_port || may_receive(in_port, ctx)) {
            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;
            }
        }
in compose_output_action__().  I think this takes care of it.  I'm
appending a full revised patch.  How does it look?

On Fri, Feb 08, 2013 at 01:29:27PM -0800, Ethan Jackson wrote:
> 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 <blp at 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 <blp at nicira.com>

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Mon, 11 Feb 2013 14:32:52 -0800
Subject: [PATCH] ofproto-dpif: Reduce number of get_ofp_port() calls during flow xlate.

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 <blp at nicira.com>
---
 ofproto/ofproto-dpif.c |   59 +++++++++++++++++++++++++++++------------------
 1 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 109e57c..f1d88f1 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3311,15 +3311,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);
         }
@@ -3335,8 +3331,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 *
@@ -5546,6 +5543,7 @@ send_packet(const struct ofport_dpif *ofport, struct ofpbuf *packet)
 
 /* OpenFlow to datapath action translation. */
 
+static bool may_receive(const struct ofport_dpif *, struct action_xlate_ctx *);
 static void do_xlate_actions(const struct ofpact *, size_t ofpacts_len,
                              struct action_xlate_ctx *);
 static void xlate_normal(struct action_xlate_ctx *);
@@ -5726,6 +5724,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");
@@ -5743,7 +5742,22 @@ 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 || may_receive(in_port, ctx)) {
+            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);
 
@@ -6273,16 +6287,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;
@@ -6466,12 +6473,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;
     }
@@ -6534,6 +6535,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);
 
@@ -6587,7 +6589,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 {
@@ -6596,7 +6599,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




More information about the dev mailing list