On Mon, Apr 04, 2011 at 06:17:19PM -0700, Ethan Jackson wrote:
> Before this patch the kernel chose the lowest available number for
> newly created datapath ports. This patch moves the port number
> choosing responsibility to user space, and implements a least
> recently used port number queue in an attempt to avoid reuse.
>
> Bug #2140.
Thanks.
Could we s/DP_MAX_PORTS/LRU_MAX_PORTS/? I want to avoid the
implication that this is known to be the maximum number of ports that
the datapath actually supports. It could be smaller or larger if the
kernel module has been modified.
In dpif_linux_push_port():
I think that we can avoid the loop with a 1024-bit bitmap. That's
only 128 bytes, which is small change next to the 2048-byte
lru_ports array, so I think that we might as well do it. We
already have bitmap helpers in bitmap.h, so it should be really
easy to do.
If you decide not to use a bitmap then the condition on the loop
needs to be != instead of <. Otherwise it breaks if the head
wraps around.
If the kernel has been modified to support more than 1024 ports
then the assertion in dpif_linux_push_port() could fail. I'd
suggest making that function a no-op when 'port' is zero or >=
LRU_PORTS.
If dpif_linux_pop_port() returned the port number on success or
UINT32_MAX on failure then its return value could be directly stored
into request.port_no.
In dpif_linux_port_add():
I believe that this function leaks memory if it has to try more
than once. It needs to ofpbuf_delete(buf).
If there are no ports in the LRU and the port add fails with EBUSY,
then dpif_linux_port_add() is likely to loop forever.
I'd suggest trying again also if the error is EFBIG. That means
that the port number we requested is too big. (Maybe someone
reduced the number of supported datapath ports to save memory.)
In dpif_linux_port_del(), I would push the port only if the deletion
returns 0 (possibly also if it returns ENOENT to indicate that there's
no such port).
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
