datapath: Make the datapath responsible for choosing port numbers.

Soon we will allow for multiple datapath implementations.  By allowing
the datapath to choose the port numbers, we possibly simplify some datapath
implementations, and the datapath's clients don't have to guess (or to
check) what port numbers are free, so this seems like a better way to go.

diff --git a/datapath/datapath.c b/datapath/datapath.c
index ceaed153706597767f26afc0c924264e6727e86c..27bc5ce47e429183056695c11624ebf06b6a3f94 100644 (file)
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -370,11 +370,6 @@ static int add_port(int dp_idx, struct odp_port __user *portp)
        if (copy_from_user(&port, portp, sizeof port))
                goto out;
        port.devname[IFNAMSIZ - 1] = '\0';
-       port_no = port.port;
-
-       err = -EINVAL;
-       if (port_no < 0 || port_no >= DP_MAX_PORTS)
-               goto out;
 
        rtnl_lock();
        dp = get_dp_locked(dp_idx);
@@ -382,10 +377,13 @@ static int add_port(int dp_idx, struct odp_port __user *portp)
        if (!dp)
                goto out_unlock_rtnl;
 
-       err = -EEXIST;
-       if (dp->ports[port_no])
-               goto out_unlock_dp;
+       for (port_no = 1; port_no < DP_MAX_PORTS; port_no++)
+               if (!dp->ports[port_no])
+                       goto got_port_no;
+       err = -EXFULL;
+       goto out_unlock_dp;
 
+got_port_no:
        if (!(port.flags & ODP_PORT_INTERNAL)) {
                err = -ENODEV;
                dev = dev_get_by_name(&init_net, port.devname);
@@ -411,6 +409,8 @@ static int add_port(int dp_idx, struct odp_port __user *portp)
        if (dp_add_if_hook)
                dp_add_if_hook(dp->ports[port_no]);
 
+       err = __put_user(port_no, &port.port);
+
 out_put:
        dev_put(dev);
 out_unlock_dp:

diff --git a/lib/dpif.c b/lib/dpif.c
index 09a47d2fcc42893f8a09144127e7dd2b359ce8c9..5558631ec14509a2a1cb7b8cb410944e0559921b 100644 (file)
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -243,25 +243,33 @@ dpif_recv_purge(struct dpif *dpif)
 }
 
 int
-dpif_port_add(struct dpif *dpif, const char *devname, uint16_t port_no,
-              uint16_t flags)
+dpif_port_add(struct dpif *dpif, const char *devname, uint16_t flags,
+              uint16_t *port_nop)
 {
     struct odp_port port;
+    uint16_t port_no;
+    int error;
 
     COVERAGE_INC(dpif_port_add);
+
     memset(&port, 0, sizeof port);
     strncpy(port.devname, devname, sizeof port.devname);
-    port.port = port_no;
     port.flags = flags;
-    if (!ioctl(dpif->fd, ODP_PORT_ADD, &port)) {
+
+    error = do_ioctl(dpif, ODP_PORT_ADD, NULL, &port);
+    if (!error) {
+        port_no = port.port;
         VLOG_DBG_RL(&dpmsg_rl, "%s: added %s as port %"PRIu16,
                     dpif_name(dpif), devname, port_no);
-        return 0;
     } else {
-        VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port %"PRIu16": %s",
-                     dpif_name(dpif), devname, port_no, strerror(errno));
-        return errno;
+        port_no = UINT16_MAX;
+        VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s",
+                     dpif_name(dpif), devname, strerror(errno));
     }
+    if (port_nop) {
+        *port_nop = port_no;
+    }
+    return error;
 }
 
 int

diff --git a/lib/dpif.h b/lib/dpif.h
index c4619c9be9b036633a17f1417fe883be73a4cda0..88bd15dcf1cd0c56aa3b1f2a6b5630fdef307649 100644 (file)
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -42,8 +42,8 @@ int dpif_get_dp_stats(const struct dpif *, struct odp_stats *);
 int dpif_get_drop_frags(const struct dpif *, bool *drop_frags);
 int dpif_set_drop_frags(struct dpif *, bool drop_frags);
 
-int dpif_port_add(struct dpif *, const char *devname, uint16_t port_no,
-                  uint16_t flags);
+int dpif_port_add(struct dpif *, const char *devname, uint16_t flags,
+                  uint16_t *port_no);
 int dpif_port_del(struct dpif *, uint16_t port_no);
 int dpif_port_query_by_number(const struct dpif *, uint16_t port_no,
                               struct odp_port *);

diff --git a/utilities/ovs-dpctl.8.in b/utilities/ovs-dpctl.8.in
index c43d4ffa013af746e081d2aed37a3c26d246b631..0a1d670298b4e68cd5fa51cb78fb43ccb537fe77 100644 (file)
--- a/utilities/ovs-dpctl.8.in
+++ b/utilities/ovs-dpctl.8.in
@@ -70,11 +70,6 @@ A \fInetdev\fR may be followed by a comma-separated list of options.
 The following options are currently supported:
 
 .RS
-.IP "\fBport=\fIportno\fR"
-Specifies \fIportno\fR (a number between 1 and 255) as the port number
-at which \fInetdev\fR will be attached.  By default, \fBadd\-if\fR
-automatically selects the lowest available port number.
-
 .IP "\fBinternal\fR"
 Instead of attaching an existing \fInetdev\fR, creates an internal
 port (analogous to the local port) with that name.

diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index 9f45b3add47b35274556f5c1c3e1341fd97d2882..270281832d94362ba81e8d7de4dcb5ef671db164 100644 (file)
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -243,29 +243,6 @@ query_ports(struct dpif *dpif, struct odp_port **ports, size_t *n_ports)
     qsort(*ports, *n_ports, sizeof **ports, compare_ports);
 }
 
-static uint16_t
-get_free_port(struct dpif *dpif)
-{
-    struct odp_port *ports;
-    size_t n_ports;
-    int port_no;
-
-    query_ports(dpif, &ports, &n_ports);
-    for (port_no = 0; port_no <= UINT16_MAX; port_no++) {
-        size_t i;
-        for (i = 0; i < n_ports; i++) {
-            if (ports[i].port == port_no) {
-                goto next_portno;
-            }
-        }
-        free(ports);
-        return port_no;
-
-    next_portno: ;
-    }
-    ovs_fatal(0, "no free datapath ports");
-}
-
 static void
 do_add_if(int argc UNUSED, char *argv[])
 {
@@ -277,7 +254,6 @@ do_add_if(int argc UNUSED, char *argv[])
     for (i = 2; i < argc; i++) {
         char *save_ptr = NULL;
         char *devname, *suboptions;
-        int port = -1;
         int flags = 0;
         int error;
 
@@ -290,11 +266,9 @@ do_add_if(int argc UNUSED, char *argv[])
         suboptions = strtok_r(NULL, "", &save_ptr);
         if (suboptions) {
             enum {
-                AP_PORT,
                 AP_INTERNAL
             };
             static char *options[] = {
-                "port",
                 "internal"
             };
 
@@ -302,13 +276,6 @@ do_add_if(int argc UNUSED, char *argv[])
                 char *value;
 
                 switch (getsubopt(&suboptions, options, &value)) {
-                case AP_PORT:
-                    if (!value) {
-                        ovs_error(0, "'port' suboption requires a value");
-                    }
-                    port = atoi(value);
-                    break;
-
                 case AP_INTERNAL:
                     flags |= ODP_PORT_INTERNAL;
                     break;
@@ -319,14 +286,10 @@ do_add_if(int argc UNUSED, char *argv[])
                 }
             }
         }
-        if (port < 0) {
-            port = get_free_port(dpif);
-        }
 
-        error = dpif_port_add(dpif, devname, port, flags);
+        error = dpif_port_add(dpif, devname, flags, NULL);
         if (error) {
-            ovs_error(error, "adding %s as port %"PRIu16" of %s failed",
-                      devname, port, argv[1]);
+            ovs_error(error, "adding %s to %s failed", devname, argv[1]);
             failure = true;
         } else if (if_up(devname)) {
             failure = true;

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index da80eed7e51cbc15c559673c4ded18bedba62c07..f0aa9c03c595e645fb5586a309705c9f3c960c1d 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -438,7 +438,6 @@ bridge_reconfigure(void)
         struct odp_port *dpif_ports;
         size_t n_dpif_ports;
         struct svec cur_ifaces, want_ifaces, add_ifaces;
-        int next_port_no;
 
         dpif_port_list(br->dpif, &dpif_ports, &n_dpif_ports);
         svec_init(&cur_ifaces);
@@ -450,29 +449,20 @@ bridge_reconfigure(void)
         bridge_get_all_ifaces(br, &want_ifaces);
         svec_diff(&want_ifaces, &cur_ifaces, &add_ifaces, NULL, NULL);
 
-        next_port_no = 1;
         for (i = 0; i < add_ifaces.n; i++) {
             const char *if_name = add_ifaces.names[i];
-            for (;;) {
-                int internal = cfg_get_bool(0, "iface.%s.internal", if_name);
-                int error = dpif_port_add(br->dpif, if_name, next_port_no++,
-                                          internal ? ODP_PORT_INTERNAL : 0);
-                if (error != EEXIST) {
-                    if (next_port_no >= 256) {
-                        VLOG_ERR("ran out of valid port numbers on %s",
-                                 dpif_name(br->dpif));
-                        goto out;
-                    }
-                    if (error) {
-                        VLOG_ERR("failed to add %s interface to %s: %s",
-                                 if_name, dpif_name(br->dpif),
-                                 strerror(error));
-                    }
-                    break;
-                }
+            int internal = cfg_get_bool(0, "iface.%s.internal", if_name);
+            int flags = internal ? ODP_PORT_INTERNAL : 0;
+            int error = dpif_port_add(br->dpif, if_name, flags, NULL);
+            if (error == EXFULL) {
+                VLOG_ERR("ran out of valid port numbers on %s",
+                         dpif_name(br->dpif));
+                break;
+            } else if (error) {
+                VLOG_ERR("failed to add %s interface to %s: %s",
+                         if_name, dpif_name(br->dpif), strerror(error));
             }
         }
-    out:
         svec_destroy(&cur_ifaces);
         svec_destroy(&want_ifaces);
         svec_destroy(&add_ifaces);