The incremental looks good to me. I'll go ahead and merge it with it
applied (after I test it). Assuming you think the patch is ok
otherwise.
Ethan
On Tue, Apr 5, 2011 at 8:29 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Tue, Apr 05, 2011 at 06:09:04PM -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.
>
> I think you should add a check for port > ODPP_LOCAL && port <
> LRU_MAX_PORTS in dpif_linux_push_port().
>
> Alternatively, here's an incremental that adds a bitmap:
>
> diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
> index 464ac88..10ddd51 100644
> --- a/lib/dpif-linux.c
> +++ b/lib/dpif-linux.c
> @@ -32,6 +32,7 @@
> #include <sys/stat.h>
> #include <unistd.h>
>
> +#include "bitmap.h"
> #include "dpif-provider.h"
> #include "netdev.h"
> #include "netdev-vport.h"
> @@ -134,6 +135,7 @@ struct dpif_linux {
> bool change_error;
>
> /* Queue of unused ports. */
> + unsigned long *lru_bitmap;
> uint16_t lru_ports[LRU_MAX_PORTS];
> size_t lru_head;
> size_t lru_tail;
> @@ -170,31 +172,24 @@ dpif_linux_cast(const struct dpif *dpif)
> static void
> dpif_linux_push_port(struct dpif_linux *dp, uint16_t port)
> {
> - size_t i;
> -
> - if (dp->lru_head - dp->lru_tail >= LRU_MAX_PORTS) {
> - return;
> - }
> -
> - /* XXX: Replace this loop with a bitmap indicating which ports are in the
> - * queue. */
> - for (i = dp->lru_tail; i != dp->lru_head; i++) {
> - if (dp->lru_ports[i & LRU_MASK] == port) {
> - return;
> - }
> + if (port < LRU_MAX_PORTS && !bitmap_is_set(dp->lru_bitmap, port)) {
> + bitmap_set1(dp->lru_bitmap, port);
> + dp->lru_ports[dp->lru_head++ & LRU_MASK] = port;
> }
> -
> - dp->lru_ports[dp->lru_head++ & LRU_MASK] = port;
> }
>
> static uint32_t
> dpif_linux_pop_port(struct dpif_linux *dp)
> {
> + uint16_t port;
> +
> if (dp->lru_head == dp->lru_tail) {
> return UINT32_MAX;
> }
>
> - return dp->lru_ports[dp->lru_tail++ & LRU_MASK];
> + port = dp->lru_ports[dp->lru_tail++ & LRU_MASK];
> + bitmap_set0(dp->lru_bitmap, port);
> + return port;
> }
>
> static int
> @@ -275,6 +270,8 @@ open_dpif(const struct dpif_linux_dp *dp, struct dpif
> **dpifp)
> *dpifp = &dpif->dpif;
>
> dpif->lru_head = dpif->lru_tail = 0;
> + dpif->lru_bitmap = bitmap_allocate(LRU_MAX_PORTS);
> + bitmap_set1(dpif->lru_bitmap, ODPP_LOCAL);
> for (i = 1; i < LRU_MAX_PORTS; i++) {
> dpif_linux_push_port(dpif, i);
> }
> @@ -291,6 +288,7 @@ dpif_linux_close(struct dpif *dpif_)
> struct dpif_linux *dpif = dpif_linux_cast(dpif_);
> rtnetlink_link_notifier_unregister(&dpif->port_notifier);
> sset_destroy(&dpif->changed_ports);
> + free(dpif->lru_bitmap);
> free(dpif);
> }
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
