ofproto: Avoid ofpbuf_clone() for OFPAT_CONTROLLER common case.

This additionally optimizes the common case of the first packet of a flow
that consists only of an OFPAT_CONTROLLER action, by avoiding an
ofpbuf_clone() call along that path.

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index a9e270b..4b9ceeb 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1927,41 +1927,44 @@ rule_has_out_port(const struct rule *rule, uint16_t out_port)
     return false;
 }
 
+/* Executes, within 'ofproto', the 'n_actions' actions in 'actions' on
+ * 'packet', which arrived on 'in_port'.
+ *
+ * Takes ownership of 'packet'. */
 static bool
 execute_odp_actions(struct ofproto *ofproto, uint16_t in_port,
                     const union odp_action *actions, size_t n_actions,
-                    const struct ofpbuf *packet)
+                    struct ofpbuf *packet)
 {
-    if (n_actions > 0 && actions[0].type == ODPAT_CONTROLLER) {
+    if (n_actions == 1 && actions[0].type == ODPAT_CONTROLLER) {
         /* As an optimization, avoid a round-trip from userspace to kernel to
          * userspace.  This also avoids possibly filling up kernel packet
          * buffers along the way. */
-        struct ofpbuf *copy;
         struct odp_msg *msg;
 
-        copy = ofpbuf_new(DPIF_RECV_MSG_PADDING + sizeof(struct odp_msg)
-                          + packet->size);
-        ofpbuf_reserve(copy, DPIF_RECV_MSG_PADDING);
-        msg = ofpbuf_put_uninit(copy, sizeof *msg);
+        msg = ofpbuf_push_uninit(packet, sizeof *msg);
         msg->type = _ODPL_ACTION_NR;
         msg->length = sizeof(struct odp_msg) + packet->size;
         msg->port = in_port;
         msg->reserved = 0;
         msg->arg = actions[0].controller.arg;
-        ofpbuf_put(copy, packet->data, packet->size);
 
-        send_packet_in(ofproto, copy);
+        send_packet_in(ofproto, packet);
 
-        actions++;
-        n_actions--;
-    }
+        return true;
+    } else {
+        int error;
 
-    return !n_actions || !dpif_execute(ofproto->dpif, in_port,
-                                       actions, n_actions, packet);
+        error = dpif_execute(ofproto->dpif, in_port,
+                             actions, n_actions, packet);
+        ofpbuf_delete(packet);
+        return !error;
+    }
 }
 
 /* Executes the actions indicated by 'rule' on 'packet', which is in flow
- * 'flow' and is considered to have arrived on ODP port 'in_port'.
+ * 'flow' and is considered to have arrived on ODP port 'in_port'.  'packet'
+ * must have at least sizeof(struct ofp_packet_in) bytes of headroom.
  *
  * The flow that 'packet' actually contains does not need to actually match
  * 'rule'; the actions in 'rule' will be applied to it either way.  Likewise,
@@ -1973,15 +1976,20 @@ execute_odp_actions(struct ofproto *ofproto, uint16_t in_port,
  * 'packet' using rule_make_actions().  If 'rule' is a wildcard rule, or if
  * 'rule' is an exact-match rule but 'flow' is not the rule's flow, then this
  * function will compose a set of ODP actions based on 'rule''s OpenFlow
- * actions and apply them to 'packet'. */
+ * actions and apply them to 'packet'.
+ *
+ * Takes ownership of 'packet'. */
 static void
 rule_execute(struct ofproto *ofproto, struct rule *rule,
              struct ofpbuf *packet, const flow_t *flow)
 {
     const union odp_action *actions;
+    struct odp_flow_stats stats;
     size_t n_actions;
     struct odp_actions a;
 
+    assert(ofpbuf_headroom(packet) >= sizeof(struct ofp_packet_in));
+
     /* Grab or compose the ODP actions.
      *
      * The special case for an exact-match 'rule' where 'flow' is not the
@@ -1992,6 +2000,7 @@ rule_execute(struct ofproto *ofproto, struct rule *rule,
         struct rule *super = rule->super ? rule->super : rule;
         if (xlate_actions(super->actions, super->n_actions, flow, ofproto,
                           packet, &a, NULL, 0, NULL)) {
+            ofpbuf_delete(packet);
             return;
         }
         actions = a.actions;
@@ -2002,16 +2011,21 @@ rule_execute(struct ofproto *ofproto, struct rule *rule,
     }
 
     /* Execute the ODP actions. */
+    flow_extract_stats(flow, packet, &stats);
     if (execute_odp_actions(ofproto, flow->in_port,
                             actions, n_actions, packet)) {
-        struct odp_flow_stats stats;
-        flow_extract_stats(flow, packet, &stats);
         update_stats(ofproto, rule, &stats);
         rule->used = time_msec();
         netflow_flow_update_time(ofproto->netflow, &rule->nf_flow, rule->used);
     }
 }
 
+/* Inserts 'rule' into 'p''s flow table.
+ *
+ * If 'packet' is nonnull, takes ownership of 'packet', executes 'rule''s
+ * actions on it and credits the statistics for sending the packet to 'rule'.
+ * 'packet' must have at least sizeof(struct ofp_packet_in) bytes of
+ * headroom. */
 static void
 rule_insert(struct ofproto *p, struct rule *rule, struct ofpbuf *packet,
             uint16_t in_port)
@@ -3585,7 +3599,6 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
     }
 
     rule_insert(p, rule, packet, in_port);
-    ofpbuf_delete(packet);
     return error;
 }
 
@@ -3623,7 +3636,6 @@ send_buffered_packet(struct ofproto *ofproto, struct ofconn *ofconn,
 
     flow_extract(packet, 0, in_port, &flow);
     rule_execute(ofproto, rule, packet, &flow);
-    ofpbuf_delete(packet);
 
     return 0;
 }
@@ -4131,9 +4143,6 @@ handle_odp_miss_msg(struct ofproto *p, struct ofpbuf *packet)
         }
     }
 
-    rule_execute(p, rule, &payload, &flow);
-    rule_reinstall(p, rule);
-
     if (rule->super && rule->super->cr.priority == FAIL_OPEN_PRIORITY) {
         /*
          * Extra-special case for fail-open mode.
@@ -4145,10 +4154,12 @@ handle_odp_miss_msg(struct ofproto *p, struct ofpbuf *packet)
          *
          * See the top-level comment in fail-open.c for more information.
          */
-        send_packet_in(p, packet);
-    } else {
-        ofpbuf_delete(packet);
+        send_packet_in(p, ofpbuf_clone(packet));
     }
+
+    ofpbuf_pull(packet, sizeof *msg);
+    rule_execute(p, rule, packet, &flow);
+    rule_reinstall(p, rule);
 }
 
 static void

diff --git a/ofproto/pktbuf.c b/ofproto/pktbuf.c
index 49e5c4d..67adb56 100644 (file)
--- a/ofproto/pktbuf.c
+++ b/ofproto/pktbuf.c
@@ -112,7 +112,9 @@ pktbuf_save(struct pktbuf *pb, struct ofpbuf *buffer, uint16_t in_port)
     if (++p->cookie >= COOKIE_MAX) {
         p->cookie = 0;
     }
-    p->buffer = ofpbuf_clone(buffer);
+    p->buffer = ofpbuf_new(sizeof(struct ofp_packet_in) + buffer->size);
+    ofpbuf_reserve(p->buffer, sizeof(struct ofp_packet_in));
+    ofpbuf_put(p->buffer, buffer->data, buffer->size);
     p->timeout = time_msec() + OVERWRITE_MSECS;
     p->in_port = in_port;
     return make_id(p - pb->packets, p->cookie);
@@ -154,6 +156,9 @@ pktbuf_get_null(void)
  * identifies a "null" packet buffer (created with pktbuf_get_null()), stores
  * NULL in '*bufferp' and UINT16_max in '*in_port'.
  *
+ * A returned packet will have at least sizeof(struct ofp_packet_in) bytes of
+ * headroom.
+ *
  * On failure, stores NULL in in '*bufferp' and UINT16_MAX in '*in_port'. */
 int
 pktbuf_retrieve(struct pktbuf *pb, uint32_t id, struct ofpbuf **bufferp,