[ovs-dev] [PATCH 3/3] dpif-linux: Give each port its own userspace-kernel channel.
jpettit at nicira.com
Tue Jan 8 01:26:39 UTC 2013
On Jan 7, 2013, at 11:06 AM, Ben Pfaff <blp at nicira.com> wrote:
> 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.
This is what the code is supposed to do--let me know if I screwed that up somehow. I probably chose a poor name, but "n_ports" is defined as "port_no + 1". I've renamed it "array_size", since it's used to size both the channel and event arrays.
> - 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.
A hash table is certainly another way to do it, but the data structures we have right now are pretty small, and a hash table has its own overhead. I think doing that with ovs-dpctl is kind of a corner-case, since the new single datapath isn't going to know what to do with a port that is not added through the database, and most users don't manually assign the datapath port number anyway. Let me know if you still think we should do that, though.
> I am not sure that the indexing logic in dpif_linux_recv() is actually
> correct. A more conventional approach, in my opinion, would be:
I'm happy to use that logic instead, since it is a bit easier to read.
I did a bit more testing and realized that ovs-dpctl port modifications didn't work properly, since it doesn't call dpif_linux_recv_set(). I'll send out a revised version of the patch with these changes.
More information about the dev