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 <b...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
