[ovs-dev] [PATCH v2 20/21] ofctrl: Negotiate OVN Geneve option.

Ben Pfaff blp at nicira.com
Mon Aug 3 20:48:24 UTC 2015


On Fri, Jul 31, 2015 at 03:12:38PM -0700, Justin Pettit wrote:
> > On Jul 28, 2015, at 8:44 AM, Ben Pfaff <blp at nicira.com> wrote:
> > +static void
> > +recv_S_GENEVE_TABLE_MOD_SENT(const struct ofp_header *oh, enum ofptype type)
> > +{
> > +    if (oh->xid != xid && oh->xid != xid2) {
> > +        ofctrl_recv(oh, type);
> > +    } else if (oh->xid == xid2 && type == OFPTYPE_BARRIER_REPLY) {
> > +        state = S_CLEAR_FLOWS;
> > +    } else if (oh->xid == xid && type == OFPTYPE_ERROR) {
> > +        enum ofperr error = ofperr_decode_msg(oh, NULL);
> > +        if (error == OFPERR_NXGTMFC_ALREADY_MAPPED ||
> > +            error == OFPERR_NXGTMFC_DUP_ENTRY) {
> > +            VLOG_INFO("raced with another controller adding "
> > +                      "Geneve option (%s); trying again",
> > +                      ofperr_to_string(error));
> > +            state = S_NEW;
> 
> I think in this case, we'll still get the barrier message which may
> log an error in ofctrl_recv().  I think we may just want to ignore
> barriers in ofctrl_recv().

Good idea, done.

> > +/* S_UPDATE_FLOWS, for maintaining the flow table over time.
> > + *
> > + * Compare the installed flows to the ones we want.  Send OFPT_FLOW_MOD as
> > + * necessary.
> > + *
> > + * This is a terminal state.  We only transition out of it if the connection
> > + * drops. */
> > +
> > +static void
> > +run_S_UPDATE_FLOWS(void)
> > +{
> > +}
> > +
> > +
> > +static void
> > +recv_S_UPDATE_FLOWS(const struct ofp_header *oh, enum ofptype type)
> > +{
> > +    ofctrl_recv(oh, type);
> > +}
> 
> The comment makes it sound like this state itself is maintaining these
> flows as opposed to it happening by calls to ofctrl_put().  It might
> be useful to clarify that in the comment.

OK, I added a comment:

    static void
    run_S_UPDATE_FLOWS(void)
    {
        /* Nothing to do here.
         *
         * Being in this state enables ofctrl_put() to work, however. */
    }

> > @@ -377,26 +586,13 @@ queue_flow_mod(struct ofputil_flow_mod *fm)
> >     queue_msg(ofputil_encode_flow_mod(fm, OFPUTIL_P_OF13_OXM));
> > }
> > 
> > -static void
> > -ofctrl_update_flows(struct hmap *desired_flows)
> > +void
> > +ofctrl_put(struct hmap *flow_table)
> 
> I think it might be nice to add a comment to this function--especially
> that it will take care of removing all the members from "flow_table".

OK, done:

    /* Replaces the flow table on the switch, if possible, by the flows in
     * 'flow_table', which should have been added with ofctrl_add_flow().
     * Regardless of whether the flow table is updated, this deletes all of the
     * flows from 'flow_table' and frees them.  (The hmap itself isn't
     * destroyed.) */


> > +    if (state != S_UPDATE_FLOWS
> > +        || rconn_packet_counter_n_packets(tx_counter)) {
> > +        ovn_flow_table_clear(flow_table);
> > +        return;
> >     }
> 
> It might be nice to explain why we're bail out if there are any
> packets queued in the rconn.

OK, done:

    /* The flow table can be updated if the connection to the switch is up and
     * in the correct state and not backlogged with existing flow_mods.  (Our
     * criteria for being backlogged appear very conservative, but the socket
     * between ovn-controller and OVS provides some buffering.)  Otherwise,
     * discard the flows.  A solution to either of those problems will cause us
     * to wake up and retry. */

> >         if (br_int) {
> > +            enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int);
> > +
> >             struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
> >             rule_run(&ctx, &flow_table);
> > -            if (chassis_id) {
> >                 physical_run(&ctx, br_int, chassis_id, &flow_table);
> > +            if (chassis_id && mff_ovn_geneve) {
> 
> Won't this prevent OVN from coming up properly if Geneve isn't a
> supported protocol on the host or negotiation fails?  It would be nice
> to still come up if STT is configured and there are any Geneve
> problems.

Good point.  I dropped this check.

> Less importantly, do we want to bother with all the Geneve negotiation
> if only STT is needed on the host?

Maybe that is an optimization for later.  It would require us to keep
track of whether any HVs require Geneve and, if a new one comes along
that does, negotiate it at that point.

> > diff --git a/ovn/controller/physical.h b/ovn/controller/physical.h
> > index 9de76de..82baa2f 100644
> > --- a/ovn/controller/physical.h
> > +++ b/ovn/controller/physical.h
> > @@ -29,6 +29,13 @@ struct hmap;
> > struct ovsdb_idl;
> > struct ovsrec_bridge;
> > 
> > +/* OVN Geneve option information.
> > + *
> > + * Keep these in sync with the documentation in ovn-architecture(7). */
> > +#define OVN_GENEVE_CLASS 0xffff
> > +#define OVN_GENEVE_TYPE 0
> > +#define OVN_GENEVE_LEN 4
> 
> Are these values just placeholders until we have assigned numbers?  If
> so, it might be worth noting that they should change in the future.
> 
> This isn't particular to your patch, but I wonder if we should
> introduce a #define for 0xffff that indicates it's the Geneve
> Experimental class.

OK, I folded this in:

diff --git a/ovn/controller/physical.h b/ovn/controller/physical.h
index 46b5fc2..95e5a20 100644
--- a/ovn/controller/physical.h
+++ b/ovn/controller/physical.h
@@ -31,8 +31,10 @@ struct ovsrec_bridge;
 
 /* OVN Geneve option information.
  *
+ * These are placeholders until OVS is assigned a Geneve option class.
+ *
  * Keep these in sync with the documentation in ovn-architecture(7). */
-#define OVN_GENEVE_CLASS 0xffff
+#define OVN_GENEVE_CLASS 0xffff  /* Geneve experimental class. */
 #define OVN_GENEVE_TYPE 0
 #define OVN_GENEVE_LEN 4
 



More information about the dev mailing list