From 99520767ca72b422d83c1357d8f876a7ed1bf0d2 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 14 Jan 2016 20:10:43 -0800 Subject: [PATCH] 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 Acked-by: Jarno Rajahalme --- ofproto/connmgr.c | 3 --- ofproto/ofproto-dpif-upcall.c | 25 -------------------- ofproto/ofproto-dpif-xlate.c | 2 -- ofproto/ofproto-dpif-xlate.h | 3 +-- ofproto/pktbuf.c | 44 ++++------------------------------- ofproto/pktbuf.h | 3 +-- 6 files changed, 7 insertions(+), 73 deletions(-) diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index 295c03ca6..c3e486c2e 100644 --- a/ofproto/connmgr.c +++ b/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 { diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 9dd7895c3..b50520609 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/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); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 57d877ff3..e2ca96041 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/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.) */ diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h index 02067a7ac..a135d8f4d 100644 --- a/ofproto/ofproto-dpif-xlate.h +++ b/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. */ diff --git a/ofproto/pktbuf.c b/ofproto/pktbuf.c index def0c929d..0ff2c6f84 100644 --- a/ofproto/pktbuf.c +++ b/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; diff --git a/ofproto/pktbuf.h b/ofproto/pktbuf.h index a5cbcd64c..307521ad1 100644 --- a/ofproto/pktbuf.h +++ b/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); -- 2.20.1