[ovs-dev] [PATCH ovn v3 07/10] ovn: Add tunnel_key concept to Bindings table, assign in ovn-northd.

Justin Pettit jpettit at nicira.com
Wed Apr 29 00:20:13 UTC 2015


> On Apr 24, 2015, at 3:34 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> When packets travel among nodes in OVN over tunnels, a tunnel key value is
> needed to convey the logical port to which the packet is destined.  This
> commit adds a tunnel_key column to the Bindings table and adds code to
> ovn-northd to assign a unique tunnel_key value to each logical port.
> 
> Signed-off-by: Ben Pfaff <blp at nicira.com>

This patch (and the implementation that follows) goes the route of having each logical port have a unique tunnel key.  I had envisioned a different approach.  I'll briefly write it down here, but then it's probably worth discussing it more fully in a different forum (or at least a different thread).  I was thinking that we'd use a 24-bit VNI that would identify a logical switch.  In Geneve, it would be in the VNI field, and in STT, it would be in the lower 24 bits of the 64 bit key.  The local OVS would receive packets from a particular VNI, and then look at the DMAC to figure out which logical ports should receive it.

For port security, we would add a logical port that would uniquely identify a port within a particular VNI (ie, the value is only unique for a particular VNI).  For this, the logical port would only be used to identify the ingress port, since the receiving OVS would still be responsible for local replication and enforcing policies.  We would store the logical port in a TLV extension in Geneve and in the next (let's say) 16 bits of the STT header.

Okay, now that I've written that down, I'll review the code as you've implemented it.  We can have a follow-up discussion on the relative merits of each approach.

> +            /* Choose unique tunnel_key for the logical port. */
> +            static uint16_t next_tunnel_key = 1;
> +            while (tunnel_key_in_use(&tk_hmap, next_tunnel_key)) {
> +                next_tunnel_key++;
> +            }
> +            sbrec_bindings_set_tunnel_key(binding, next_tunnel_key++);

Were you trying to avoid a tunnel key of zero?  If so, I think we'll hit that on wrap-around.  Also, if we have 64K logical ports, I think this will get stuck in a permanent loop with no way to get out, since it's the only thing that ever deletes a tunnel_key.  Finally, should you add the new tunnel to tk_hmap so that if we're anywhere near full that we don't assign the same tunnel key twice?

> +    struct binding_hash_node *hash_node;
> +    HMAP_FOR_EACH (hash_node, lp_node, &lp_hmap) {
> +        hmap_remove(&lp_hmap, &hash_node->lp_node);

Should you be using HMAP_FOR_EACH_SAFE()?

> +                "tunnel_key": {
> +                     "type": {"key": {"type": "integer",
> +                                      "minInteger": 1,
> +                                      "maxInteger": 65535}}},

If we decide to go with a unique id for each logical port, I do worry that 64K is too small for the total number of logical ports supported by OVN.

--Justin





More information about the dev mailing list