[ovs-dev] [PATCH v2 21/21] ovn: Change strategy for tunnel keys.

[Ben Pfaff]

I applied most of your comments as obvious.  A few deserved responses,
see below.

On Mon, Aug 03, 2015 at 01:19:01AM -0700, Justin Pettit wrote:
> 
> > On Jul 28, 2015, at 8:44 AM, Ben Pfaff <blp at nicira.com> wrote:
> > +        /* Translate logical table ID to physical table ID. */
> > +        bool ingress = !strcmp(rule->pipeline, "ingress");
> > +        uint8_t phys_table = rule->table_id + (ingress ? 16 : 48);
> > +        uint8_t next_phys_table = rule->table_id < 15 ? phys_table + 1 : 0;
> > +        uint8_t output_phys_table = ingress ? 32 : 64;
> 
> I think these numbers could use some #define's.  In addition to making
> it easier to change later on, the code will be easier to understand.

OK, I added macros:

/* OpenFlow table numbers.
 *
 * These are heavily documented in ovn-architecture(7), please update it if
 * you make any changes. */
#define OFTABLE_PHY_TO_LOG            0
#define OFTABLE_LOG_INGRESS_PIPELINE 16 /* First of LOG_PIPELINE_LEN tables. */
#define OFTABLE_REMOTE_OUTPUT        32
#define OFTABLE_LOCAL_OUTPUT         33
#define OFTABLE_DROP_LOOPBACK        34
#define OFTABLE_LOG_EGRESS_PIPELINE  48 /* First of LOG_PIPELINE_LEN tables. */
#define OFTABLE_LOG_TO_PHY           64

/* The number of tables for the ingress and egress pipelines. */
#define LOG_PIPELINE_LEN 16

> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index eac5546..5ecd13e 100644
> > 
> > ...
> > 
> > +static void
> > +keys_destroy(struct hmap *keys)
> 
> Very minor, but with add_key() and allocate_key(), it would be more
> consistent to call this destroy_keys().  And, since I'm already
> commenting on this function, all the other functions use "set" instead
> of "keys" for this argument.  :-)

I switched from "keys" to "tnlids" here to avoid confusion.

> > +    uint32_t max_port_key;
> 
> I think "prev_port_key" would be a more accurate name.

Well, it really is the max initially when we scan for the in-use keys.
I guess it's the previous key after that.

I changed it to "port_key_hint".

> > +static void
> > +join_datapaths(struct northd_context *ctx, struct hmap *dp_map,
> > +               struct ovs_list *sb_only, struct ovs_list *nb_only,
> > +               struct ovs_list *both)
> > +{
> > +    hmap_init(dp_map);
> 
> I don't think "dp_map" gets destroyed.

I fixed this and other memory leaks.

> > +#define MC_FLOOD "_MC_flood"
> > +static const struct multicast_group mc_flood = { MC_FLOOD, 65535 };
> > +
> > +#define MC_UNKNOWN "_MC_unknown"
> > +static const struct multicast_group mc_unknown = { MC_UNKNOWN, 65534 };
> 
> Should we note somewhere (maybe in architecture) that these two
> multicast groups and their values?  It might be useful for debugging.

It's pretty easy to dump the table.

> Should we mention the apparent convention of starting a group name
> with an underscore?

I added a note to the Multicast_Group docs.

> > +      The <code>output</code> action also introduces recursion.  Its effect
> > +      depends on the current value of the <code>outport</code> field.  Suppose
> > +      <code>outport</code> designates a logical port.  First, OVN compares
> > +      <code>inport</code> to <code>outport</code>; if they are equal, it treats
> > +      the <code>output</code> as a no-op.  In the common case, where they are
> > +      different, the packet enters the egress pipeline.  This transition to the
> > +      egress pipeline discards register data (<code>reg0</code>
> > +      ... <code>reg5</code>).
> 
> It might be worth mentioning this is done because the egress pipeline
> may be on a different hypervisor, and the registers wouldn't survive.

OK, I added that.

> BTW, I didn't notice the implementation doing that, but it's possible
> that I missed it in my quick look in physical_run().

You're right, I fixed that.

> Also, do we know that "queue" will be an integer (and not a string)?
> Should it be deleted until it's implemented?

I deleted it.

> > +    <column name="external_ids" key="logical-switch" type='{"type": "uuid"}'>
> > +      Each row in <ref table="Datapath_Binding"/> is associated with some
> > +      logical datapath.  <code>ovn-northd</code> uses this key to store the
> > +      UUID of the logical datapath <ref table="Logical_Switch"
> > +      db="OVN_Northbound"/> row in the <ref db="OVN_Northbound"/> database.
> 
> It could also be a "Logical_Router", right?

I don't know, we haven't designed that yet, but it seems entirely
possible we'd use an external-ids:logical-router to designate that.