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

[Ben Pfaff]

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.

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.

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;
+        }

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++) {

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

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".).

Thanks,

Ben.