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

Ben Pfaff blp at nicira.com
Mon Jan 7 19:06:14 UTC 2013


On Sat, Jan 05, 2013 at 11:25:57AM -0800, Justin Pettit wrote:
> +    - The Linux datapath implementation creates a different kernel-
> +      userspace channel for each port instead of sharing a static 16
> +      channels to provide better performance isolation.

I think that's going to prompt more questions than it's going to
answer.  How about:

    - With the Linux datapath, packets for new flows are now queued
      separately on a per-port basis, so it should no longer be
      possible for a large number of new flows arriving on one port to
      prevent new flows from being processed on other ports.

It's still very technical, but I think it is likely to help more
readers.

> +    /* We assume that the datapath densely chooses port numbers, which
> +     * can therefore be used as an index into an array of channels. */

We can assume that that is the common case, but it looks like your
code goes farther than that and actually dereferences beyond array
bounds if the assumption is bad.  If port_no is 10 but there are only
5 ports, it looks very much to me that we allocate 5 elements and then
blindly access index 10.

I'd suggest one of two approaches:

    - Allocate the maximum array index based on the largest port
      number yet observed, instead of on the number of ports.

    - Use a hash table.

The latter is better if you consider somebody using ovs-dpctl to add a
port numbered 1000000 or, worse, UINT32_MAX - 1.

I am not sure that the indexing logic in dpif_linux_recv() is actually
correct.  A more conventional approach, in my opinion, would be:

    if (dpif->event_offset >= dpif->n_events) {
        int retval;

        dpif->event_offset = dpif->n_events = 0;
...
        } else if (retval > 0) {
            dpif->n_events = retval;
        }
    }

    while (dpif->event_offset < dpif->n_events) {
        int idx = dpif->epoll_events[dpif->event_offset].data.u32;
        struct dpif_channel *ch = &dpif->channels[idx];

        dpif->event_offset++;
...
    }

Thanks,

Ben.



More information about the dev mailing list