> 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().
> +/* 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.
> @@ -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".
> + 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.
> 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.
Less importantly, do we want to bother with all the Geneve negotiation if only
STT is needed on the host?
> 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.
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
