[ovs-dev] [packet_in v2 11/18] ofproto: Always clone packets in PACKET_IN message.
Ethan Jackson
ethan at nicira.com
Sat Jan 7 19:11:52 UTC 2012
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 at nicira.com>
---
lib/ofp-util.c | 29 +++++----------------------
lib/ofp-util.h | 3 +-
ofproto/connmgr.c | 33 +++++++------------------------
ofproto/connmgr.h | 2 +-
ofproto/ofproto-dpif.c | 50 +++++++++++++++++------------------------------
5 files changed, 34 insertions(+), 83 deletions(-)
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 42dab87..bf7a82b 100644
--- 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 8fa729e..adff0d9 100644
--- 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 bd8ec91..64d6005 100644
--- 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);
}
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 0224352..d8a5e56 100644
--- 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 1645632..b9730cc 100644
--- 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;
}
--
1.7.7.1
More information about the dev
mailing list