ofproto: Always clone packets in PACKET_IN message.

This patch removes an optimization which significantly complicates
the code in ways which would get worse in future patches if not
removed.  Furthermore, future patches will have fewer cases which
can take advantage of the optimization further mitigating its
justification.

Signed-off-by: Ethan Jackson <ethan@nicira.com>

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 42dab87f4714125dd1b90e08087a505db34a237b..bf7a82b659932538463f0697c3e344da4beb0326 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1543,34 +1543,17 @@ ofputil_encode_flow_removed(const struct ofputil_flow_removed *fr,
 }
 
 /* Converts abstract ofputil_packet_in 'pin' into an OFPT_PACKET_IN message
- * and returns the message.
- *
- * If 'rw_packet' is NULL, the caller takes ownership of the newly allocated
- * returned ofpbuf.
- *
- * If 'rw_packet' is nonnull, then it must contain the same data as
- * pin->packet.  'rw_packet' is allowed to be the same ofpbuf as pin->packet.
- * It is modified in-place into an OFPT_PACKET_IN message according to 'pin',
- * and then ofputil_encode_packet_in() returns 'rw_packet'.  If 'rw_packet' has
- * enough headroom to insert a "struct ofp_packet_in", this is more efficient
- * than ofputil_encode_packet_in() because it does not copy the packet
- * payload. */
+ * and returns the message. */
 struct ofpbuf *
-ofputil_encode_packet_in(const struct ofputil_packet_in *pin,
-                        struct ofpbuf *rw_packet)
+ofputil_encode_packet_in(const struct ofputil_packet_in *pin)
 {
     int total_len = pin->packet->size;
     struct ofp_packet_in opi;
+    struct ofpbuf *rw_packet;
 
-    if (rw_packet) {
-        if (pin->send_len < rw_packet->size) {
-            rw_packet->size = pin->send_len;
-        }
-    } else {
-        rw_packet = ofpbuf_clone_data_with_headroom(
-            pin->packet->data, MIN(pin->send_len, pin->packet->size),
-            offsetof(struct ofp_packet_in, data));
-    }
+    rw_packet = ofpbuf_clone_data_with_headroom(
+        pin->packet->data, MIN(pin->send_len, pin->packet->size),
+        offsetof(struct ofp_packet_in, data));
 
     /* Add OFPT_PACKET_IN. */
     memset(&opi, 0, sizeof opi);

diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 8fa729e93359fc01eebc48feca6cdb4ef04de65f..adff0d9ba52879e3a9950a0553b75b4e606cfd4d 100644 (file)
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -222,8 +222,7 @@ struct ofputil_packet_in {
     int send_len;
 };
 
-struct ofpbuf *ofputil_encode_packet_in(const struct ofputil_packet_in *,
-                                        struct ofpbuf *rw_packet);
+struct ofpbuf *ofputil_encode_packet_in(const struct ofputil_packet_in *);
 
 /* OpenFlow protocol utility functions. */
 void *make_openflow(size_t openflow_len, uint8_t type, struct ofpbuf **);

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index bd8ec91bf2f8def77add9a1d4b645fa2d1d40bcd..64d6005574500e570cd601f610a420b1ed8e32ed 100644 (file)
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1096,7 +1096,7 @@ ofconn_send(const struct ofconn *ofconn, struct ofpbuf *msg,
 /* Sending asynchronous messages. */
 
 static void schedule_packet_in(struct ofconn *, struct ofputil_packet_in,
-                               const struct flow *, struct ofpbuf *rw_packet);
+                               const struct flow *);
 
 /* Sends an OFPT_PORT_STATUS message with 'opp' and 'reason' to appropriate
  * controllers managed by 'mgr'. */
@@ -1151,32 +1151,19 @@ connmgr_send_flow_removed(struct connmgr *mgr,
 }
 
 /* Given 'pin', sends an OFPT_PACKET_IN message to each OpenFlow controller as
- * necessary according to their individual configurations.
- *
- * 'rw_packet' may be NULL.  Otherwise, 'rw_packet' must contain the same data
- * as pin->packet.  (rw_packet == pin->packet is also valid.)  Ownership of
- * 'rw_packet' is transferred to this function. */
+ * necessary according to their individual configurations. */
 void
 connmgr_send_packet_in(struct connmgr *mgr,
                        const struct ofputil_packet_in *pin,
-                       const struct flow *flow, struct ofpbuf *rw_packet)
+                       const struct flow *flow)
 {
-    struct ofconn *ofconn, *prev;
+    struct ofconn *ofconn;
 
-    prev = NULL;
     LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
         if (ofconn_receives_async_msgs(ofconn)) {
-            if (prev) {
-                schedule_packet_in(prev, *pin, flow, NULL);
-            }
-            prev = ofconn;
+            schedule_packet_in(ofconn, *pin, flow);
         }
     }
-    if (prev) {
-        schedule_packet_in(prev, *pin, flow, rw_packet);
-    } else {
-        ofpbuf_delete(rw_packet);
-    }
 }
 
 /* pinsched callback for sending 'ofp_packet_in' on 'ofconn'. */
@@ -1191,14 +1178,10 @@ do_send_packet_in(struct ofpbuf *ofp_packet_in, void *ofconn_)
 
 /* Takes 'pin', whose packet has the flow specified by 'flow', composes an
  * OpenFlow packet-in message from it, and passes it to 'ofconn''s packet
- * scheduler for sending.
- *
- * 'rw_packet' may be NULL.  Otherwise, 'rw_packet' must contain the same data
- * as pin->packet.  (rw_packet == pin->packet is also valid.)  Ownership of
- * 'rw_packet' is transferred to this function. */
+ * scheduler for sending. */
 static void
 schedule_packet_in(struct ofconn *ofconn, struct ofputil_packet_in pin,
-                   const struct flow *flow, struct ofpbuf *rw_packet)
+                   const struct flow *flow)
 {
     struct connmgr *mgr = ofconn->connmgr;
 
@@ -1229,7 +1212,7 @@ schedule_packet_in(struct ofconn *ofconn, struct ofputil_packet_in pin,
      * immediately call into do_send_packet_in() or it might buffer it for a
      * while (until a later call to pinsched_run()). */
     pinsched_send(ofconn->schedulers[pin.reason == OFPR_NO_MATCH ? 0 : 1],
-                  flow->in_port, ofputil_encode_packet_in(&pin, rw_packet),
+                  flow->in_port, ofputil_encode_packet_in(&pin),
                   do_send_packet_in, ofconn);
 }
 \f

diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 022435265cee77d38253b45aafa1e3b510a7025c..d8a5e56bbcf7bf663649334cd82c34c2d60764b6 100644 (file)
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -110,7 +110,7 @@ void connmgr_send_port_status(struct connmgr *, const struct ofp_phy_port *,
 void connmgr_send_flow_removed(struct connmgr *,
                                const struct ofputil_flow_removed *);
 void connmgr_send_packet_in(struct connmgr *, const struct ofputil_packet_in *,
-                            const struct flow *, struct ofpbuf *rw_packet);
+                            const struct flow *);
 
 /* Fail-open settings. */
 enum ofproto_fail_mode connmgr_get_fail_mode(const struct connmgr *);

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 1645632022fb68fe598d664e7ae45c5ab3b91322..b9730cc49bcdf571d34fc74182ee43027e948a48 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -328,7 +328,7 @@ static bool execute_controller_action(struct ofproto_dpif *,
                                       const struct flow *,
                                       const struct nlattr *odp_actions,
                                       size_t actions_len,
-                                      struct ofpbuf *packet, bool clone);
+                                      struct ofpbuf *packet);
 
 static void facet_flush_stats(struct ofproto_dpif *, struct facet *);
 
@@ -2410,13 +2410,10 @@ struct flow_miss_op {
 
 /* Sends an OFPT_PACKET_IN message for 'packet' of type OFPR_NO_MATCH to each
  * OpenFlow controller as necessary according to their individual
- * configurations.
- *
- * If 'clone' is true, the caller retains ownership of 'packet'.  Otherwise,
- * ownership is transferred to this function. */
+ * configurations. */
 static void
 send_packet_in_miss(struct ofproto_dpif *ofproto, struct ofpbuf *packet,
-                    const struct flow *flow, bool clone)
+                    const struct flow *flow)
 {
     struct ofputil_packet_in pin;
 
@@ -2425,8 +2422,7 @@ send_packet_in_miss(struct ofproto_dpif *ofproto, struct ofpbuf *packet,
     pin.reason = OFPR_NO_MATCH;
     pin.buffer_id = 0;          /* not yet known */
     pin.send_len = 0;           /* not used for flow table misses */
-    connmgr_send_packet_in(ofproto->up.connmgr, &pin, flow,
-                           clone ? NULL : packet);
+    connmgr_send_packet_in(ofproto->up.connmgr, &pin, flow);
 }
 
 /* Sends an OFPT_PACKET_IN message for 'packet' of type OFPR_ACTION to each
@@ -2434,13 +2430,10 @@ send_packet_in_miss(struct ofproto_dpif *ofproto, struct ofpbuf *packet,
  * configurations.
  *
  * 'send_len' should be the number of bytes of 'packet' to send to the
- * controller, as specified in the action that caused the packet to be sent.
- *
- * If 'clone' is true, the caller retains ownership of 'upcall->packet'.
- * Otherwise, ownership is transferred to this function. */
+ * controller, as specified in the action that caused the packet to be sent. */
 static void
 send_packet_in_action(struct ofproto_dpif *ofproto, struct ofpbuf *packet,
-                      uint64_t userdata, const struct flow *flow, bool clone)
+                      uint64_t userdata, const struct flow *flow)
 {
     struct ofputil_packet_in pin;
     struct user_action_cookie cookie;
@@ -2452,8 +2445,7 @@ send_packet_in_action(struct ofproto_dpif *ofproto, struct ofpbuf *packet,
     pin.reason = OFPR_ACTION;
     pin.buffer_id = 0;          /* not yet known */
     pin.send_len = cookie.data;
-    connmgr_send_packet_in(ofproto->up.connmgr, &pin, flow,
-                           clone ? NULL : packet);
+    connmgr_send_packet_in(ofproto->up.connmgr, &pin, flow);
 }
 
 static bool
@@ -2540,10 +2532,8 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
                              flow->in_port);
             }
 
-            LIST_FOR_EACH_SAFE (packet, next_packet, list_node,
-                                &miss->packets) {
-                list_remove(&packet->list_node);
-                send_packet_in_miss(ofproto, packet, flow, false);
+            LIST_FOR_EACH (packet, list_node, &miss->packets) {
+                send_packet_in_miss(ofproto, packet, flow);
             }
 
             return;
@@ -2573,7 +2563,7 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
              *
              * See the top-level comment in fail-open.c for more information.
              */
-            send_packet_in_miss(ofproto, packet, flow, true);
+            send_packet_in_miss(ofproto, packet, flow);
         }
 
         if (!facet->may_install || !subfacet->actions) {
@@ -2587,7 +2577,7 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
 
         if (!execute_controller_action(ofproto, &facet->flow,
                                        subfacet->actions,
-                                       subfacet->actions_len, packet, true)
+                                       subfacet->actions_len, packet)
             && subfacet->actions_len > 0) {
             struct flow_miss_op *op = &ops[(*n_ops)++];
             struct dpif_execute *execute = &op->dpif_op.execute;
@@ -2817,15 +2807,14 @@ handle_userspace_upcall(struct ofproto_dpif *ofproto,
             dpif_sflow_received(ofproto->sflow, upcall->packet, &flow,
                                 &cookie);
         }
-        ofpbuf_delete(upcall->packet);
     } else if (cookie.type == USER_ACTION_COOKIE_CONTROLLER) {
         COVERAGE_INC(ofproto_dpif_ctlr_action);
         send_packet_in_action(ofproto, upcall->packet, upcall->userdata,
-                              &flow, false);
+                              &flow);
     } else {
         VLOG_WARN_RL(&rl, "invalid user cookie : 0x%"PRIx64, upcall->userdata);
-        ofpbuf_delete(upcall->packet);
     }
+    ofpbuf_delete(upcall->packet);
 }
 
 static int
@@ -3160,15 +3149,12 @@ facet_free(struct facet *facet)
 
 /* If the 'actions_len' bytes of actions in 'odp_actions' are just a single
  * OVS_ACTION_ATTR_USERSPACE action, executes it internally and returns true.
- * Otherwise, returns false without doing anything.
- *
- * If 'clone' is true, the caller always retains ownership of 'packet'.
- * Otherwise, ownership is transferred to this function if it returns true. */
+ * Otherwise, returns false without doing anything. */
 static bool
 execute_controller_action(struct ofproto_dpif *ofproto,
                           const struct flow *flow,
                           const struct nlattr *odp_actions, size_t actions_len,
-                          struct ofpbuf *packet, bool clone)
+                          struct ofpbuf *packet)
 {
     if (actions_len
         && odp_actions->nla_type == OVS_ACTION_ATTR_USERSPACE
@@ -3183,8 +3169,7 @@ execute_controller_action(struct ofproto_dpif *ofproto,
         const struct nlattr *nla;
 
         nla = nl_attr_find_nested(odp_actions, OVS_USERSPACE_ATTR_USERDATA);
-        send_packet_in_action(ofproto, packet, nl_attr_get_u64(nla), flow,
-                              clone);
+        send_packet_in_action(ofproto, packet, nl_attr_get_u64(nla), flow);
         return true;
     } else {
         return false;
@@ -3205,7 +3190,8 @@ execute_odp_actions(struct ofproto_dpif *ofproto, const struct flow *flow,
     int error;
 
     if (execute_controller_action(ofproto, flow, odp_actions, actions_len,
-                                  packet, false)) {
+                                  packet)) {
+        ofpbuf_delete(packet);
         return true;
     }