I think I understood it when I implemented it, but it's been a long time
and apparently I didn't write it down...
On Tue, Jan 19, 2016 at 04:32:35PM -0800, Jarno Rajahalme wrote:
> I never understood this special case anyway,
>
> Acked-by: Jarno Rajahalme <ja...@ovn.org>
>
> > On Jan 18, 2016, at 11:27 PM, Ben Pfaff <b...@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 <b...@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@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
