diff mbox

[v2] kvm: Use a bitmap for tracking used GSIs

Message ID 20090508222925.5119.94814.stgit@dl380g6-3.ned.telco.ned.telco (mailing list archive)
State New, archived
Headers show

Commit Message

Alex Williamson May 8, 2009, 10:31 p.m. UTC 
We're currently using a counter to track the most recent GSI we've
handed out.  This quickly hits KVM_MAX_IRQ_ROUTES when using device
assignment with a driver that regularly toggles the MSI enable bit.
This can mean only a few minutes of usable run time.  Instead, track
used GSIs in a bitmap.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 v2: Add a mutex around the bitmap to protect from races

 hw/device-assignment.c  |    4 ++
 kvm/libkvm/kvm-common.h |    4 ++
 kvm/libkvm/libkvm.c     |   82 ++++++++++++++++++++++++++++++++++++++++++-----
 kvm/libkvm/libkvm.h     |   10 ++++++
 4 files changed, 89 insertions(+), 11 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Michael S. Tsirkin May 12, 2009, 7:39 p.m. UTC | #1 
On Fri, May 08, 2009 at 04:31:21PM -0600, Alex Williamson wrote:
> +#ifdef KVM_CAP_IRQ_ROUTING

We don't need these anymore.

> +static inline void set_bit(unsigned int *buf, int bit)
> +{
> +	buf[bit >> 5] |= (1U << (bit & 0x1f));
> +}

external () not needed here. bit >> 5 might be clearer as bit / 32 IMO.

> +
> +static inline void clear_bit(unsigned int *buf, int bit)
> +{
> +	buf[bit >> 5] &= ~(1U << (bit & 0x1f));
> +}

Make bit unsigned. And then bit & 0x1f can be written as bit % 32.
IMO that's easier to parse.

> +
> +static int kvm_find_free_gsi(kvm_context_t kvm)
> +{
> +	int i, bit, gsi;
> +	unsigned int *buf = kvm->used_gsi_bitmap;
> +
> +	for (i = 0; i < (kvm->max_gsi >> 5); i++) {

may be clearer as kvm->max_gsi / 32

> +		if (buf[i] != ~0U)
> +			break;
> +	}

{} around single statement isn't needed

> +
> +	if (i == kvm->max_gsi >> 5)
> +		return -ENOSPC;

May be clearer as kvm->max_gsi / 32
By the way, this math means we can't use all gsi's
if the number is not a multiple of 32.
Round up instead? It's not hard: (kvm->max_gsi + 31) / 32

> +
> +	bit = ffs(~buf[i]);
> +	if (!bit)
> +		return -EAGAIN;

We know it won't be 0, right?
Instead of checking twice, move the ffs call within the loop above?
E.g. like this:
	for (i = 0; i < kvm->max_gsi / 32; ++i)
		if ((bit = ffs(~buf[i])) {
		    gsi = bit - 1 + i * 32;
		    set_bit(buf, gsi);
		    return gsi;
		}

	return -ENOSPC;

> +
> +	gsi = (bit - 1) | (i << 5);

clearer as bit - 1 + i * 32

> +	set_bit(buf, gsi);
> +	return gsi;
> +}
> +#endif
> +
>  int kvm_get_irq_route_gsi(kvm_context_t kvm)
>  {
>  #ifdef KVM_CAP_IRQ_ROUTING
> -	if (kvm->max_used_gsi >= KVM_IOAPIC_NUM_PINS)  {
> -	    if (kvm->max_used_gsi + 1 < kvm_get_gsi_count(kvm))
> -                return kvm->max_used_gsi + 1;
> -            else
> -                return -ENOSPC;
> -        } else
> -            return KVM_IOAPIC_NUM_PINS;
> +	int gsi;
> +
> +	pthread_mutex_lock(&kvm->gsi_mutex);
> +
> +	if (!kvm->max_gsi) {

Why is this lazy allocation required?
Let's do the below in some init function,
and keep code simple?

> +		int i;
> +
> +		/* Round the number of GSIs supported to a 4 byte
> +		 * value so we can search it using ints and ffs */
> +		i = kvm_get_gsi_count(kvm) & ~0x1f;

Should not we round up? Why not?
Again, we can count = kvm_get_gsi_count(kvm) / 32, and use count * 4 below.

> +		kvm->used_gsi_bitmap = malloc(i >> 3);

why not qemu_malloc?

> +		if (!kvm->used_gsi_bitmap) {
> +			pthread_mutex_unlock(&kvm->gsi_mutex);
> +			return -ENOMEM;
> +		}
> +		memset(kvm->used_gsi_bitmap, 0, i >> 3);
> +		kvm->max_gsi = i;
> +
> +		/* Mark all the IOAPIC pin GSIs as already used */
> +		for (i = 0; i <= KVM_IOAPIC_NUM_PINS; i++)

Is this really <=? Not <?

> +			set_bit(kvm->used_gsi_bitmap, i);
> +	}
> +
> +	gsi = kvm_find_free_gsi(kvm);
> +	pthread_mutex_unlock(&kvm->gsi_mutex);
> +	return gsi;
>  #else
>  	return -ENOSYS;
>  #endif
>  }
> + 
> +void kvm_free_irq_route_gsi(kvm_context_t kvm, int gsi)
> +{
> +#ifdef KVM_CAP_IRQ_ROUTING
> +	pthread_mutex_lock(&kvm->gsi_mutex);
> +	clear_bit(kvm->used_gsi_bitmap, gsi);
> +	pthread_mutex_unlock(&kvm->gsi_mutex);
> +#endif
> +}
>  
>  #ifdef KVM_CAP_DEVICE_MSIX
>  int kvm_assign_set_msix_nr(kvm_context_t kvm,
> diff --git a/kvm/libkvm/libkvm.h b/kvm/libkvm/libkvm.h
> index c23d37b..4e9344c 100644
> --- a/kvm/libkvm/libkvm.h
> +++ b/kvm/libkvm/libkvm.h
> @@ -856,6 +856,16 @@ int kvm_commit_irq_routes(kvm_context_t kvm);
>   */
>  int kvm_get_irq_route_gsi(kvm_context_t kvm);
>  
> +/*!
> + * \brief Free used GSI number
> + *
> + * Free used GSI number acquired from kvm_get_irq_route_gsi()
> + *
> + * \param kvm Pointer to the current kvm_context
> + * \param gsi GSI number to free
> + */
> +void kvm_free_irq_route_gsi(kvm_context_t kvm, int gsi);
> +
>  #ifdef KVM_CAP_DEVICE_MSIX
>  int kvm_assign_set_msix_nr(kvm_context_t kvm,
>  			   struct kvm_assigned_msix_nr *msix_nr);
>
Alex Williamson May 12, 2009, 9:56 p.m. UTC | #2 
On Tue, 2009-05-12 at 22:39 +0300, Michael S. Tsirkin wrote:
> On Fri, May 08, 2009 at 04:31:21PM -0600, Alex Williamson wrote:
> > +#ifdef KVM_CAP_IRQ_ROUTING
> 
> We don't need these anymore.

Doesn't that depend on the kernel it's built against?  In any case, I
think it would deserve it's own patch to remove those throughout.
Removing them here would make their usage inconsistent.

> > +static inline void set_bit(unsigned int *buf, int bit)
> > +{
> > +	buf[bit >> 5] |= (1U << (bit & 0x1f));
> > +}
> 
> external () not needed here. bit >> 5 might be clearer as bit / 32 IMO.

Fixed

> > +
> > +static inline void clear_bit(unsigned int *buf, int bit)
> > +{
> > +	buf[bit >> 5] &= ~(1U << (bit & 0x1f));
> > +}
> 
> Make bit unsigned. And then bit & 0x1f can be written as bit % 32.
> IMO that's easier to parse.

Fixed

> > +
> > +static int kvm_find_free_gsi(kvm_context_t kvm)
> > +{
> > +	int i, bit, gsi;
> > +	unsigned int *buf = kvm->used_gsi_bitmap;
> > +
> > +	for (i = 0; i < (kvm->max_gsi >> 5); i++) {
> 
> may be clearer as kvm->max_gsi / 32

Fixed

> > +		if (buf[i] != ~0U)
> > +			break;
> > +	}
> 
> {} around single statement isn't needed

This is different in my new version, so the {} makes sense.

> > +
> > +	if (i == kvm->max_gsi >> 5)
> > +		return -ENOSPC;
> 
> May be clearer as kvm->max_gsi / 32
> By the way, this math means we can't use all gsi's
> if the number is not a multiple of 32.
> Round up instead? It's not hard: (kvm->max_gsi + 31) / 32

Fixed

> > +
> > +	bit = ffs(~buf[i]);
> > +	if (!bit)
> > +		return -EAGAIN;
> 
> We know it won't be 0, right?
> Instead of checking twice, move the ffs call within the loop above?
> E.g. like this:
> 	for (i = 0; i < kvm->max_gsi / 32; ++i)
> 		if ((bit = ffs(~buf[i])) {
> 		    gsi = bit - 1 + i * 32;
> 		    set_bit(buf, gsi);
> 		    return gsi;
> 		}
> 
> 	return -ENOSPC;

Nice, I updated to something similar.

> > +
> > +	gsi = (bit - 1) | (i << 5);
> 
> clearer as bit - 1 + i * 32

Yup, fixed.

> > +	set_bit(buf, gsi);
> > +	return gsi;
> > +}
> > +#endif
> > +
> >  int kvm_get_irq_route_gsi(kvm_context_t kvm)
> >  {
> >  #ifdef KVM_CAP_IRQ_ROUTING
> > -	if (kvm->max_used_gsi >= KVM_IOAPIC_NUM_PINS)  {
> > -	    if (kvm->max_used_gsi + 1 < kvm_get_gsi_count(kvm))
> > -                return kvm->max_used_gsi + 1;
> > -            else
> > -                return -ENOSPC;
> > -        } else
> > -            return KVM_IOAPIC_NUM_PINS;
> > +	int gsi;
> > +
> > +	pthread_mutex_lock(&kvm->gsi_mutex);
> > +
> > +	if (!kvm->max_gsi) {
> 
> Why is this lazy allocation required?
> Let's do the below in some init function,
> and keep code simple?

Moved to kvm_init()

> > +		int i;
> > +
> > +		/* Round the number of GSIs supported to a 4 byte
> > +		 * value so we can search it using ints and ffs */
> > +		i = kvm_get_gsi_count(kvm) & ~0x1f;
> 
> Should not we round up? Why not?
> Again, we can count = kvm_get_gsi_count(kvm) / 32, and use count * 4 below.

Yep, rounded up, and pre-marked the excess bits as used.

> > +		kvm->used_gsi_bitmap = malloc(i >> 3);
> 
> why not qemu_malloc?

qemu_malloc isn't available in libkvm.c.  malloc is consistent with the
rest of the file.

> > +		if (!kvm->used_gsi_bitmap) {
> > +			pthread_mutex_unlock(&kvm->gsi_mutex);
> > +			return -ENOMEM;
> > +		}
> > +		memset(kvm->used_gsi_bitmap, 0, i >> 3);
> > +		kvm->max_gsi = i;
> > +
> > +		/* Mark all the IOAPIC pin GSIs as already used */
> > +		for (i = 0; i <= KVM_IOAPIC_NUM_PINS; i++)
> 
> Is this really <=? Not <?

Fixed.

Thanks, I'll send out a new version shortly.

Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index e06dd08..5bdae24 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -561,8 +561,10 @@  static void free_dev_irq_entries(AssignedDevice *dev)
 {
     int i;
 
-    for (i = 0; i < dev->irq_entries_nr; i++)
+    for (i = 0; i < dev->irq_entries_nr; i++) {
         kvm_del_routing_entry(kvm_context, &dev->entry[i]);
+        kvm_free_irq_route_gsi(kvm_context, dev->entry[i].gsi);
+    }
     free(dev->entry);
     dev->entry = NULL;
     dev->irq_entries_nr = 0;
diff --git a/kvm/libkvm/kvm-common.h b/kvm/libkvm/kvm-common.h
index 591fb53..4b3cb51 100644
--- a/kvm/libkvm/kvm-common.h
+++ b/kvm/libkvm/kvm-common.h
@@ -66,8 +66,10 @@  struct kvm_context {
 #ifdef KVM_CAP_IRQ_ROUTING
 	struct kvm_irq_routing *irq_routes;
 	int nr_allocated_irq_routes;
+	void *used_gsi_bitmap;
+	int max_gsi;
+	pthread_mutex_t gsi_mutex;
 #endif
-	int max_used_gsi;
 };
 
 int kvm_alloc_kernel_memory(kvm_context_t kvm, unsigned long memory,
diff --git a/kvm/libkvm/libkvm.c b/kvm/libkvm/libkvm.c
index 2a4165a..13a2c21 100644
--- a/kvm/libkvm/libkvm.c
+++ b/kvm/libkvm/libkvm.c
@@ -35,6 +35,7 @@ 
 #include <errno.h>
 #include <sys/ioctl.h>
 #include <inttypes.h>
+#include <pthread.h>
 #include "libkvm.h"
 
 #if defined(__x86_64__) || defined(__i386__)
@@ -322,6 +323,9 @@  kvm_context_t kvm_init(struct kvm_callbacks *callbacks,
 	kvm->dirty_pages_log_all = 0;
 	kvm->no_irqchip_creation = 0;
 	kvm->no_pit_creation = 0;
+#ifdef KVM_CAP_IRQ_ROUTING
+	pthread_mutex_init(&kvm->gsi_mutex, NULL);
+#endif
 
 	return kvm;
  out_close:
@@ -1298,8 +1302,6 @@  int kvm_add_routing_entry(kvm_context_t kvm,
 	new->flags = entry->flags;
 	new->u = entry->u;
 
-	if (entry->gsi > kvm->max_used_gsi)
-		kvm->max_used_gsi = entry->gsi;
 	return 0;
 #else
 	return -ENOSYS;
@@ -1404,20 +1406,82 @@  int kvm_commit_irq_routes(kvm_context_t kvm)
 #endif
 }
 
+#ifdef KVM_CAP_IRQ_ROUTING
+static inline void set_bit(unsigned int *buf, int bit)
+{
+	buf[bit >> 5] |= (1U << (bit & 0x1f));
+}
+
+static inline void clear_bit(unsigned int *buf, int bit)
+{
+	buf[bit >> 5] &= ~(1U << (bit & 0x1f));
+}
+
+static int kvm_find_free_gsi(kvm_context_t kvm)
+{
+	int i, bit, gsi;
+	unsigned int *buf = kvm->used_gsi_bitmap;
+
+	for (i = 0; i < (kvm->max_gsi >> 5); i++) {
+		if (buf[i] != ~0U)
+			break;
+	}
+
+	if (i == kvm->max_gsi >> 5)
+		return -ENOSPC;
+
+	bit = ffs(~buf[i]);
+	if (!bit)
+		return -EAGAIN;
+
+	gsi = (bit - 1) | (i << 5);
+	set_bit(buf, gsi);
+	return gsi;
+}
+#endif
+
 int kvm_get_irq_route_gsi(kvm_context_t kvm)
 {
 #ifdef KVM_CAP_IRQ_ROUTING
-	if (kvm->max_used_gsi >= KVM_IOAPIC_NUM_PINS)  {
-	    if (kvm->max_used_gsi + 1 < kvm_get_gsi_count(kvm))
-                return kvm->max_used_gsi + 1;
-            else
-                return -ENOSPC;
-        } else
-            return KVM_IOAPIC_NUM_PINS;
+	int gsi;
+
+	pthread_mutex_lock(&kvm->gsi_mutex);
+
+	if (!kvm->max_gsi) {
+		int i;
+
+		/* Round the number of GSIs supported to a 4 byte
+		 * value so we can search it using ints and ffs */
+		i = kvm_get_gsi_count(kvm) & ~0x1f;
+		kvm->used_gsi_bitmap = malloc(i >> 3);
+		if (!kvm->used_gsi_bitmap) {
+			pthread_mutex_unlock(&kvm->gsi_mutex);
+			return -ENOMEM;
+		}
+		memset(kvm->used_gsi_bitmap, 0, i >> 3);
+		kvm->max_gsi = i;
+
+		/* Mark all the IOAPIC pin GSIs as already used */
+		for (i = 0; i <= KVM_IOAPIC_NUM_PINS; i++)
+			set_bit(kvm->used_gsi_bitmap, i);
+	}
+
+	gsi = kvm_find_free_gsi(kvm);
+	pthread_mutex_unlock(&kvm->gsi_mutex);
+	return gsi;
 #else
 	return -ENOSYS;
 #endif
 }
+ 
+void kvm_free_irq_route_gsi(kvm_context_t kvm, int gsi)
+{
+#ifdef KVM_CAP_IRQ_ROUTING
+	pthread_mutex_lock(&kvm->gsi_mutex);
+	clear_bit(kvm->used_gsi_bitmap, gsi);
+	pthread_mutex_unlock(&kvm->gsi_mutex);
+#endif
+}
 
 #ifdef KVM_CAP_DEVICE_MSIX
 int kvm_assign_set_msix_nr(kvm_context_t kvm,
diff --git a/kvm/libkvm/libkvm.h b/kvm/libkvm/libkvm.h
index c23d37b..4e9344c 100644
--- a/kvm/libkvm/libkvm.h
+++ b/kvm/libkvm/libkvm.h
@@ -856,6 +856,16 @@  int kvm_commit_irq_routes(kvm_context_t kvm);
  */
 int kvm_get_irq_route_gsi(kvm_context_t kvm);
 
+/*!
+ * \brief Free used GSI number
+ *
+ * Free used GSI number acquired from kvm_get_irq_route_gsi()
+ *
+ * \param kvm Pointer to the current kvm_context
+ * \param gsi GSI number to free
+ */
+void kvm_free_irq_route_gsi(kvm_context_t kvm, int gsi);
+
 #ifdef KVM_CAP_DEVICE_MSIX
 int kvm_assign_set_msix_nr(kvm_context_t kvm,
 			   struct kvm_assigned_msix_nr *msix_nr);