[ovs-dev] [PATCHv2] dpif-linux: Give each port its own userspace-kernel channel.

Justin Pettit jpettit at nicira.com
Thu Jan 10 21:59:06 UTC 2013


On Jan 10, 2013, at 11:24 AM, Ben Pfaff <blp at nicira.com> wrote:

> On Thu, Jan 10, 2013 at 09:30:31AM -0800, Justin Pettit wrote:
>> -    for (ch = dpif->channels; ch < &dpif->channels[dpif->n_channels]; ch++) {
>> +    for (ch = dpif->channels; ch < &dpif->channels[dpif->uc_array_size]; ch++) 
> 
> A { got lost here somehow.

It was an email issue.

> Also, I noticed that in the commit message you refer to a commit by
> only 6 digits.  I'd use at least 8-10, 6 is likely to be ambiguous, if
> not now then sometime later.

Done.

> It looks like the dpif_name() call here is indented 4 spaces too few:
> +        if (new_size > 65535) {
> +            VLOG_WARN_RL(&error_rl, "%s: datapath port %"PRIu32" too big",
> +                         dpif_name(&dpif->dpif), port_no);
> +            return EFBIG;
> +        }

Done.

> In add_channel(), when we increase the array size, we do not
> initialize dpif->channels[port_no].sock or
> dpif->channels[port_no].last_poll.  Ordinarily it will be set later on
> after we add the epoll_fd, but if that fails then I believe it will be
> left indeterminate.  I think that we could fix that by changing
>        for (i = dpif->uc_array_size; i < port_no; i++) {
> to
>        for (i = dpif->uc_array_size; i < new_size; i++) {

Fixed.  Thanks.

> In dpif_linux_port_add(), if add_channel() fails, do you think we
> should try to delete the port?

Make sense.  I added that.

> In dpif_linux_recv_set(), we give up and return an error if setting
> the uplink pid for any port fails.  I agree that returning an error is
> the right thing to do, but I think we should consider still continuing
> to add channels for the remaining ports.  That way, we will be more
> resilient against ports getting deleted while recv_set() runs.  (One
> way that a port could get deleted is for a VM to terminate and the
> netdevs for its VIFs to disappear from the datapath.  It is hard to
> avoid that race.  Another way, of course, is "ovs-dpctl del-port".).


Returning an error is considered a fatal error by the callers.  I aded some logic so that we only return an error if setting the PID didn't return ENODEV or ENOENT, which should cover the sorts of issues you mentioned.

Can you take a look at the incremental and let me know what you think?

--Justin


-=-=-=-=-=-=-=-=-=-=-=-

--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -322,7 +322,7 @@ add_channel(struct dpif_linux *dpif, uint32_t port_no, struc
 
         dpif->channels = xrealloc(dpif->channels,
                                   new_size * sizeof *dpif->channels);
-        for (i = dpif->uc_array_size; i < port_no; i++) {
+        for (i = dpif->uc_array_size; i < new_size; i++) {
             dpif->channels[i].sock = NULL;
         }
 
@@ -489,6 +489,14 @@ dpif_linux_port_add(struct dpif *dpif_, struct netdev *netd
         if (error) {
             VLOG_INFO("%s: could not add channel for port %s",
                       dpif_name(dpif_), name);
+
+            /* Delete the port. */
+            dpif_linux_vport_init(&request);
+            request.cmd = OVS_VPORT_CMD_DEL;
+            request.dp_ifindex = dpif->dp_ifindex;
+            request.port_no = *port_nop;
+            dpif_linux_vport_transact(&request, NULL, NULL);
+
             nl_sock_destroy(sock);
             return error;
         }
@@ -1122,7 +1130,13 @@ dpif_linux_recv_set(struct dpif *dpif_, bool enable)
                              "%s: failed to set upcall pid on port: %s",
                              dpif_name(&dpif->dpif), strerror(error));
                 nl_sock_destroy(sock);
-                return error;
+
+                if (error == ENODEV || error == ENOENT) {
+                    /* This device isn't there, but keep trying the others. */
+                    continue;
+                } else {
+                    return error;
+                }
             }
 
             error = add_channel(dpif, port.port_no, sock);






More information about the dev mailing list