[ovs-dev] [PATCH 31/41] openflow: Better abstract handling of packet-in messages.

Ben Pfaff blp at ovn.org
Tue Jan 19 07:27:18 UTC 2016


Packet-in messages have been a bit of a mess.  First, their abstraction
in the form of struct ofputil_packet_in has some fields that are used
in a clear way for incoming and outgoing packet-ins, and others
(packet_len, total_len, buffer_id) have have confusing meanings or
usage pattern depending on their direction.

Second, it's very confusing how a packet-in has both a reason (OFPR_*)
and a miss type (OFPROTO_PACKET_IN_*) and how those add up to the
actual reason that is used "on the wire" for each OpenFlow version (and
even whether the packet-in is sent at all!).

Finally, there's all kind of low-level detail randomly scattered between
connmgr, ofproto-dpif-xlate, and ofp-util.

This commit attempts to clear up some of the confusion.  It simplifies
the struct ofputil_packet_in abstraction by removing the members that
didn't have a clear and consistent meaning between incoming and outgoing
packet-ins.  It gets rid of OFPROTO_PACKET_IN_*, instead adding a couple
of nonstandard OFPR_* reasons that add up to what OFPROTO_PACKET_IN_*
was meant to say (in what I hope is a clearer way).  And it consolidates
the tricky parts into ofp-util, where I hope it will be easier to
understand all in one place.

Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 include/openflow/openflow-common.h |   5 +
 lib/learning-switch.c              |  18 +--
 lib/ofp-print.c                    |  27 +++--
 lib/ofp-util.c                     | 233 +++++++++++++++++++++++++------------
 lib/ofp-util.h                     |  40 ++++---
 ofproto/connmgr.c                  | 140 ++++------------------
 ofproto/connmgr.h                  |  22 +---
 ofproto/fail-open.c                |  23 ++--
 ofproto/ofproto-dpif-xlate.c       |  47 ++++----
 ofproto/ofproto-dpif.c             |   2 +-
 ofproto/ofproto.c                  |   2 +-
 ovn/controller/pinctrl.c           |   4 +-
 12 files changed, 270 insertions(+), 293 deletions(-)

diff --git a/include/openflow/openflow-common.h b/include/openflow/openflow-common.h
index 3985705..da2b7a5 100644
--- a/include/openflow/openflow-common.h
+++ b/include/openflow/openflow-common.h
@@ -274,6 +274,7 @@ enum ofp_capabilities {
 
 /* Why is this packet being sent to the controller? */
 enum ofp_packet_in_reason {
+    /* Standard reasons. */
     OFPR_NO_MATCH,          /* No matching flow. */
     OFPR_ACTION,            /* Action explicitly output to controller. */
     OFPR_INVALID_TTL,       /* Packet has invalid TTL. */
@@ -287,6 +288,10 @@ enum ofp_packet_in_reason {
     (OFPR10_BITS |                                                      \
      (1u << OFPR_ACTION_SET) | (1u << OFPR_GROUP) | (1u << OFPR_PACKET_OUT))
 
+    /* Nonstandard reason--not exposed via OpenFlow. */
+    OFPR_EXPLICIT_MISS,
+    OFPR_IMPLICIT_MISS,
+
     OFPR_N_REASONS
 };
 
diff --git a/lib/learning-switch.c b/lib/learning-switch.c
index 2b764f6..35c3fef 100644
--- a/lib/learning-switch.c
+++ b/lib/learning-switch.c
@@ -512,6 +512,8 @@ static void
 process_packet_in(struct lswitch *sw, const struct ofp_header *oh)
 {
     struct ofputil_packet_in pi;
+    size_t total_len;
+    uint32_t buffer_id;
     uint32_t queue_id;
     ofp_port_t out_port;
 
@@ -524,7 +526,7 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh)
     struct dp_packet pkt;
     struct flow flow;
 
-    error = ofputil_decode_packet_in(&pi, oh);
+    error = ofputil_decode_packet_in(oh, &pi, &total_len, &buffer_id);
     if (error) {
         VLOG_WARN_RL(&rl, "failed to decode packet-in: %s",
                      ofperr_to_string(error));
@@ -538,8 +540,8 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh)
         return;
     }
 
-    /* Extract flow data from 'opi' into 'flow'. */
-    dp_packet_use_const(&pkt, pi.packet, pi.packet_len);
+    /* Extract flow data from 'pi' into 'flow'. */
+    dp_packet_use_const(&pkt, pi.packet, pi.len);
     flow_extract(&pkt, &flow);
     flow.in_port.ofp_port = pi.flow_metadata.flow.in_port.ofp_port;
     flow.tunnel.tun_id = pi.flow_metadata.flow.tunnel.tun_id;
@@ -562,8 +564,8 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh)
     }
 
     /* Prepare packet_out in case we need one. */
-    po.buffer_id = pi.buffer_id;
-    if (po.buffer_id == UINT32_MAX) {
+    po.buffer_id = buffer_id;
+    if (buffer_id == UINT32_MAX) {
         po.packet = dp_packet_data(&pkt);
         po.packet_len = dp_packet_size(&pkt);
     } else {
@@ -583,7 +585,7 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh)
             .table_id = 0xff,
             .command = OFPFC_ADD,
             .idle_timeout = sw->max_idle,
-            .buffer_id = pi.buffer_id,
+            .buffer_id = buffer_id,
             .out_port = OFPP_NONE,
             .ofpacts = ofpacts.data,
             .ofpacts_len = ofpacts.size,
@@ -596,13 +598,13 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh)
         queue_tx(sw, buffer);
 
         /* If the switch didn't buffer the packet, we need to send a copy. */
-        if (pi.buffer_id == UINT32_MAX && out_port != OFPP_NONE) {
+        if (buffer_id == UINT32_MAX && out_port != OFPP_NONE) {
             queue_tx(sw, ofputil_encode_packet_out(&po, sw->protocol));
         }
     } else {
         /* We don't know that MAC, or we don't set up flows.  Send along the
          * packet without setting up a flow. */
-        if (pi.buffer_id != UINT32_MAX || out_port != OFPP_NONE) {
+        if (buffer_id != UINT32_MAX || out_port != OFPP_NONE) {
             queue_tx(sw, ofputil_encode_packet_out(&po, sw->protocol));
         }
     }
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index bedfc94..5c9a6ab 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -100,9 +100,11 @@ ofp_print_packet_in(struct ds *string, const struct ofp_header *oh,
 {
     char reasonbuf[OFPUTIL_PACKET_IN_REASON_BUFSIZE];
     struct ofputil_packet_in pin;
+    uint32_t buffer_id;
+    size_t total_len;
     int error;
 
-    error = ofputil_decode_packet_in(&pin, oh);
+    error = ofputil_decode_packet_in(oh, &pin, &total_len, &buffer_id);
     if (error) {
         ofp_print_error(string, error);
         return;
@@ -116,35 +118,36 @@ ofp_print_packet_in(struct ds *string, const struct ofp_header *oh,
         ds_put_format(string, " cookie=0x%"PRIx64, ntohll(pin.cookie));
     }
 
-    ds_put_format(string, " total_len=%"PRIuSIZE" ", pin.total_len);
+    ds_put_format(string, " total_len=%"PRIuSIZE" ", total_len);
 
     match_format(&pin.flow_metadata, string, OFP_DEFAULT_PRIORITY);
 
     ds_put_format(string, " (via %s)",
-                  ofputil_packet_in_reason_to_string(pin.reason, reasonbuf,
+                  ofputil_packet_in_reason_to_string(pin.reason,
+                                                     reasonbuf,
                                                      sizeof reasonbuf));
 
-    ds_put_format(string, " data_len=%"PRIuSIZE, pin.packet_len);
-    if (pin.buffer_id == UINT32_MAX) {
+    ds_put_format(string, " data_len=%"PRIuSIZE, pin.len);
+    if (buffer_id == UINT32_MAX) {
         ds_put_format(string, " (unbuffered)");
-        if (pin.total_len != pin.packet_len) {
+        if (total_len != pin.len) {
             ds_put_format(string, " (***total_len != data_len***)");
         }
     } else {
-        ds_put_format(string, " buffer=0x%08"PRIx32, pin.buffer_id);
-        if (pin.total_len < pin.packet_len) {
+        ds_put_format(string, " buffer=0x%08"PRIx32, buffer_id);
+        if (total_len < pin.len) {
             ds_put_format(string, " (***total_len < data_len***)");
         }
     }
     ds_put_char(string, '\n');
 
     if (verbosity > 0) {
-        char *packet = ofp_packet_to_string(pin.packet, pin.packet_len);
+        char *packet = ofp_packet_to_string(pin.packet, pin.len);
         ds_put_cstr(string, packet);
         free(packet);
     }
     if (verbosity > 2) {
-        ds_put_hex_dump(string, pin.packet, pin.packet_len, 0, false);
+        ds_put_hex_dump(string, pin.packet, pin.len, 0, false);
     }
 }
 
@@ -2091,7 +2094,9 @@ ofp_print_set_async_config(struct ds *string, const struct ofp_header *oh,
 
                     reason = ofp_async_config_reason_to_string(
                         j, type, reasonbuf, sizeof reasonbuf);
-                    ds_put_format(string, " %s", reason);
+                    if (reason[0]) {
+                        ds_put_format(string, " %s", reason);
+                    }
                 }
             }
             if (!role) {
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 347cb61..48e2e8e 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -41,6 +41,7 @@
 #include "ofpbuf.h"
 #include "openflow/netronome-ext.h"
 #include "packets.h"
+#include "pktbuf.h"
 #include "random.h"
 #include "tun-metadata.h"
 #include "unaligned.h"
@@ -3311,9 +3312,18 @@ ofputil_encode_flow_removed(const struct ofputil_flow_removed *fr,
     return msg;
 }
 
+/* Decodes the packet-in message starting at 'oh' into '*pin'.  Populates
+ * 'pin->packet' and 'pin->len' with the part of the packet actually included
+ * in the message, and '*total_len' with the original length of the packet
+ * (which is larger than 'packet->len' if only part of the packet was
+ * included).  Stores the packet's buffer ID in '*buffer_id' (UINT32_MAX if it
+ * was not buffered).
+ *
+ * Returns 0 if successful, otherwise an OpenFlow error code. */
 enum ofperr
-ofputil_decode_packet_in(struct ofputil_packet_in *pin,
-                         const struct ofp_header *oh)
+ofputil_decode_packet_in(const struct ofp_header *oh,
+                         struct ofputil_packet_in *pin,
+                         size_t *total_len, uint32_t *buffer_id)
 {
     enum ofpraw raw;
     struct ofpbuf b;
@@ -3339,28 +3349,26 @@ ofputil_decode_packet_in(struct ofputil_packet_in *pin,
 
         pin->reason = opi->reason;
         pin->table_id = opi->table_id;
-        pin->buffer_id = ntohl(opi->buffer_id);
-        pin->total_len = ntohs(opi->total_len);
-
-        if (cookie) {
-            pin->cookie = *cookie;
-        }
+        *buffer_id = ntohl(opi->buffer_id);
+        *total_len = ntohs(opi->total_len);
+        pin->cookie = cookie ? *cookie : OVS_BE64_MAX;
 
         pin->packet = b.data;
-        pin->packet_len = b.size;
+        pin->len = b.size;
     } else if (raw == OFPRAW_OFPT10_PACKET_IN) {
         const struct ofp10_packet_in *opi;
 
         opi = ofpbuf_pull(&b, offsetof(struct ofp10_packet_in, data));
 
         pin->packet = opi->data;
-        pin->packet_len = b.size;
+        pin->len = b.size;
 
         match_init_catchall(&pin->flow_metadata);
-        match_set_in_port(&pin->flow_metadata, u16_to_ofp(ntohs(opi->in_port)));
+        match_set_in_port(&pin->flow_metadata,
+                          u16_to_ofp(ntohs(opi->in_port)));
         pin->reason = opi->reason;
-        pin->buffer_id = ntohl(opi->buffer_id);
-        pin->total_len = ntohs(opi->total_len);
+        *buffer_id = ntohl(opi->buffer_id);
+        *total_len = ntohs(opi->total_len);
     } else if (raw == OFPRAW_OFPT11_PACKET_IN) {
         const struct ofp11_packet_in *opi;
         ofp_port_t in_port;
@@ -3369,16 +3377,16 @@ ofputil_decode_packet_in(struct ofputil_packet_in *pin,
         opi = ofpbuf_pull(&b, sizeof *opi);
 
         pin->packet = b.data;
-        pin->packet_len = b.size;
+        pin->len = b.size;
 
-        pin->buffer_id = ntohl(opi->buffer_id);
+        *buffer_id = ntohl(opi->buffer_id);
         error = ofputil_port_from_ofp11(opi->in_port, &in_port);
         if (error) {
             return error;
         }
         match_init_catchall(&pin->flow_metadata);
         match_set_in_port(&pin->flow_metadata, in_port);
-        pin->total_len = ntohs(opi->total_len);
+        *total_len = ntohs(opi->total_len);
         pin->reason = opi->reason;
         pin->table_id = opi->table_id;
     } else if (raw == OFPRAW_NXT_PACKET_IN) {
@@ -3400,11 +3408,11 @@ ofputil_decode_packet_in(struct ofputil_packet_in *pin,
         pin->table_id = npi->table_id;
         pin->cookie = npi->cookie;
 
-        pin->buffer_id = ntohl(npi->buffer_id);
-        pin->total_len = ntohs(npi->total_len);
+        *buffer_id = ntohl(npi->buffer_id);
+        *total_len = ntohs(npi->total_len);
 
         pin->packet = b.data;
-        pin->packet_len = b.size;
+        pin->len = b.size;
     } else {
         OVS_NOT_REACHED();
     }
@@ -3412,77 +3420,103 @@ ofputil_decode_packet_in(struct ofputil_packet_in *pin,
     return 0;
 }
 
+static int
+encode_packet_in_reason(enum ofp_packet_in_reason reason,
+                        enum ofp_version version)
+{
+    switch (reason) {
+    case OFPR_NO_MATCH:
+    case OFPR_ACTION:
+    case OFPR_INVALID_TTL:
+        return reason;
+
+    case OFPR_ACTION_SET:
+    case OFPR_GROUP:
+    case OFPR_PACKET_OUT:
+        return version < OFP14_VERSION ? OFPR_ACTION : reason;
+
+    case OFPR_EXPLICIT_MISS:
+        return version < OFP13_VERSION ? OFPR_ACTION : OFPR_NO_MATCH;
+
+    case OFPR_IMPLICIT_MISS:
+        return OFPR_NO_MATCH;
+
+    case OFPR_N_REASONS:
+    default:
+        OVS_NOT_REACHED();
+    }
+}
+
 static struct ofpbuf *
-ofputil_encode_ofp10_packet_in(const struct ofputil_packet_in *pin)
+ofputil_encode_ofp10_packet_in(const struct ofputil_packet_in *pin,
+                               uint32_t buffer_id)
 {
     struct ofp10_packet_in *opi;
-    struct ofpbuf *packet;
+    struct ofpbuf *msg;
 
-    packet = ofpraw_alloc_xid(OFPRAW_OFPT10_PACKET_IN, OFP10_VERSION,
-                              htonl(0), pin->packet_len);
-    opi = ofpbuf_put_zeros(packet, offsetof(struct ofp10_packet_in, data));
-    opi->total_len = htons(pin->total_len);
+    msg = ofpraw_alloc_xid(OFPRAW_OFPT10_PACKET_IN, OFP10_VERSION,
+                           htonl(0), pin->len);
+    opi = ofpbuf_put_zeros(msg, offsetof(struct ofp10_packet_in, data));
+    opi->total_len = htons(pin->len);
     opi->in_port = htons(ofp_to_u16(pin->flow_metadata.flow.in_port.ofp_port));
-    opi->reason = pin->reason;
-    opi->buffer_id = htonl(pin->buffer_id);
-
-    ofpbuf_put(packet, pin->packet, pin->packet_len);
+    opi->reason = encode_packet_in_reason(pin->reason, OFP10_VERSION);
+    opi->buffer_id = htonl(buffer_id);
 
-    return packet;
+    return msg;
 }
 
 static struct ofpbuf *
-ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin)
+ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin,
+                            uint32_t buffer_id)
 {
     struct nx_packet_in *npi;
-    struct ofpbuf *packet;
+    struct ofpbuf *msg;
     size_t match_len;
 
     /* The final argument is just an estimate of the space required. */
-    packet = ofpraw_alloc_xid(OFPRAW_NXT_PACKET_IN, OFP10_VERSION,
-                              htonl(0), NXM_TYPICAL_LEN + 2 + pin->packet_len);
-    ofpbuf_put_zeros(packet, sizeof *npi);
-    match_len = nx_put_match(packet, &pin->flow_metadata, 0, 0);
-    ofpbuf_put_zeros(packet, 2);
-    ofpbuf_put(packet, pin->packet, pin->packet_len);
-
-    npi = packet->msg;
-    npi->buffer_id = htonl(pin->buffer_id);
-    npi->total_len = htons(pin->total_len);
-    npi->reason = pin->reason;
+    msg = ofpraw_alloc_xid(OFPRAW_NXT_PACKET_IN, OFP10_VERSION,
+                           htonl(0), NXM_TYPICAL_LEN + 2 + pin->len);
+    ofpbuf_put_zeros(msg, sizeof *npi);
+    match_len = nx_put_match(msg, &pin->flow_metadata, 0, 0);
+    ofpbuf_put_zeros(msg, 2);
+
+    npi = msg->msg;
+    npi->buffer_id = htonl(buffer_id);
+    npi->total_len = htons(pin->len);
+    npi->reason = encode_packet_in_reason(pin->reason, OFP10_VERSION);
     npi->table_id = pin->table_id;
     npi->cookie = pin->cookie;
     npi->match_len = htons(match_len);
 
-    return packet;
+    return msg;
 }
 
 static struct ofpbuf *
-ofputil_encode_ofp11_packet_in(const struct ofputil_packet_in *pin)
+ofputil_encode_ofp11_packet_in(const struct ofputil_packet_in *pin,
+                               uint32_t buffer_id)
 {
     struct ofp11_packet_in *opi;
-    struct ofpbuf *packet;
+    struct ofpbuf *msg;
 
-    packet = ofpraw_alloc_xid(OFPRAW_OFPT11_PACKET_IN, OFP11_VERSION,
-                              htonl(0), pin->packet_len);
-    opi = ofpbuf_put_zeros(packet, sizeof *opi);
-    opi->buffer_id = htonl(pin->buffer_id);
-    opi->in_port = ofputil_port_to_ofp11(pin->flow_metadata.flow.in_port.ofp_port);
+    msg = ofpraw_alloc_xid(OFPRAW_OFPT11_PACKET_IN, OFP11_VERSION,
+                           htonl(0), pin->len);
+    opi = ofpbuf_put_zeros(msg, sizeof *opi);
+    opi->buffer_id = htonl(buffer_id);
+    opi->in_port = ofputil_port_to_ofp11(
+        pin->flow_metadata.flow.in_port.ofp_port);
     opi->in_phy_port = opi->in_port;
-    opi->total_len = htons(pin->total_len);
-    opi->reason = pin->reason;
+    opi->total_len = htons(pin->len);
+    opi->reason = encode_packet_in_reason(pin->reason, OFP11_VERSION);
     opi->table_id = pin->table_id;
 
-    ofpbuf_put(packet, pin->packet, pin->packet_len);
-
-    return packet;
+    return msg;
 }
 
 static struct ofpbuf *
 ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
-                               enum ofputil_protocol protocol)
+                               enum ofp_version version,
+                               uint32_t buffer_id)
 {
-    enum ofp_version version = ofputil_protocol_to_ofp_version(protocol);
     enum ofpraw raw = (version >= OFP13_VERSION
                        ? OFPRAW_OFPT13_PACKET_IN
                        : OFPRAW_OFPT12_PACKET_IN);
@@ -3490,13 +3524,14 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
 
     /* The final argument is just an estimate of the space required. */
     msg = ofpraw_alloc_xid(raw, version,
-                           htonl(0), NXM_TYPICAL_LEN + 2 + pin->packet_len);
+                           htonl(0), NXM_TYPICAL_LEN + 2 + pin->len);
 
     struct ofp12_packet_in *opi = ofpbuf_put_zeros(msg, sizeof *opi);
-    opi->buffer_id = htonl(pin->buffer_id);
-    opi->total_len = htons(pin->total_len);
-    opi->reason = pin->reason;
+    opi->buffer_id = htonl(buffer_id);
+    opi->total_len = htons(pin->len);
+    opi->reason = encode_packet_in_reason(pin->reason, version);
     opi->table_id = pin->table_id;
+
     if (version >= OFP13_VERSION) {
         ovs_be64 cookie = pin->cookie;
         ofpbuf_put(msg, &cookie, sizeof cookie);
@@ -3504,47 +3539,70 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
 
     oxm_put_match(msg, &pin->flow_metadata, version);
     ofpbuf_put_zeros(msg, 2);
-    ofpbuf_put(msg, pin->packet, pin->packet_len);
 
     return msg;
 }
 
-/* Converts abstract ofputil_packet_in 'pin' into a PACKET_IN message
- * in the format specified by 'packet_in_format'.  */
+/* Converts abstract ofputil_packet_in 'pin' into a PACKET_IN message for
+ * 'protocol', using the packet-in format specified by 'packet_in_format'.
+ *
+ * If 'pkt_buf' is nonnull and 'max_len' allows the packet to be buffered, this
+ * function will attempt to obtain a buffer ID from 'pktbuf' and truncate the
+ * packet to 'max_len' bytes.  Otherwise, or if 'pktbuf' doesn't have a free
+ * buffer, it will send the whole packet without buffering. */
 struct ofpbuf *
 ofputil_encode_packet_in(const struct ofputil_packet_in *pin,
                          enum ofputil_protocol protocol,
-                         enum nx_packet_in_format packet_in_format)
+                         enum nx_packet_in_format packet_in_format,
+                         uint16_t max_len, struct pktbuf *pktbuf)
 {
-    struct ofpbuf *packet;
+    enum ofp_version version = ofputil_protocol_to_ofp_version(protocol);
+
+    /* Get buffer ID. */
+    ofp_port_t in_port = pin->flow_metadata.flow.in_port.ofp_port;
+    uint32_t buffer_id = (max_len != OFPCML12_NO_BUFFER && pktbuf
+                          ? pktbuf_save(pktbuf, pin->packet,
+                                        pin->len, in_port)
+                          : UINT32_MAX);
 
+    struct ofpbuf *msg;
     switch (protocol) {
     case OFPUTIL_P_OF10_STD:
     case OFPUTIL_P_OF10_STD_TID:
     case OFPUTIL_P_OF10_NXM:
     case OFPUTIL_P_OF10_NXM_TID:
-        packet = (packet_in_format == NXPIF_NXM
-                  ? ofputil_encode_nx_packet_in(pin)
-                  : ofputil_encode_ofp10_packet_in(pin));
+        msg = (packet_in_format == NXPIF_NXM
+               ? ofputil_encode_nx_packet_in(pin, buffer_id)
+               : ofputil_encode_ofp10_packet_in(pin, buffer_id));
         break;
 
     case OFPUTIL_P_OF11_STD:
-        packet = ofputil_encode_ofp11_packet_in(pin);
+        msg = ofputil_encode_ofp11_packet_in(pin, buffer_id);
         break;
 
     case OFPUTIL_P_OF12_OXM:
     case OFPUTIL_P_OF13_OXM:
     case OFPUTIL_P_OF14_OXM:
     case OFPUTIL_P_OF15_OXM:
-        packet = ofputil_encode_ofp12_packet_in(pin, protocol);
+        msg = ofputil_encode_ofp12_packet_in(pin, version, buffer_id);
         break;
 
     default:
         OVS_NOT_REACHED();
     }
 
-    ofpmsg_update_length(packet);
-    return packet;
+    /* Append some of the packet:
+     *
+     *    - If not buffered, the whole thing.
+     *
+     *    - Otherwise, no more than 'max_len' bytes. */
+    ofpbuf_put(msg, pin->packet,
+               (buffer_id == UINT32_MAX
+                ? pin->len
+                : MIN(max_len, pin->len)));
+
+    ofpmsg_update_length(msg);
+    return msg;
 }
 
 /* Returns a string form of 'reason'.  The return value is either a statically
@@ -3567,6 +3625,9 @@ ofputil_packet_in_reason_to_string(enum ofp_packet_in_reason reason,
         return "group";
     case OFPR_PACKET_OUT:
         return "packet_out";
+    case OFPR_EXPLICIT_MISS:
+    case OFPR_IMPLICIT_MISS:
+        return "";
 
     case OFPR_N_REASONS:
     default:
@@ -9496,6 +9557,15 @@ decode_async_mask(ovs_be32 src,
         }
     }
 
+    if (ap->oam == OAM_PACKET_IN) {
+        if (mask & (1u << OFPR_NO_MATCH)) {
+            mask |= 1u << OFPR_EXPLICIT_MISS;
+            if (version < OFP13_VERSION) {
+                mask |= 1u << OFPR_IMPLICIT_MISS;
+            }
+        }
+    }
+
     uint32_t *array = ap->master ? dst->master : dst->slave;
     array[ap->oam] = mask;
     return 0;
@@ -9723,10 +9793,19 @@ ofputil_encode_set_async_config(const struct ofputil_async_cfg *ac,
 struct ofputil_async_cfg
 ofputil_async_cfg_default(enum ofp_version version)
 {
+    /* We enable all of the OF1.4 reasons regardless of 'version' because the
+     * reasons added in OF1.4 just are just refinements of the OFPR_ACTION
+     * introduced in OF1.0, breaking it into more specific categories.  When we
+     * encode these for earlier OpenFlow versions, we translate them into
+     * OFPR_ACTION.  */
+    uint32_t pin = OFPR14_BITS & ~(1u << OFPR_INVALID_TTL);
+    pin |= 1u << OFPR_EXPLICIT_MISS;
+    if (version <= OFP12_VERSION) {
+        pin |= 1u << OFPR_IMPLICIT_MISS;
+    }
+
     return (struct ofputil_async_cfg) {
-        .master[OAM_PACKET_IN]
-            = ((version >= OFP14_VERSION ? OFPR14_BITS : OFPR10_BITS)
-               & ~(1u << OFPR_INVALID_TTL)),
+        .master[OAM_PACKET_IN] = pin,
 
         .master[OAM_FLOW_REMOVED]
             = (version >= OFP14_VERSION ? OFPRR14_BITS : OFPRR10_BITS),
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 1c359b3..cf77d8a 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -35,6 +35,7 @@
 struct ofpbuf;
 union ofp_action;
 struct ofpact_set_field;
+struct pktbuf;
 
 /* Port numbers. */
 enum ofperr ofputil_port_from_ofp11(ovs_be32 ofp11_port,
@@ -411,25 +412,25 @@ enum ofperr ofputil_decode_flow_removed(struct ofputil_flow_removed *,
 struct ofpbuf *ofputil_encode_flow_removed(const struct ofputil_flow_removed *,
                                            enum ofputil_protocol);
 
-/* Abstract packet-in message. */
+/* Abstract packet-in message.
+ *
+ * This omits the 'total_len' and 'buffer_id' fields, which we handle
+ * differently for encoding and decoding.*/
 struct ofputil_packet_in {
     /* Packet data and metadata.
      *
-     * To save bandwidth, in some cases a switch may send only the first
-     * several bytes of a packet, indicated by 'packet_len < total_len'.  When
-     * the full packet is included, 'packet_len == total_len'. */
-    const void *packet;
-    size_t packet_len;          /* Number of bytes in 'packet'. */
-    size_t total_len;           /* Size of packet, pre-truncation. */
-    struct match flow_metadata;
-
-    /* Identifies a buffer in the switch that contains the full packet, to
-     * allow the controller to reference it later without having to send the
-     * entire packet back to the switch.
+     * On encoding, the full packet should be supplied, but depending on its
+     * other parameters ofputil_encode_packet_in() might send only the first
+     * part of the packet.
      *
-     * UINT32_MAX indicates that the packet is not buffered in the switch.  A
-     * switch should only use UINT32_MAX when it sends the entire packet. */
-    uint32_t buffer_id;
+     * On decoding, the 'len' bytes in 'packet' might only be the first part of
+     * the original packet.  ofputil_decode_packet_in() reports the full
+     * original length of the packet using its 'total_len' output parameter. */
+    const void *packet;         /* The packet. */
+    size_t len;                 /* Length of 'packet' in bytes. */
+
+    /* Input port and other metadata for packet. */
+    struct match flow_metadata;
 
     /* Reason that the packet-in is being sent. */
     enum ofp_packet_in_reason reason;    /* One of OFPR_*. */
@@ -442,11 +443,14 @@ struct ofputil_packet_in {
     ovs_be64 cookie;                     /* Flow's cookie. */
 };
 
-enum ofperr ofputil_decode_packet_in(struct ofputil_packet_in *,
-                                     const struct ofp_header *);
 struct ofpbuf *ofputil_encode_packet_in(const struct ofputil_packet_in *,
                                         enum ofputil_protocol protocol,
-                                        enum nx_packet_in_format);
+                                        enum nx_packet_in_format,
+                                        uint16_t max_len, struct pktbuf *);
+
+enum ofperr ofputil_decode_packet_in(const struct ofp_header *,
+                                     struct ofputil_packet_in *,
+                                     size_t *total_len, uint32_t *buffer_id);
 
 enum { OFPUTIL_PACKET_IN_REASON_BUFSIZE = INT_STRLEN(int) + 1 };
 const char *ofputil_packet_in_reason_to_string(enum ofp_packet_in_reason,
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index c3e486c..fb8f251 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1483,33 +1483,6 @@ ofconn_receives_async_msg(const struct ofconn *ofconn,
 /* The default "table-miss" behaviour for OpenFlow1.3+ is to drop the
  * packet rather than to send the packet to the controller.
  *
- * This function returns false to indicate the packet should be dropped if
- * the controller action was the result of the default table-miss behaviour
- * and the controller is using OpenFlow1.3+.
- *
- * Otherwise true is returned to indicate the packet should be forwarded to
- * the controller */
-static bool
-ofconn_wants_packet_in_on_miss(struct ofconn *ofconn,
-                               const struct ofproto_packet_in *pin)
-{
-    if (pin->miss_type == OFPROTO_PACKET_IN_MISS_WITHOUT_FLOW) {
-        enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
-
-        if (protocol != OFPUTIL_P_NONE
-            && ofputil_protocol_to_ofp_version(protocol) >= OFP13_VERSION
-            && (ofproto_table_get_miss_config(ofconn->connmgr->ofproto,
-                                              pin->up.table_id)
-                == OFPUTIL_TABLE_MISS_DEFAULT)) {
-            return false;
-        }
-    }
-    return true;
-}
-
-/* The default "table-miss" behaviour for OpenFlow1.3+ is to drop the
- * packet rather than to send the packet to the controller.
- *
  * This function returns true to indicate that a packet_in message
  * for a "table-miss" should be sent to at least one controller.
  * That is there is at least one controller with controller_id 0
@@ -1582,9 +1555,6 @@ ofconn_send(const struct ofconn *ofconn, struct ofpbuf *msg,
 
 /* Sending asynchronous messages. */
 
-static void schedule_packet_in(struct ofconn *, struct ofproto_packet_in,
-                               enum ofp_packet_in_reason wire_reason);
-
 /* Sends an OFPT_PORT_STATUS message with 'opp' and 'reason' to appropriate
  * controllers managed by 'mgr'.  For messages caused by a controller
  * OFPT_PORT_MOD, specify 'source' as the controller connection that sent the
@@ -1678,41 +1648,6 @@ connmgr_send_flow_removed(struct connmgr *mgr,
     }
 }
 
-/* Normally a send-to-controller action uses reason OFPR_ACTION.  However, in
- * OpenFlow 1.3 and later, packet_ins generated by a send-to-controller action
- * in a "table-miss" flow (one with priority 0 and completely wildcarded) are
- * sent as OFPR_NO_MATCH.  This function returns the reason that should
- * actually be sent on 'ofconn' for 'pin'. */
-static enum ofp_packet_in_reason
-wire_reason(struct ofconn *ofconn, const struct ofproto_packet_in *pin)
-{
-    enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
-
-    if (pin->miss_type == OFPROTO_PACKET_IN_MISS_FLOW
-        && pin->up.reason == OFPR_ACTION
-        && protocol != OFPUTIL_P_NONE
-        && ofputil_protocol_to_ofp_version(protocol) >= OFP13_VERSION) {
-        return OFPR_NO_MATCH;
-    }
-
-    switch (pin->up.reason) {
-    case OFPR_ACTION_SET:
-    case OFPR_GROUP:
-    case OFPR_PACKET_OUT:
-        if (!(protocol & OFPUTIL_P_OF14_UP)) {
-            /* Only supported in OF1.4+ */
-            return OFPR_ACTION;
-        }
-        /* Fall through. */
-	case OFPR_NO_MATCH:
-	case OFPR_ACTION:
-	case OFPR_INVALID_TTL:
-	case OFPR_N_REASONS:
-    default:
-        return pin->up.reason;
-    }
-}
-
 /* Given 'pin', sends an OFPT_PACKET_IN message to each OpenFlow controller as
  * necessary according to their individual configurations.
  *
@@ -1724,13 +1659,27 @@ connmgr_send_packet_in(struct connmgr *mgr,
     struct ofconn *ofconn;
 
     LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
-        enum ofp_packet_in_reason reason = wire_reason(ofconn, pin);
-
-        if (ofconn_wants_packet_in_on_miss(ofconn, pin)
-            && ofconn_receives_async_msg(ofconn, OAM_PACKET_IN, reason)
-            && ofconn->controller_id == pin->controller_id) {
-            schedule_packet_in(ofconn, *pin, reason);
+        enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
+        if (protocol == OFPUTIL_P_NONE || !rconn_is_connected(ofconn->rconn)
+            || ofconn->controller_id != pin->controller_id
+            || !ofconn_receives_async_msg(ofconn, OAM_PACKET_IN,
+                                          pin->up.reason)) {
+            continue;
         }
+
+        struct ofpbuf *msg = ofputil_encode_packet_in(
+            &pin->up, protocol, ofconn->packet_in_format,
+            pin->max_len >= 0 ? pin->max_len : ofconn->miss_send_len,
+            ofconn->pktbuf);
+
+        struct ovs_list txq;
+        bool is_miss = (pin->up.reason == OFPR_NO_MATCH ||
+                        pin->up.reason == OFPR_EXPLICIT_MISS ||
+                        pin->up.reason == OFPR_IMPLICIT_MISS);
+        pinsched_send(ofconn->schedulers[is_miss],
+                      pin->up.flow_metadata.flow.in_port.ofp_port /* XXX */,
+                      msg, &txq);
+        do_send_packet_ins(ofconn, &txq);
     }
 }
 
@@ -1749,55 +1698,6 @@ do_send_packet_ins(struct ofconn *ofconn, struct ovs_list *txq)
         }
     }
 }
-
-/* Takes 'pin', composes an OpenFlow packet-in message from it, and passes it
- * to 'ofconn''s packet scheduler for sending. */
-static void
-schedule_packet_in(struct ofconn *ofconn, struct ofproto_packet_in pin,
-                   enum ofp_packet_in_reason wire_reason)
-{
-    uint16_t controller_max_len;
-    struct ovs_list txq;
-
-    pin.up.total_len = pin.up.packet_len;
-
-    pin.up.reason = wire_reason;
-    if (pin.up.reason == OFPR_ACTION) {
-        controller_max_len = pin.send_len;  /* max_len */
-    } else {
-        controller_max_len = ofconn->miss_send_len;
-    }
-
-    /* Get OpenFlow buffer_id.
-     * For OpenFlow 1.2+, OFPCML_NO_BUFFER (== UINT16_MAX) specifies
-     * unbuffered.  This behaviour doesn't violate prior versions, too. */
-    if (controller_max_len == UINT16_MAX) {
-        pin.up.buffer_id = UINT32_MAX;
-    } else if (!ofconn->pktbuf) {
-        pin.up.buffer_id = UINT32_MAX;
-    } else {
-        pin.up.buffer_id = pktbuf_save(ofconn->pktbuf,
-                                       pin.up.packet, pin.up.packet_len,
-                                       pin.up.flow_metadata.flow.in_port.ofp_port);
-    }
-
-    /* Figure out how much of the packet to send.
-     * If not buffered, send the entire packet.  Otherwise, depending on
-     * the reason of packet-in, send what requested by the controller. */
-    if (pin.up.buffer_id != UINT32_MAX
-        && controller_max_len < pin.up.packet_len) {
-        pin.up.packet_len = controller_max_len;
-    }
-
-    /* Make OFPT_PACKET_IN and hand over to packet scheduler. */
-    pinsched_send(ofconn->schedulers[pin.up.reason == OFPR_NO_MATCH ? 0 : 1],
-                  pin.up.flow_metadata.flow.in_port.ofp_port,
-                  ofputil_encode_packet_in(&pin.up,
-                                           ofconn_get_protocol(ofconn),
-                                           ofconn->packet_in_format),
-                  &txq);
-    do_send_packet_ins(ofconn, &txq);
-}
 
 /* Fail-open settings. */
 
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 98821bc..ced6a68 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -54,32 +54,12 @@ enum ofconn_type {
     OFCONN_SERVICE              /* A service connection, e.g. "ovs-ofctl". */
 };
 
-enum ofproto_packet_in_miss_type {
-    /* Not generated by a flow miss or table-miss flow. */
-    OFPROTO_PACKET_IN_NO_MISS,
-
-    /* The packet_in was generated directly by a table-miss flow, that is, a
-     * flow with priority 0 that wildcards all fields.  See OF1.3.3 section
-     * 5.4.
-     *
-     * (Our interpretation of "directly" is "not via groups".  Packet_ins
-     * generated by table-miss flows via groups use
-     * OFPROTO_PACKET_IN_NO_MISS.) */
-    OFPROTO_PACKET_IN_MISS_FLOW,
-
-    /* The packet-in was generated directly by a table-miss, but not a
-     * table-miss flow.  That is, it was generated by the OpenFlow 1.0, 1.1, or
-     * 1.2 table-miss behavior. */
-    OFPROTO_PACKET_IN_MISS_WITHOUT_FLOW,
-};
-
 /* A packet_in, with extra members to assist in queuing and routing it. */
 struct ofproto_packet_in {
     struct ofputil_packet_in up;
     struct ovs_list list_node;  /* For queuing. */
     uint16_t controller_id;     /* Controller ID to send to. */
-    int send_len;               /* Length that the action requested sending. */
-    enum ofproto_packet_in_miss_type miss_type;
+    int max_len;                /* From action, or -1 if none. */
 };
 
 /* Basics. */
diff --git a/ofproto/fail-open.c b/ofproto/fail-open.c
index 3c9a9ad..5a692bf 100644
--- a/ofproto/fail-open.c
+++ b/ofproto/fail-open.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -118,7 +118,6 @@ fail_open_is_active(const struct fail_open *fo)
 static void
 send_bogus_packet_ins(struct fail_open *fo)
 {
-    struct ofproto_packet_in pin;
     struct eth_addr mac;
     struct dp_packet b;
 
@@ -126,14 +125,18 @@ send_bogus_packet_ins(struct fail_open *fo)
     eth_addr_nicira_random(&mac);
     compose_rarp(&b, mac);
 
-    memset(&pin, 0, sizeof pin);
-    pin.up.packet = dp_packet_data(&b);
-    pin.up.packet_len = dp_packet_size(&b);
-    pin.up.reason = OFPR_NO_MATCH;
-    match_init_catchall(&pin.up.flow_metadata);
-    match_set_in_port(&pin.up.flow_metadata, OFPP_LOCAL);
-    pin.send_len = dp_packet_size(&b);
-    pin.miss_type = OFPROTO_PACKET_IN_NO_MISS;
+    struct ofproto_packet_in pin = {
+        .up = {
+            .packet = dp_packet_data(&b),
+            .len = dp_packet_size(&b),
+            .flow_metadata = MATCH_CATCHALL_INITIALIZER,
+            .flow_metadata.flow.in_port.ofp_port = OFPP_LOCAL,
+            .flow_metadata.wc.masks.in_port.ofp_port = u16_to_ofp(UINT16_MAX),
+            .reason = OFPR_NO_MATCH,
+            .cookie = OVS_BE64_MAX,
+        },
+        .max_len = UINT16_MAX,
+    };
     connmgr_send_packet_in(fo->connmgr, &pin);
 
     dp_packet_uninit(&b);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index e2ca960..c377963 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -3582,33 +3582,30 @@ execute_controller_action(struct xlate_ctx *ctx, int len,
     odp_execute_actions(NULL, &packet, 1, false,
                         ctx->odp_actions->data, ctx->odp_actions->size, NULL);
 
-    pin = xmalloc(sizeof *pin);
-    pin->up.packet_len = dp_packet_size(packet);
-    pin->up.packet = dp_packet_steal_data(packet);
-    pin->up.reason = reason;
-    pin->up.table_id = ctx->table_id;
-    pin->up.cookie = ctx->rule_cookie;
+    /* A packet sent by an action in a table-miss rule is considered an
+     * explicit table miss.  OpenFlow before 1.3 doesn't have that concept so
+     * it will get translated back to OFPR_ACTION for those versions. */
+    if (reason == OFPR_ACTION
+        && ctx->rule && rule_dpif_is_table_miss(ctx->rule)) {
+        reason = OFPR_EXPLICIT_MISS;
+    }
+
+    size_t packet_len = dp_packet_size(packet);
 
+    pin = xmalloc(sizeof *pin);
+    *pin = (struct ofproto_packet_in) {
+        .controller_id = controller_id,
+        .up = {
+            .packet = dp_packet_steal_data(packet),
+            .len = packet_len,
+            .reason = reason,
+            .table_id = ctx->table_id,
+            .cookie = ctx->rule_cookie,
+        },
+        .max_len = len,
+    };
     flow_get_metadata(&ctx->xin->flow, &pin->up.flow_metadata);
 
-    pin->controller_id = controller_id;
-    pin->send_len = len;
-    /* If a rule is a table-miss rule then this is
-     * a table-miss handled by a table-miss rule.
-     *
-     * Else, if rule is internal and has a controller action,
-     * the later being implied by the rule being processed here,
-     * then this is a table-miss handled without a table-miss rule.
-     *
-     * Otherwise this is not a table-miss. */
-    pin->miss_type = OFPROTO_PACKET_IN_NO_MISS;
-    if (ctx->rule) {
-        if (rule_dpif_is_table_miss(ctx->rule)) {
-            pin->miss_type = OFPROTO_PACKET_IN_MISS_FLOW;
-        } else if (rule_dpif_is_internal(ctx->rule)) {
-            pin->miss_type = OFPROTO_PACKET_IN_MISS_WITHOUT_FLOW;
-        }
-    }
     ofproto_dpif_send_packet_in(ctx->xbridge->ofproto, pin);
     dp_packet_delete(packet);
 }
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 8186f6b..fbb392f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1406,7 +1406,7 @@ add_internal_flows(struct ofproto_dpif *ofproto)
     controller = ofpact_put_CONTROLLER(&ofpacts);
     controller->max_len = UINT16_MAX;
     controller->controller_id = 0;
-    controller->reason = OFPR_NO_MATCH;
+    controller->reason = OFPR_IMPLICIT_MISS;
 
     error = add_internal_miss_flow(ofproto, id++, &ofpacts,
                                    &ofproto->miss_rule);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index cbd9c47..3faf42a 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3378,7 +3378,7 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
     /* Get payload. */
     if (po.buffer_id != UINT32_MAX) {
         error = ofconn_pktbuf_retrieve(ofconn, po.buffer_id, &payload, NULL);
-        if (error || !payload) {
+        if (error) {
             goto exit_free_ofpacts;
         }
     } else {
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 8c53c19..360f38b 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -78,8 +78,10 @@ process_packet_in(struct controller_ctx *ctx OVS_UNUSED,
                   const struct ofp_header *msg)
 {
     struct ofputil_packet_in pin;
+    uint32_t buffer_id;
+    size_t total_len;
 
-    if (ofputil_decode_packet_in(&pin, msg) != 0) {
+    if (ofputil_decode_packet_in(msg, &pin, &total_len, &buffer_id) != 0) {
         return;
     }
     if (pin.reason != OFPR_ACTION) {
-- 
2.1.3





More information about the dev mailing list