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

Justin Pettit jpettit at nicira.com
Mon Aug 3 18:48:41 UTC 2015


I forgot to give my ack:

Acked-by: Justin Pettit <jpettit at nicira.com>

--Justin


> On Jul 31, 2015, at 3:12 PM, Justin Pettit <jpettit at nicira.com> 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().
> 
>> +/* 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
> 
> 




More information about the dev mailing list