[Qemu-devel] [PATCH] two level table for IO port lookup

[Qemu-devel] [PATCH] two level table for IO port lookup

[Brian Wheeler]

The alpha architecture uses 24 bits for the io port address so this
patch adds a two level table and puts the IO port data into a
struct...because sizeof(void *) * 7 * 16777216 is nearly a 1G on my
workstation.

I've set the alpha target to use a 12/12 split and everything else to
use 8/8.  




Signed-off-by: Brian Wheeler <bdwheele@indiana.edu>

--- vl.c.orig	2009-04-10 10:01:52.000000000 -0400
+++ vl.c	2009-04-10 10:13:33.000000000 -0400
@@ -168,7 +168,7 @@
 //#define DEBUG_IOPORT
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
-
+//#define DEBUG_IOPORT_FIND
 
 #ifdef DEBUG_IOPORT
 #  define LOG_IOPORT(...) qemu_log_mask(CPU_LOG_IOPORT, ## __VA_ARGS__)
@@ -184,14 +184,30 @@
 /* Max number of bluetooth switches on the commandline.  */
 #define MAX_BT_CMDLINE 10
 
-/* XXX: use a two level table to limit memory usage */
-#define MAX_IOPORTS 65536
-
 const char *bios_dir = CONFIG_QEMU_SHAREDIR;
 const char *bios_name = NULL;
-static void *ioport_opaque[MAX_IOPORTS];
-static IOPortReadFunc *ioport_read_table[3][MAX_IOPORTS];
-static IOPortWriteFunc *ioport_write_table[3][MAX_IOPORTS];
+
+struct ioport {
+	void *opaque;
+	IOPortReadFunc *read[3];
+	IOPortWriteFunc *write[3];
+};
+typedef struct ioport ioport_t;
+
+#ifdef TARGET_ALPHA
+#define IOPORT_MAXBITS 24
+#define IOPORT_PAGESIZE 12
+#else
+#define IOPORT_MAXBITS 16
+#define IOPORT_PAGESIZE 8
+#endif
+
+#define IOPORT_ENTRYMASK ((1<<IOPORT_PAGESIZE)-1)
+#define IOPORT_PAGEMASK ~IOPORT_ENTRYMASK
+#define MAX_IOPORTS (1<<IOPORT_MAXBITS)
+
+void *ioport[1<<(IOPORT_MAXBITS-IOPORT_PAGESIZE)];
+
 /* Note: drives_table[MAX_DRIVES] is a dummy block driver if none available
    to store the VM snapshots */
 DriveInfo drives_table[MAX_DRIVES+1];
@@ -288,6 +304,33 @@
 static IOPortReadFunc default_ioport_readb, default_ioport_readw, default_ioport_readl;
 static IOPortWriteFunc default_ioport_writeb, default_ioport_writew, default_ioport_writel;
 
+
+static inline ioport_t *ioport_find(uint32_t address) 
+{
+  uint32_t page = (address & IOPORT_PAGEMASK) >> IOPORT_PAGESIZE;
+  uint32_t entry = address & IOPORT_ENTRYMASK;
+  if(address >= (1<<IOPORT_MAXBITS))
+    hw_error("Maximum port # for this architecture is %d.  Port %d requested.",
+	     (1<<IOPORT_MAXBITS)-1, address);
+  
+  if(ioport[page]==NULL) {
+    ioport[page]=calloc((1<<IOPORT_PAGESIZE), sizeof(ioport_t));
+#ifdef DEBUG_IOPORT_FIND
+    printf("Initializing ioport page %d to: %p\n", page, ioport[page]);
+#endif
+  }
+  ioport_t *p = (ioport_t *)(ioport[page] + entry * sizeof(ioport_t));
+
+#ifdef DEBUG_IOPORT_FIND
+  printf("port find %d:  page=%d, address=%p, entry=%d, address=%p\n", 
+	 address, page, ioport[page], entry, p);
+  printf("  data: %p\n", p->opaque);
+  printf("  read: %p, %p, %p\n", p->read[0], p->read[1], p->read[2]);
+  printf(" write: %p, %p, %p\n", p->write[0], p->write[1], p->write[2]);
+#endif
+  return p;
+}
+
 static uint32_t ioport_read(int index, uint32_t address)
 {
     static IOPortReadFunc *default_func[3] = {
@@ -295,10 +338,11 @@
         default_ioport_readw,
         default_ioport_readl
     };
-    IOPortReadFunc *func = ioport_read_table[index][address];
+    ioport_t *p = ioport_find(address);
+    IOPortReadFunc *func = p->read[index];
     if (!func)
         func = default_func[index];
-    return func(ioport_opaque[address], address);
+    return func(p->opaque, address);
 }
 
 static void ioport_write(int index, uint32_t address, uint32_t data)
@@ -308,10 +352,11 @@
         default_ioport_writew,
         default_ioport_writel
     };
-    IOPortWriteFunc *func = ioport_write_table[index][address];
+    ioport_t *p = ioport_find(address);
+    IOPortWriteFunc *func = p->write[index];
     if (!func)
         func = default_func[index];
-    func(ioport_opaque[address], address, data);
+    func(p->opaque, address, data);
 }
 
 static uint32_t default_ioport_readb(void *opaque, uint32_t address)
@@ -378,10 +423,11 @@
         return -1;
     }
     for(i = start; i < start + length; i += size) {
-        ioport_read_table[bsize][i] = func;
-        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
+        ioport_t *p = ioport_find(i);
+        p->read[bsize] = func;
+        if (p->opaque != NULL && p->opaque != opaque)
             hw_error("register_ioport_read: invalid opaque");
-        ioport_opaque[i] = opaque;
+        p->opaque = opaque;
     }
     return 0;
 }
@@ -403,10 +449,11 @@
         return -1;
     }
     for(i = start; i < start + length; i += size) {
-        ioport_write_table[bsize][i] = func;
-        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
+        ioport_t *p = ioport_find(i);
+        p->write[bsize] = func;
+        if (p->opaque != NULL && p->opaque != opaque)
             hw_error("register_ioport_write: invalid opaque");
-        ioport_opaque[i] = opaque;
+        p->opaque = opaque;
     }
     return 0;
 }
@@ -416,15 +463,16 @@
     int i;
 
     for(i = start; i < start + length; i++) {
-        ioport_read_table[0][i] = default_ioport_readb;
-        ioport_read_table[1][i] = default_ioport_readw;
-        ioport_read_table[2][i] = default_ioport_readl;
-
-        ioport_write_table[0][i] = default_ioport_writeb;
-        ioport_write_table[1][i] = default_ioport_writew;
-        ioport_write_table[2][i] = default_ioport_writel;
+        ioport_t *p = ioport_find(i);
+        p->read[0] = default_ioport_readb;
+        p->read[1] = default_ioport_readw;
+        p->read[2] = default_ioport_readl;
+
+        p->write[0] = default_ioport_writeb;
+        p->write[1] = default_ioport_writew;
+        p->write[2] = default_ioport_writel;
 
-        ioport_opaque[i] = NULL;
+        p->opaque = NULL;
     }
 }
 

[Qemu-devel] Re: [PATCH] two level table for IO port lookup

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 6675 bytes --]

Brian Wheeler wrote:
> The alpha architecture uses 24 bits for the io port address so this
> patch adds a two level table and puts the IO port data into a
> struct...because sizeof(void *) * 7 * 16777216 is nearly a 1G on my
> workstation.
> 
> I've set the alpha target to use a 12/12 split and everything else to
> use 8/8.  
> 
> 
> 
> 
> Signed-off-by: Brian Wheeler <bdwheele@indiana.edu>
> 
> --- vl.c.orig	2009-04-10 10:01:52.000000000 -0400
> +++ vl.c	2009-04-10 10:13:33.000000000 -0400
> @@ -168,7 +168,7 @@
>  //#define DEBUG_IOPORT
>  //#define DEBUG_NET
>  //#define DEBUG_SLIRP
> -
> +//#define DEBUG_IOPORT_FIND
>  
>  #ifdef DEBUG_IOPORT
>  #  define LOG_IOPORT(...) qemu_log_mask(CPU_LOG_IOPORT, ## __VA_ARGS__)
> @@ -184,14 +184,30 @@
>  /* Max number of bluetooth switches on the commandline.  */
>  #define MAX_BT_CMDLINE 10
>  
> -/* XXX: use a two level table to limit memory usage */
> -#define MAX_IOPORTS 65536
> -
>  const char *bios_dir = CONFIG_QEMU_SHAREDIR;
>  const char *bios_name = NULL;
> -static void *ioport_opaque[MAX_IOPORTS];
> -static IOPortReadFunc *ioport_read_table[3][MAX_IOPORTS];
> -static IOPortWriteFunc *ioport_write_table[3][MAX_IOPORTS];
> +
> +struct ioport {
> +	void *opaque;
> +	IOPortReadFunc *read[3];
> +	IOPortWriteFunc *write[3];
> +};
> +typedef struct ioport ioport_t;
> +
> +#ifdef TARGET_ALPHA
> +#define IOPORT_MAXBITS 24
> +#define IOPORT_PAGESIZE 12
> +#else
> +#define IOPORT_MAXBITS 16
> +#define IOPORT_PAGESIZE 8
> +#endif
> +
> +#define IOPORT_ENTRYMASK ((1<<IOPORT_PAGESIZE)-1)
> +#define IOPORT_PAGEMASK ~IOPORT_ENTRYMASK
> +#define MAX_IOPORTS (1<<IOPORT_MAXBITS)
> +
> +void *ioport[1<<(IOPORT_MAXBITS-IOPORT_PAGESIZE)];
> +
>  /* Note: drives_table[MAX_DRIVES] is a dummy block driver if none available
>     to store the VM snapshots */
>  DriveInfo drives_table[MAX_DRIVES+1];
> @@ -288,6 +304,33 @@
>  static IOPortReadFunc default_ioport_readb, default_ioport_readw, default_ioport_readl;
>  static IOPortWriteFunc default_ioport_writeb, default_ioport_writew, default_ioport_writel;
>  
> +
> +static inline ioport_t *ioport_find(uint32_t address) 
> +{
> +  uint32_t page = (address & IOPORT_PAGEMASK) >> IOPORT_PAGESIZE;
> +  uint32_t entry = address & IOPORT_ENTRYMASK;
> +  if(address >= (1<<IOPORT_MAXBITS))
> +    hw_error("Maximum port # for this architecture is %d.  Port %d requested.",
> +	     (1<<IOPORT_MAXBITS)-1, address);
> +  
> +  if(ioport[page]==NULL) {
> +    ioport[page]=calloc((1<<IOPORT_PAGESIZE), sizeof(ioport_t));

As you use ioport_find also for guest-driven port access, doing
allocation here is a bad idea. Consider a guest that probes the whole io
address space of your alpha box: you would end up with the same gig
being allocated that you try to avoid with this approach.

IOW: Only allocate on handler registration. On unsuccessful lookup, just
call the proper default handler.

> +#ifdef DEBUG_IOPORT_FIND
> +    printf("Initializing ioport page %d to: %p\n", page, ioport[page]);
> +#endif
> +  }
> +  ioport_t *p = (ioport_t *)(ioport[page] + entry * sizeof(ioport_t));
> +
> +#ifdef DEBUG_IOPORT_FIND
> +  printf("port find %d:  page=%d, address=%p, entry=%d, address=%p\n", 
> +	 address, page, ioport[page], entry, p);
> +  printf("  data: %p\n", p->opaque);
> +  printf("  read: %p, %p, %p\n", p->read[0], p->read[1], p->read[2]);
> +  printf(" write: %p, %p, %p\n", p->write[0], p->write[1], p->write[2]);
> +#endif
> +  return p;
> +}
> +
>  static uint32_t ioport_read(int index, uint32_t address)
>  {
>      static IOPortReadFunc *default_func[3] = {
> @@ -295,10 +338,11 @@
>          default_ioport_readw,
>          default_ioport_readl
>      };
> -    IOPortReadFunc *func = ioport_read_table[index][address];
> +    ioport_t *p = ioport_find(address);
> +    IOPortReadFunc *func = p->read[index];
>      if (!func)
>          func = default_func[index];
> -    return func(ioport_opaque[address], address);
> +    return func(p->opaque, address);
>  }
>  
>  static void ioport_write(int index, uint32_t address, uint32_t data)
> @@ -308,10 +352,11 @@
>          default_ioport_writew,
>          default_ioport_writel
>      };
> -    IOPortWriteFunc *func = ioport_write_table[index][address];
> +    ioport_t *p = ioport_find(address);
> +    IOPortWriteFunc *func = p->write[index];
>      if (!func)
>          func = default_func[index];
> -    func(ioport_opaque[address], address, data);
> +    func(p->opaque, address, data);
>  }
>  
>  static uint32_t default_ioport_readb(void *opaque, uint32_t address)
> @@ -378,10 +423,11 @@
>          return -1;
>      }
>      for(i = start; i < start + length; i += size) {
> -        ioport_read_table[bsize][i] = func;
> -        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
> +        ioport_t *p = ioport_find(i);
> +        p->read[bsize] = func;
> +        if (p->opaque != NULL && p->opaque != opaque)
>              hw_error("register_ioport_read: invalid opaque");
> -        ioport_opaque[i] = opaque;
> +        p->opaque = opaque;
>      }
>      return 0;
>  }
> @@ -403,10 +449,11 @@
>          return -1;
>      }
>      for(i = start; i < start + length; i += size) {
> -        ioport_write_table[bsize][i] = func;
> -        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
> +        ioport_t *p = ioport_find(i);
> +        p->write[bsize] = func;
> +        if (p->opaque != NULL && p->opaque != opaque)
>              hw_error("register_ioport_write: invalid opaque");
> -        ioport_opaque[i] = opaque;
> +        p->opaque = opaque;
>      }
>      return 0;
>  }
> @@ -416,15 +463,16 @@
>      int i;
>  
>      for(i = start; i < start + length; i++) {
> -        ioport_read_table[0][i] = default_ioport_readb;
> -        ioport_read_table[1][i] = default_ioport_readw;
> -        ioport_read_table[2][i] = default_ioport_readl;
> -
> -        ioport_write_table[0][i] = default_ioport_writeb;
> -        ioport_write_table[1][i] = default_ioport_writew;
> -        ioport_write_table[2][i] = default_ioport_writel;
> +        ioport_t *p = ioport_find(i);
> +        p->read[0] = default_ioport_readb;
> +        p->read[1] = default_ioport_readw;
> +        p->read[2] = default_ioport_readl;
> +
> +        p->write[0] = default_ioport_writeb;
> +        p->write[1] = default_ioport_writew;
> +        p->write[2] = default_ioport_writel;
>  
> -        ioport_opaque[i] = NULL;
> +        p->opaque = NULL;
>      }
>  }
>  
> 

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: [PATCH] two level table for IO port lookup [Patch updated]

[Brian Wheeler]

On Fri, 2009-04-10 at 17:17 +0200, Jan Kiszka wrote:
> Brian Wheeler wrote:
> > The alpha architecture uses 24 bits for the io port address so this
> > patch adds a two level table and puts the IO port data into a
> > struct...because sizeof(void *) * 7 * 16777216 is nearly a 1G on my
> > workstation.
> > 
> > I've set the alpha target to use a 12/12 split and everything else to
> > use 8/8.  
> > 
> > 
> > 
> > 
> > Signed-off-by: Brian Wheeler <bdwheele@indiana.edu>
> > 

> > +  if(ioport[page]==NULL) {
> > +    ioport[page]=calloc((1<<IOPORT_PAGESIZE), sizeof(ioport_t));
> 
> As you use ioport_find also for guest-driven port access, doing
> allocation here is a bad idea. Consider a guest that probes the whole io
> address space of your alpha box: you would end up with the same gig
> being allocated that you try to avoid with this approach.
> 
> IOW: Only allocate on handler registration. On unsuccessful lookup, just
> call the proper default handler.
> 

Good point.  This patch does that:  there's an allocate flag which is
set only by the registration calls so only they will allocate memory.

Signed-off-by: Brian Wheeler <bdwheele@indiana.edu>


--- vl.c.orig	2009-04-10 10:01:52.000000000 -0400
+++ vl.c	2009-04-10 11:40:27.000000000 -0400
@@ -168,7 +168,7 @@
 //#define DEBUG_IOPORT
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
-
+//#define DEBUG_IOPORT_FIND
 
 #ifdef DEBUG_IOPORT
 #  define LOG_IOPORT(...) qemu_log_mask(CPU_LOG_IOPORT, ## __VA_ARGS__)
@@ -184,14 +184,30 @@
 /* Max number of bluetooth switches on the commandline.  */
 #define MAX_BT_CMDLINE 10
 
-/* XXX: use a two level table to limit memory usage */
-#define MAX_IOPORTS 65536
-
 const char *bios_dir = CONFIG_QEMU_SHAREDIR;
 const char *bios_name = NULL;
-static void *ioport_opaque[MAX_IOPORTS];
-static IOPortReadFunc *ioport_read_table[3][MAX_IOPORTS];
-static IOPortWriteFunc *ioport_write_table[3][MAX_IOPORTS];
+
+struct ioport {
+	void *opaque;
+	IOPortReadFunc *read[3];
+	IOPortWriteFunc *write[3];
+};
+typedef struct ioport ioport_t;
+
+#ifdef TARGET_ALPHA
+#define IOPORT_MAXBITS 24
+#define IOPORT_PAGESIZE 12
+#else
+#define IOPORT_MAXBITS 16
+#define IOPORT_PAGESIZE 8
+#endif
+
+#define IOPORT_ENTRYMASK ((1<<IOPORT_PAGESIZE)-1)
+#define IOPORT_PAGEMASK ~IOPORT_ENTRYMASK
+#define MAX_IOPORTS (1<<IOPORT_MAXBITS)
+
+void *ioport[1<<(IOPORT_MAXBITS-IOPORT_PAGESIZE)];
+
 /* Note: drives_table[MAX_DRIVES] is a dummy block driver if none available
    to store the VM snapshots */
 DriveInfo drives_table[MAX_DRIVES+1];
@@ -288,6 +304,37 @@
 static IOPortReadFunc default_ioport_readb, default_ioport_readw, default_ioport_readl;
 static IOPortWriteFunc default_ioport_writeb, default_ioport_writew, default_ioport_writel;
 
+
+static inline ioport_t *ioport_find(uint32_t address, int allocate) 
+{
+  uint32_t page = (address & IOPORT_PAGEMASK) >> IOPORT_PAGESIZE;
+  uint32_t entry = address & IOPORT_ENTRYMASK;
+  if(address >= (1<<IOPORT_MAXBITS))
+    hw_error("Maximum port # for this architecture is %d.  Port %d requested.",
+	     (1<<IOPORT_MAXBITS)-1, address);
+  
+  if(ioport[page]==NULL) {
+    if(allocate) {
+      ioport[page]=calloc((1<<IOPORT_PAGESIZE), sizeof(ioport_t));
+#ifdef DEBUG_IOPORT_FIND
+    printf("Initializing ioport page %d to: %p\n", page, ioport[page]);
+#endif
+    } else {
+      return NULL;
+    }
+  }
+  ioport_t *p = (ioport_t *)(ioport[page] + entry * sizeof(ioport_t));
+
+#ifdef DEBUG_IOPORT_FIND
+  printf("port find %d:  page=%d, address=%p, entry=%d, address=%p\n", 
+	 address, page, ioport[page], entry, p);
+  printf("  data: %p\n", p->opaque);
+  printf("  read: %p, %p, %p\n", p->read[0], p->read[1], p->read[2]);
+  printf(" write: %p, %p, %p\n", p->write[0], p->write[1], p->write[2]);
+#endif
+  return p;
+}
+
 static uint32_t ioport_read(int index, uint32_t address)
 {
     static IOPortReadFunc *default_func[3] = {
@@ -295,10 +342,16 @@
         default_ioport_readw,
         default_ioport_readl
     };
-    IOPortReadFunc *func = ioport_read_table[index][address];
+    ioport_t *p = ioport_find(address, 0);
+    IOPortReadFunc *func = NULL;
+    void *opaque = NULL;
+    if(p) {
+      func = p->read[index];
+      opaque = p->opaque;
+    }
     if (!func)
         func = default_func[index];
-    return func(ioport_opaque[address], address);
+    return func(opaque, address);
 }
 
 static void ioport_write(int index, uint32_t address, uint32_t data)
@@ -308,10 +361,16 @@
         default_ioport_writew,
         default_ioport_writel
     };
-    IOPortWriteFunc *func = ioport_write_table[index][address];
+    ioport_t *p = ioport_find(address, 0);
+    IOPortWriteFunc *func = NULL;
+    void *opaque = NULL;
+    if(p) {
+      func = p->write[index];
+      opaque = p->opaque;
+    }
     if (!func)
         func = default_func[index];
-    func(ioport_opaque[address], address, data);
+    func(opaque, address, data);
 }
 
 static uint32_t default_ioport_readb(void *opaque, uint32_t address)
@@ -378,10 +437,11 @@
         return -1;
     }
     for(i = start; i < start + length; i += size) {
-        ioport_read_table[bsize][i] = func;
-        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
+      ioport_t *p = ioport_find(i, 1);
+        p->read[bsize] = func;
+        if (p->opaque != NULL && p->opaque != opaque)
             hw_error("register_ioport_read: invalid opaque");
-        ioport_opaque[i] = opaque;
+        p->opaque = opaque;
     }
     return 0;
 }
@@ -403,10 +463,11 @@
         return -1;
     }
     for(i = start; i < start + length; i += size) {
-        ioport_write_table[bsize][i] = func;
-        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
+        ioport_t *p = ioport_find(i, 1);
+        p->write[bsize] = func;
+        if (p->opaque != NULL && p->opaque != opaque)
             hw_error("register_ioport_write: invalid opaque");
-        ioport_opaque[i] = opaque;
+        p->opaque = opaque;
     }
     return 0;
 }
@@ -416,15 +477,16 @@
     int i;
 
     for(i = start; i < start + length; i++) {
-        ioport_read_table[0][i] = default_ioport_readb;
-        ioport_read_table[1][i] = default_ioport_readw;
-        ioport_read_table[2][i] = default_ioport_readl;
-
-        ioport_write_table[0][i] = default_ioport_writeb;
-        ioport_write_table[1][i] = default_ioport_writew;
-        ioport_write_table[2][i] = default_ioport_writel;
+        ioport_t *p = ioport_find(i, 1);
+        p->read[0] = default_ioport_readb;
+        p->read[1] = default_ioport_readw;
+        p->read[2] = default_ioport_readl;
+
+        p->write[0] = default_ioport_writeb;
+        p->write[1] = default_ioport_writew;
+        p->write[2] = default_ioport_writel;
 
-        ioport_opaque[i] = NULL;
+        p->opaque = NULL;
     }
 }
 

Re: [Qemu-devel] [PATCH] two level table for IO port lookup

[Anthony Liguori]

Brian Wheeler wrote:
> The alpha architecture uses 24 bits for the io port address so this
> patch adds a two level table and puts the IO port data into a
> struct...because sizeof(void *) * 7 * 16777216 is nearly a 1G on my
> workstation.
>
> I've set the alpha target to use a 12/12 split and everything else to
> use 8/8.  
>   

The table lookups really kill performance.  It's probably a better idea 
just to switch to a linear list of IO ports.  There's usually going to 
be a small number of registered IO regions (certainly, less than 100).

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] two level table for IO port lookup

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

Anthony Liguori wrote:
> Brian Wheeler wrote:
>> The alpha architecture uses 24 bits for the io port address so this
>> patch adds a two level table and puts the IO port data into a
>> struct...because sizeof(void *) * 7 * 16777216 is nearly a 1G on my
>> workstation.
>>
>> I've set the alpha target to use a 12/12 split and everything else to
>> use 8/8.    
> 
> The table lookups really kill performance.  It's probably a better idea
> just to switch to a linear list of IO ports.  There's usually going to
> be a small number of registered IO regions (certainly, less than 100).

Sorry, can't follow: you want to search a linear list of at least a few
ten entries on average instead of doing a simple two-stage table lookup?
  If you had said: "Use a tree." Maybe. But even then you can quickly
end up consulting >2 entries before finding the right one.

Nevertheless, numbers on the performance impact and memory consumption
of this patch would surely help to decide which way to go.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] two level table for IO port lookup [Patch updated]

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 8750 bytes --]

Brian Wheeler wrote:
> On Fri, 2009-04-10 at 17:17 +0200, Jan Kiszka wrote:
>> Brian Wheeler wrote:
>>> The alpha architecture uses 24 bits for the io port address so this
>>> patch adds a two level table and puts the IO port data into a
>>> struct...because sizeof(void *) * 7 * 16777216 is nearly a 1G on my
>>> workstation.
>>>
>>> I've set the alpha target to use a 12/12 split and everything else to
>>> use 8/8.  
>>>
>>>
>>>
>>>
>>> Signed-off-by: Brian Wheeler <bdwheele@indiana.edu>
>>>
> 
>>> +  if(ioport[page]==NULL) {
>>> +    ioport[page]=calloc((1<<IOPORT_PAGESIZE), sizeof(ioport_t));
>> As you use ioport_find also for guest-driven port access, doing
>> allocation here is a bad idea. Consider a guest that probes the whole io
>> address space of your alpha box: you would end up with the same gig
>> being allocated that you try to avoid with this approach.
>>
>> IOW: Only allocate on handler registration. On unsuccessful lookup, just
>> call the proper default handler.
>>
> 
> Good point.  This patch does that:  there's an allocate flag which is
> set only by the registration calls so only they will allocate memory.
> 
> Signed-off-by: Brian Wheeler <bdwheele@indiana.edu>
> 
> 
> --- vl.c.orig	2009-04-10 10:01:52.000000000 -0400
> +++ vl.c	2009-04-10 11:40:27.000000000 -0400
> @@ -168,7 +168,7 @@
>  //#define DEBUG_IOPORT
>  //#define DEBUG_NET
>  //#define DEBUG_SLIRP
> -
> +//#define DEBUG_IOPORT_FIND
>  
>  #ifdef DEBUG_IOPORT
>  #  define LOG_IOPORT(...) qemu_log_mask(CPU_LOG_IOPORT, ## __VA_ARGS__)
> @@ -184,14 +184,30 @@
>  /* Max number of bluetooth switches on the commandline.  */
>  #define MAX_BT_CMDLINE 10
>  
> -/* XXX: use a two level table to limit memory usage */
> -#define MAX_IOPORTS 65536
> -
>  const char *bios_dir = CONFIG_QEMU_SHAREDIR;
>  const char *bios_name = NULL;
> -static void *ioport_opaque[MAX_IOPORTS];
> -static IOPortReadFunc *ioport_read_table[3][MAX_IOPORTS];
> -static IOPortWriteFunc *ioport_write_table[3][MAX_IOPORTS];
> +
> +struct ioport {
> +	void *opaque;
> +	IOPortReadFunc *read[3];
> +	IOPortWriteFunc *write[3];
> +};

Hmm, should we pad this struct to 8 pointers?

> +typedef struct ioport ioport_t;
> +
> +#ifdef TARGET_ALPHA
> +#define IOPORT_MAXBITS 24
> +#define IOPORT_PAGESIZE 12

I think IOPORT_PAGEBITS is a better name - the page size isn't 12 bytes.

> +#else
> +#define IOPORT_MAXBITS 16
> +#define IOPORT_PAGESIZE 8

I wonder if it wouldn't be a good idea to tune the page size (in bytes)
to the host page size (or some small multiple of it), e.g. to 7 bits for
32-bit hosts with 4k pages (2^7 * sizeof(<padded-ioport>) = 4096). That
way we could re-introduce initializing the page content to default
handlers, dropping the null function check from the io access fast path.

I think the reason this was once introduced is that lots of ioport pages
with default but non-null content consumed real RAM while null pages
typically don't do this on many OSes. But this two-level table should
already address the issue at its root.

> +#endif
> +
> +#define IOPORT_ENTRYMASK ((1<<IOPORT_PAGESIZE)-1)
> +#define IOPORT_PAGEMASK ~IOPORT_ENTRYMASK
> +#define MAX_IOPORTS (1<<IOPORT_MAXBITS)
> +
> +void *ioport[1<<(IOPORT_MAXBITS-IOPORT_PAGESIZE)];

Why not stick with "ioport_table" as name?

> +
>  /* Note: drives_table[MAX_DRIVES] is a dummy block driver if none available
>     to store the VM snapshots */
>  DriveInfo drives_table[MAX_DRIVES+1];
> @@ -288,6 +304,37 @@
>  static IOPortReadFunc default_ioport_readb, default_ioport_readw, default_ioport_readl;
>  static IOPortWriteFunc default_ioport_writeb, default_ioport_writew, default_ioport_writel;
>  
> +
> +static inline ioport_t *ioport_find(uint32_t address, int allocate) 
> +{
> +  uint32_t page = (address & IOPORT_PAGEMASK) >> IOPORT_PAGESIZE;
> +  uint32_t entry = address & IOPORT_ENTRYMASK;
> +  if(address >= (1<<IOPORT_MAXBITS))
> +    hw_error("Maximum port # for this architecture is %d.  Port %d requested.",
> +	     (1<<IOPORT_MAXBITS)-1, address);

This check shouldn't be performed in the fast path (io access), at least
not unconditionally. The CPU models already have to take care to not
invoke this service with invalid arguments.

BTW, you tend to use 2 spaces as indention. The QEMU style is 4 spaces,
please fix.

> +  
> +  if(ioport[page]==NULL) {

Please check CODING_STYLE for proper formatting.

> +    if(allocate) {
> +      ioport[page]=calloc((1<<IOPORT_PAGESIZE), sizeof(ioport_t));

If calloc fails, qemu will crash. Use qemu_mallocz instead, it fails
gracefully in case of OOM.

> +#ifdef DEBUG_IOPORT_FIND
> +    printf("Initializing ioport page %d to: %p\n", page, ioport[page]);
> +#endif
> +    } else {
> +      return NULL;
> +    }
> +  }
> +  ioport_t *p = (ioport_t *)(ioport[page] + entry * sizeof(ioport_t));

You can beautify this a lot by declaring ioport as
"ioport_t *ioport[...]"...

> +
> +#ifdef DEBUG_IOPORT_FIND
> +  printf("port find %d:  page=%d, address=%p, entry=%d, address=%p\n", 
> +	 address, page, ioport[page], entry, p);
> +  printf("  data: %p\n", p->opaque);
> +  printf("  read: %p, %p, %p\n", p->read[0], p->read[1], p->read[2]);
> +  printf(" write: %p, %p, %p\n", p->write[0], p->write[1], p->write[2]);
> +#endif
> +  return p;
> +}
> +
>  static uint32_t ioport_read(int index, uint32_t address)
>  {
>      static IOPortReadFunc *default_func[3] = {
> @@ -295,10 +342,16 @@
>          default_ioport_readw,
>          default_ioport_readl
>      };
> -    IOPortReadFunc *func = ioport_read_table[index][address];
> +    ioport_t *p = ioport_find(address, 0);
> +    IOPortReadFunc *func = NULL;
> +    void *opaque = NULL;
> +    if(p) {
> +      func = p->read[index];
> +      opaque = p->opaque;
> +    }
>      if (!func)
>          func = default_func[index];
> -    return func(ioport_opaque[address], address);
> +    return func(opaque, address);
>  }
>  
>  static void ioport_write(int index, uint32_t address, uint32_t data)
> @@ -308,10 +361,16 @@
>          default_ioport_writew,
>          default_ioport_writel
>      };
> -    IOPortWriteFunc *func = ioport_write_table[index][address];
> +    ioport_t *p = ioport_find(address, 0);
> +    IOPortWriteFunc *func = NULL;
> +    void *opaque = NULL;
> +    if(p) {
> +      func = p->write[index];
> +      opaque = p->opaque;
> +    }
>      if (!func)
>          func = default_func[index];
> -    func(ioport_opaque[address], address, data);
> +    func(opaque, address, data);
>  }
>  
>  static uint32_t default_ioport_readb(void *opaque, uint32_t address)
> @@ -378,10 +437,11 @@
>          return -1;
>      }
>      for(i = start; i < start + length; i += size) {
> -        ioport_read_table[bsize][i] = func;
> -        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
> +      ioport_t *p = ioport_find(i, 1);
> +        p->read[bsize] = func;
> +        if (p->opaque != NULL && p->opaque != opaque)
>              hw_error("register_ioport_read: invalid opaque");
> -        ioport_opaque[i] = opaque;
> +        p->opaque = opaque;
>      }
>      return 0;
>  }
> @@ -403,10 +463,11 @@
>          return -1;
>      }
>      for(i = start; i < start + length; i += size) {
> -        ioport_write_table[bsize][i] = func;
> -        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
> +        ioport_t *p = ioport_find(i, 1);
> +        p->write[bsize] = func;
> +        if (p->opaque != NULL && p->opaque != opaque)
>              hw_error("register_ioport_write: invalid opaque");
> -        ioport_opaque[i] = opaque;
> +        p->opaque = opaque;
>      }
>      return 0;
>  }
> @@ -416,15 +477,16 @@
>      int i;
>  
>      for(i = start; i < start + length; i++) {
> -        ioport_read_table[0][i] = default_ioport_readb;
> -        ioport_read_table[1][i] = default_ioport_readw;
> -        ioport_read_table[2][i] = default_ioport_readl;
> -
> -        ioport_write_table[0][i] = default_ioport_writeb;
> -        ioport_write_table[1][i] = default_ioport_writew;
> -        ioport_write_table[2][i] = default_ioport_writel;
> +        ioport_t *p = ioport_find(i, 1);
> +        p->read[0] = default_ioport_readb;
> +        p->read[1] = default_ioport_readw;
> +        p->read[2] = default_ioport_readl;
> +
> +        p->write[0] = default_ioport_writeb;
> +        p->write[1] = default_ioport_writew;
> +        p->write[2] = default_ioport_writel;
>  
> -        ioport_opaque[i] = NULL;
> +        p->opaque = NULL;
>      }
>  }
>  

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: [PATCH] two level table for IO port lookup [Patch V3]

[Brian Wheeler]

On Fri, 2009-04-10 at 19:26 +0200, Jan Kiszka wrote:
> Brian Wheeler wrote:
> > On Fri, 2009-04-10 at 17:17 +0200, Jan Kiszka wrote:
> >> Brian Wheeler wrote:
> > +
> > +struct ioport {
> > +	void *opaque;
> > +	IOPortReadFunc *read[3];
> > +	IOPortWriteFunc *write[3];
> > +};
> 
> Hmm, should we pad this struct to 8 pointers?
> 

Probably.  I've done that in my working patch.


> > +typedef struct ioport ioport_t;
> > +
> > +#ifdef TARGET_ALPHA
> > +#define IOPORT_MAXBITS 24
> > +#define IOPORT_PAGESIZE 12
> 
> I think IOPORT_PAGEBITS is a better name - the page size isn't 12 bytes.
> 

Yes, its a better name.


> > +#else
> > +#define IOPORT_MAXBITS 16
> > +#define IOPORT_PAGESIZE 8
> 
> I wonder if it wouldn't be a good idea to tune the page size (in bytes)
> to the host page size (or some small multiple of it), e.g. to 7 bits for
> 32-bit hosts with 4k pages (2^7 * sizeof(<padded-ioport>) = 4096). That
> way we could re-introduce initializing the page content to default
> handlers, dropping the null function check from the io access fast path.
> 
> I think the reason this was once introduced is that lots of ioport pages
> with default but non-null content consumed real RAM while null pages
> typically don't do this on many OSes. But this two-level table should
> already address the issue at its root.
> 

I'm going to have to defer to someone else on this, because I don't
know.  I tried the patch with other values and it worked ok, so if we
decide to tune it later it shouldn't require any code changes.  Using 8
bits for the entry size gives a table size of 8K, so it is two x86-32
pages which isn't bad. 

On the topic of the extra null checks, I've rewritten it so non-allocate
find with a null page returns a pointer to a default ioport_t -- that
way ioport_find never returns null and the check isn't needed.


> > +#endif
> > +
> > +#define IOPORT_ENTRYMASK ((1<<IOPORT_PAGESIZE)-1)
> > +#define IOPORT_PAGEMASK ~IOPORT_ENTRYMASK
> > +#define MAX_IOPORTS (1<<IOPORT_MAXBITS)
> > +
> > +void *ioport[1<<(IOPORT_MAXBITS-IOPORT_PAGESIZE)];
> 
> Why not stick with "ioport_table" as name?


Yep, that's a better name.

> > +
> > +static inline ioport_t *ioport_find(uint32_t address, int allocate) 
> > +{
> > +  uint32_t page = (address & IOPORT_PAGEMASK) >> IOPORT_PAGESIZE;
> > +  uint32_t entry = address & IOPORT_ENTRYMASK;
> > +  if(address >= (1<<IOPORT_MAXBITS))
> > +    hw_error("Maximum port # for this architecture is %d.  Port %d requested.",
> > +	     (1<<IOPORT_MAXBITS)-1, address);
> 
> This check shouldn't be performed in the fast path (io access), at least
> not unconditionally. The CPU models already have to take care to not
> invoke this service with invalid arguments.
> 

I put it into the #ifdef DEBUG_IOPORT_FIND block, so it'll only get
called if there's debugging in process.


> BTW, you tend to use 2 spaces as indention. The QEMU style is 4 spaces,
> please fix.
> 

I blame emacs.  I've fixed it.


> > +  
> > +  if(ioport[page]==NULL) {
> 
> Please check CODING_STYLE for proper formatting.
> 

Ok, I've got through the patch and tried to find all of the coding style
issues.

> > +    if(allocate) {
> > +      ioport[page]=calloc((1<<IOPORT_PAGESIZE), sizeof(ioport_t));
> 
> If calloc fails, qemu will crash. Use qemu_mallocz instead, it fails
> gracefully in case of OOM.
> 

Done


> > +#ifdef DEBUG_IOPORT_FIND
> > +    printf("Initializing ioport page %d to: %p\n", page, ioport[page]);
> > +#endif
> > +    } else {
> > +      return NULL;
> > +    }
> > +  }
> > +  ioport_t *p = (ioport_t *)(ioport[page] + entry * sizeof(ioport_t));
> 
> You can beautify this a lot by declaring ioport as
> "ioport_t *ioport[...]"...
> 

Yeah, my C is really rusty so I knew what I wanted to do, but I couldn't
remember how to do it so it was pretty.

ioport is a pointer to the table of ioport_t entries.  How do I declare
and use it?  If I declare ioport as:

ioport_t *ioport[1 << (IOPORT_MAXBITS-IOPORT_PAGEBITS)];

then I can take out the cast, but how do I get the table entry since I
have to dereference the ioport[page] to get to the array?


Here's the updated patch:

Signed-off-by: Brian Wheeler <bdwheele@indiana.edu>

--- vl.c.orig	2009-04-10 10:01:52.000000000 -0400
+++ vl.c	2009-04-10 14:52:07.000000000 -0400
@@ -168,7 +168,7 @@
 //#define DEBUG_IOPORT
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
-
+//#define DEBUG_IOPORT_FIND
 
 #ifdef DEBUG_IOPORT
 #  define LOG_IOPORT(...) qemu_log_mask(CPU_LOG_IOPORT, ## __VA_ARGS__)
@@ -184,14 +184,31 @@
 /* Max number of bluetooth switches on the commandline.  */
 #define MAX_BT_CMDLINE 10
 
-/* XXX: use a two level table to limit memory usage */
-#define MAX_IOPORTS 65536
-
 const char *bios_dir = CONFIG_QEMU_SHAREDIR;
 const char *bios_name = NULL;
-static void *ioport_opaque[MAX_IOPORTS];
-static IOPortReadFunc *ioport_read_table[3][MAX_IOPORTS];
-static IOPortWriteFunc *ioport_write_table[3][MAX_IOPORTS];
+
+struct ioport {
+    void *opaque;
+    IOPortReadFunc *read[3];
+    IOPortWriteFunc *write[3];
+    void *pad;
+};
+typedef struct ioport ioport_t;
+
+#ifdef TARGET_ALPHA
+#define IOPORT_MAXBITS 24
+#define IOPORT_PAGEBITS 12
+#else
+#define IOPORT_MAXBITS 16
+#define IOPORT_PAGEBITS 8
+#endif
+
+#define IOPORT_ENTRYMASK ((1<<IOPORT_PAGEBITS)-1)
+#define IOPORT_PAGEMASK ~IOPORT_ENTRYMASK
+#define MAX_IOPORTS (1<<IOPORT_MAXBITS)
+
+void *ioport_table[1<<(IOPORT_MAXBITS-IOPORT_PAGEBITS)];
+
 /* Note: drives_table[MAX_DRIVES] is a dummy block driver if none available
    to store the VM snapshots */
 DriveInfo drives_table[MAX_DRIVES+1];
@@ -288,30 +305,67 @@
 static IOPortReadFunc default_ioport_readb, default_ioport_readw, default_ioport_readl;
 static IOPortWriteFunc default_ioport_writeb, default_ioport_writew, default_ioport_writel;
 
+
+static ioport_t ioport_default_func = {
+    NULL,
+    {default_ioport_readb, default_ioport_readw, default_ioport_readl},
+    {default_ioport_writeb, default_ioport_writew, default_ioport_writel},
+    NULL
+};
+
+static inline ioport_t *ioport_find(uint32_t address, int allocate) 
+{
+    uint32_t page = (address & IOPORT_PAGEMASK) >> IOPORT_PAGEBITS;
+    uint32_t entry = address & IOPORT_ENTRYMASK;
+#ifdef DEBUG_IOPORT_FIND
+    if (address >= (1<<IOPORT_MAXBITS)) {
+	hw_error("Maximum port # for this architecture is %d.  "
+		 "Port %d requested.", (1<<IOPORT_MAXBITS)-1, address);
+    }
+#endif
+    if (ioport_table[page] == NULL) {
+	if (allocate) {
+	    ioport_table[page]=qemu_mallocz((1<<IOPORT_PAGEBITS) * 
+					    sizeof(ioport_t));
+#ifdef DEBUG_IOPORT_FIND
+	    printf("Initializing ioport page %d to: %p\n", page, ioport_table[page]);
+#endif
+	} else {
+	    return &ioport_default_func;
+	}
+    }
+    ioport_t *p = (ioport_table[page] + entry * sizeof(ioport_t));
+  
+#ifdef DEBUG_IOPORT_FIND
+    printf("port find %d:  page=%d, address=%p, entry=%d, address=%p\n", 
+	   address, page, ioport_table[page], entry, p);
+    printf("  data: %p\n", p->opaque);
+    printf("  read: %p, %p, %p\n", p->read[0], p->read[1], p->read[2]);
+    printf(" write: %p, %p, %p\n", p->write[0], p->write[1], p->write[2]);
+#endif
+    return p;
+}
+
 static uint32_t ioport_read(int index, uint32_t address)
 {
-    static IOPortReadFunc *default_func[3] = {
-        default_ioport_readb,
-        default_ioport_readw,
-        default_ioport_readl
-    };
-    IOPortReadFunc *func = ioport_read_table[index][address];
-    if (!func)
-        func = default_func[index];
-    return func(ioport_opaque[address], address);
+    ioport_t *p = ioport_find(address, 0);
+    IOPortReadFunc *func = p->read[index];
+    void *opaque = p->opaque;
+    if (!func) {
+        func = ioport_default_func.read[index];
+    }
+    return func(opaque, address);
 }
 
 static void ioport_write(int index, uint32_t address, uint32_t data)
 {
-    static IOPortWriteFunc *default_func[3] = {
-        default_ioport_writeb,
-        default_ioport_writew,
-        default_ioport_writel
-    };
-    IOPortWriteFunc *func = ioport_write_table[index][address];
-    if (!func)
-        func = default_func[index];
-    func(ioport_opaque[address], address, data);
+    ioport_t *p = ioport_find(address, 0);
+    IOPortWriteFunc *func = p->write[index];
+    void *opaque = p->opaque;
+    if (!func) {
+        func = ioport_default_func.write[index];
+    }
+    func(opaque, address, data);
 }
 
 static uint32_t default_ioport_readb(void *opaque, uint32_t address)
@@ -378,10 +432,11 @@
         return -1;
     }
     for(i = start; i < start + length; i += size) {
-        ioport_read_table[bsize][i] = func;
-        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
+	 ioport_t *p = ioport_find(i, 1);
+        p->read[bsize] = func;
+        if (p->opaque != NULL && p->opaque != opaque)
             hw_error("register_ioport_read: invalid opaque");
-        ioport_opaque[i] = opaque;
+        p->opaque = opaque;
     }
     return 0;
 }
@@ -403,10 +458,11 @@
         return -1;
     }
     for(i = start; i < start + length; i += size) {
-        ioport_write_table[bsize][i] = func;
-        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
+        ioport_t *p = ioport_find(i, 1);
+        p->write[bsize] = func;
+        if (p->opaque != NULL && p->opaque != opaque)
             hw_error("register_ioport_write: invalid opaque");
-        ioport_opaque[i] = opaque;
+        p->opaque = opaque;
     }
     return 0;
 }
@@ -416,15 +472,16 @@
     int i;
 
     for(i = start; i < start + length; i++) {
-        ioport_read_table[0][i] = default_ioport_readb;
-        ioport_read_table[1][i] = default_ioport_readw;
-        ioport_read_table[2][i] = default_ioport_readl;
-
-        ioport_write_table[0][i] = default_ioport_writeb;
-        ioport_write_table[1][i] = default_ioport_writew;
-        ioport_write_table[2][i] = default_ioport_writel;
+        ioport_t *p = ioport_find(i, 1);
+        p->read[0] = default_ioport_readb;
+        p->read[1] = default_ioport_readw;
+        p->read[2] = default_ioport_readl;
+
+        p->write[0] = default_ioport_writeb;
+        p->write[1] = default_ioport_writew;
+        p->write[2] = default_ioport_writel;
 
-        ioport_opaque[i] = NULL;
+        p->opaque = NULL;
     }
 }

Re: [Qemu-devel] [PATCH] two level table for IO port lookup

[Brian Wheeler]

On Fri, 2009-04-10 at 11:03 -0500, Anthony Liguori wrote:
> Brian Wheeler wrote:
> > The alpha architecture uses 24 bits for the io port address so this
> > patch adds a two level table and puts the IO port data into a
> > struct...because sizeof(void *) * 7 * 16777216 is nearly a 1G on my
> > workstation.
> >
> > I've set the alpha target to use a 12/12 split and everything else to
> > use 8/8.  
> >   
> 
> The table lookups really kill performance.  It's probably a better idea 
> just to switch to a linear list of IO ports.  There's usually going to 
> be a small number of registered IO regions (certainly, less than 100).

Well, on ioport_read the hot path does 2 lookups and a null check.  The
new one does 3 lookups, and two null checks.  Its probably slower, but I
don't know if it would kill performance, per se...especially since IO
port access isn't the fastest in real life anyway.

Brian

Re: [Qemu-devel] [PATCH] two level table for IO port lookup

[Jamie Lokier]

Brian Wheeler wrote:
> On Fri, 2009-04-10 at 11:03 -0500, Anthony Liguori wrote:
> > Brian Wheeler wrote:
> > > The alpha architecture uses 24 bits for the io port address so this
> > > patch adds a two level table and puts the IO port data into a
> > > struct...because sizeof(void *) * 7 * 16777216 is nearly a 1G on my
> > > workstation.
> > >
> > > I've set the alpha target to use a 12/12 split and everything else to
> > > use 8/8.  
> > >   
> > 
> > The table lookups really kill performance.  It's probably a better idea 
> > just to switch to a linear list of IO ports.  There's usually going to 
> > be a small number of registered IO regions (certainly, less than 100).
> 

> Well, on ioport_read the hot path does 2 lookups and a null check.  The
> new one does 3 lookups, and two null checks.

You could reduce it to one null check by pointing null top-level
entries to an a shared all-nulls leaf table.  I doubt it would make
any difference to performance though.

-- Jamie