On Fri, Jul 31, 2015 at 03:12:38PM -0700, Justin Pettit wrote:
> > On Jul 28, 2015, at 8:44 AM, Ben Pfaff <b...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
