fail-open: Drop some of the weirder special cases.

I don't have any real evidence that these special cases make a difference
in real-world cases.  The messages for the commits that add them are not
clear about the reasons for them.  I seem to recall that they were only
tested with the dummy controller inside OVS, which isn't very good evidence
for real controllers.  Finally, they cut across layers in nasty ways and
make it hard to better abstract packet-ins and packet buffering.

If these solve real problems then we can reconsider after some reports
come in.

Signed-off-by: Ben Pfaff <[email protected]>
Acked-by: Jarno Rajahalme <[email protected]>

ofproto/connmgr.c

@@ -1756,7 +1756,6 @@ static void
 schedule_packet_in(struct ofconn *ofconn, struct ofproto_packet_in pin,
                    enum ofp_packet_in_reason wire_reason)
 {
-    struct connmgr *mgr = ofconn->connmgr;
     uint16_t controller_max_len;
     struct ovs_list txq;
 
@@ -1774,8 +1773,6 @@ schedule_packet_in(struct ofconn *ofconn, struct ofproto_packet_in pin,
      * unbuffered.  This behaviour doesn't violate prior versions, too. */
     if (controller_max_len == UINT16_MAX) {
         pin.up.buffer_id = UINT32_MAX;
-    } else if (mgr->fail_open && fail_open_is_active(mgr->fail_open)) {
-        pin.up.buffer_id = pktbuf_get_null();
     } else if (!ofconn->pktbuf) {
         pin.up.buffer_id = UINT32_MAX;
     } else {

ofproto/ofproto-dpif-upcall.c

@@ -1090,31 +1090,6 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
     xlate_actions(&xin, &upcall->xout);
     upcall->xout_initialized = true;
 
-    /* Special case for fail-open mode.
-     *
-     * If we are in fail-open mode, but we are connected to a controller too,
-     * then we should send the packet up to the controller in the hope that it
-     * will try to set up a flow and thereby allow us to exit fail-open.
-     *
-     * See the top-level comment in fail-open.c for more information.
-     *
-     * Copy packets before they are modified by execution. */
-    if (upcall->xout.fail_open) {
-        const struct dp_packet *packet = upcall->packet;
-        struct ofproto_packet_in *pin;
-
-        pin = xmalloc(sizeof *pin);
-        pin->up.packet = xmemdup(dp_packet_data(packet), dp_packet_size(packet));
-        pin->up.packet_len = dp_packet_size(packet);
-        pin->up.reason = OFPR_NO_MATCH;
-        pin->up.table_id = 0;
-        pin->up.cookie = OVS_BE64_MAX;
-        flow_get_metadata(upcall->flow, &pin->up.flow_metadata);
-        pin->send_len = 0; /* Not used for flow table misses. */
-        pin->miss_type = OFPROTO_PACKET_IN_NO_MISS;
-        ofproto_dpif_send_packet_in(upcall->ofproto, pin);
-    }
-
     if (!upcall->xout.slow) {
         ofpbuf_use_const(&upcall->put_actions,
                          odp_actions->data, odp_actions->size);

ofproto/ofproto-dpif-xlate.c

@@ -5033,7 +5033,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
 {
     *xout = (struct xlate_out) {
         .slow = 0,
-        .fail_open = false,
         .recircs = RECIRC_REFS_EMPTY_INITIALIZER,
     };
 
@@ -5229,7 +5228,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
             ctx.xin->resubmit_hook(ctx.xin, ctx.rule, 0);
         }
     }
-    xout->fail_open = ctx.rule && rule_dpif_is_fail_open(ctx.rule);
 
     /* Get the proximate input port of the packet.  (If xin->recirc,
      * flow->in_port is the ultimate input port of the packet.) */

ofproto/ofproto-dpif-xlate.h

@@ -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.
@@ -39,7 +39,6 @@ struct xlate_cache;
 
 struct xlate_out {
     enum slow_path_reason slow; /* 0 if fast path may be used. */
-    bool fail_open;             /* Initial rule is fail open? */
 
     struct recirc_refs recircs; /* Recirc action IDs on which references are
                                  * held. */

ofproto/pktbuf.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 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.
@@ -29,7 +29,6 @@
 VLOG_DEFINE_THIS_MODULE(pktbuf);
 
 COVERAGE_DEFINE(pktbuf_buffer_unknown);
-COVERAGE_DEFINE(pktbuf_null_cookie);
 COVERAGE_DEFINE(pktbuf_retrieved);
 COVERAGE_DEFINE(pktbuf_reuse_error);
 
@@ -128,40 +127,12 @@ pktbuf_save(struct pktbuf *pb, const void *buffer, size_t buffer_size,
     return make_id(p - pb->packets, p->cookie);
 }
 
-/*
- * Allocates and returns a "null" packet buffer id.  The returned packet buffer
- * id is considered valid by pktbuf_retrieve(), but it is not associated with
- * actual buffered data.
- *
- * This function is always successful.
- *
- * This is useful in one special case: with the current OpenFlow design, the
- * "fail-open" code cannot always know whether a connection to a controller is
- * actually valid until it receives a OFPT_PACKET_OUT or OFPT_FLOW_MOD request,
- * but at that point the packet in question has already been forwarded (since
- * we are still in "fail-open" mode).  If the packet was buffered in the usual
- * way, then the OFPT_PACKET_OUT or OFPT_FLOW_MOD would cause a duplicate
- * packet in the network.  Null packet buffer ids identify such a packet that
- * has already been forwarded, so that Open vSwitch can quietly ignore the
- * request to re-send it.  (After that happens, the switch exits fail-open
- * mode.)
- *
- * See the top-level comment in fail-open.c for an overview.
- */
-uint32_t
-pktbuf_get_null(void)
-{
-    return make_id(0, COOKIE_MAX);
-}
-
 /* Attempts to retrieve a saved packet with the given 'id' from 'pb'.  Returns
  * 0 if successful, otherwise an OpenFlow error code.
  *
- * On success, ordinarily stores the buffered packet in '*bufferp' and the
- * OpenFlow port number on which the packet was received in '*in_port'.  The
- * caller becomes responsible for freeing the buffer.  However, if 'id'
- * identifies a "null" packet buffer (created with pktbuf_get_null()), stores
- * NULL in '*bufferp' and OFPP_NONE in '*in_port'.
+ * On success, stores the buffered packet in '*bufferp' and the OpenFlow port
+ * number on which the packet was received in '*in_port'.  The caller becomes
+ * responsible for freeing the buffer.
  *
  * 'in_port' may be NULL if the input port is not of interest.
  *
@@ -204,16 +175,11 @@ pktbuf_retrieve(struct pktbuf *pb, uint32_t id, struct dp_packet **bufferp,
             VLOG_WARN_RL(&rl, "attempt to reuse buffer %08"PRIx32, id);
             error = OFPERR_OFPBRC_BUFFER_EMPTY;
         }
-    } else if (id >> PKTBUF_BITS != COOKIE_MAX) {
+    } else {
         COVERAGE_INC(pktbuf_buffer_unknown);
         VLOG_WARN_RL(&rl, "cookie mismatch: %08"PRIx32" != %08"PRIx32,
                      id, (id & PKTBUF_MASK) | (p->cookie << PKTBUF_BITS));
         error = OFPERR_OFPBRC_BUFFER_UNKNOWN;
-    } else {
-        COVERAGE_INC(pktbuf_null_cookie);
-        VLOG_INFO_RL(&rl, "Received null cookie %08"PRIx32" (this is normal "
-                     "if the switch was recently in fail-open mode)", id);
-        error = 0;
     }
 error:
     *bufferp = NULL;

ofproto/pktbuf.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2011, 2012, 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.
@@ -31,7 +31,6 @@ struct pktbuf *pktbuf_create(void);
 void pktbuf_destroy(struct pktbuf *);
 uint32_t pktbuf_save(struct pktbuf *, const void *buffer, size_t buffer_size,
                      ofp_port_t in_port);
-uint32_t pktbuf_get_null(void);
 enum ofperr pktbuf_retrieve(struct pktbuf *, uint32_t id,
                             struct dp_packet **bufferp, ofp_port_t *in_port);
 void pktbuf_discard(struct pktbuf *, uint32_t id);