[ovs-dev] [PATCH 29/41] fail-open: Drop some of the weirder special cases.
Jarno Rajahalme
jarno at ovn.org
Wed Jan 20 00:32:35 UTC 2016
I never understood this special case anyway,
Acked-by: Jarno Rajahalme <jarno at ovn.org>
> On Jan 18, 2016, at 11:27 PM, Ben Pfaff <blp at ovn.org> wrote:
>
> 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 <blp at ovn.org>
> ---
> 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 295c03c..c3e486c 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 9dd7895..b505206 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 57d877f..e2ca960 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 02067a7..a135d8f 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 def0c92..0ff2c6f 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 a5cbcd6..307521a 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.1.3
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list