I forgot to give my ack:
Acked-by: Justin Pettit <jpet...@nicira.com>
--Justin
> On Jul 31, 2015, at 3:12 PM, Justin Pettit <jpet...@nicira.com> 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().
>
>> +/* 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
