[ovs-dev] [RFC flow tunnels 7/8] lib: Switch to flow based tunneling.

[Ben Pfaff]

On Wed, Jan 09, 2013 at 03:43:47PM -0800, Ethan Jackson wrote:
> With this patch, ovs-vswitchd uses flow based tunneling
> exclusively.  I.E. each kind of tunnel shares a single tunnel
> backer in the datapath.  Tunnel headers are set by userspace using
> the ipv4_tunnel datapath action.  There are still some significant
> pieces of work to do, but the basic building blocks are there to
> begin testing.
> 
> Signed-off-by: Jesse Gross <jesse at nicira.com>
> Signed-off-by: Ethan Jackson <ethan at nicira.com>

The commit message could use more information on the high-level changes
in the patch.  Some of it took me a while to figure out.  Here's my
suggestion for text to add:

    The configuration of individual tunnels is now a userspace
    responsibility, so netdev-vport no longer marshals and unmarshals
    Netlink attributes for tunnel configuration, instead only storing
    the configuration internally.


> +    - Tunneling requires the version of the kernel module paired Open vSwitch
> +      1.9.0 or later.

There's something missing from that sentence.  Perhaps add "with"
following "paired"?

Looking at the set_tunnel_config() change, the reason you didn't bother
breaking it up into parse-then-assemble-Netlink in an earlier patch is
obvious.  Thanks.

port_construct() makes a call to VLOG_ERR passing a new-line terminated
string.  Our usual style is to omit the new-line, although it makes no
actual difference.  (You didn't originate this but I noticed it anyway.)

I found may_dpif_port_del() a little hard to read because of the two
different types, ofport and ofport_dpif, used unnecessarily in this
case.  I think it's easier if you always use ofport_dpif, as in this
incremental:

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 87f3332..36bb37a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2963,24 +2963,25 @@ static bool
 may_dpif_port_del(struct ofproto_dpif *ofproto, struct ofport_dpif *ofport)
 {
     struct ofproto_dpif *ofproto_iter;
-    struct ofport *ofport_up;
 
     if (!netdev_get_tunnel_config(ofport->up.netdev)) {
         return true;
     }
 
     HMAP_FOR_EACH (ofproto_iter, all_ofproto_dpifs_node, &all_ofproto_dpifs) {
+        struct ofport_dpif *iter;
+
         if (ofproto->backer != ofproto_iter->backer) {
             continue;
         }
 
-        HMAP_FOR_EACH (ofport_up, hmap_node, &ofproto_iter->up.ports) {
-            if (&ofport->up == ofport_up) {
+        HMAP_FOR_EACH (iter, up.hmap_node, &ofproto_iter->up.ports) {
+            if (ofport == iter) {
                 continue;
             }
 
             if (!strcmp(netdev_get_dpif_port(ofport->up.netdev),
-                        netdev_get_dpif_port(ofport_up->netdev))) {
+                        netdev_get_dpif_port(iter->up.netdev))) {
                 return false;
             }
         }

Also may_dpif_port_del() could use a comment explaining what it's up to.

Did you consider reference-counting the backer ports instead of doing a
search per-port-destruction?

The change to ofproto_receive() dropped the comment
        /* Let the caller know that we can't reproduce 'key' from 'flow'. */
        if (fitness == ODP_FIT_PERFECT) {
            fitness = ODP_FIT_TOO_MUCH;
        }
but in fact it answered a question I asked myself when I reviewed the
new code.

Thanks,

Ben.