| Message ID | 20090507222015.5216.18027.stgit@dl380g6-3.ned.telco.ned.telco (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 08 May 2009 06:22:20 Alex Williamson wrote: > 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> > --- > > Applies on top of "kvm: device-assignment: Catch GSI overflow" > > hw/device-assignment.c | 4 ++- > kvm/libkvm/kvm-common.h | 3 +- > kvm/libkvm/libkvm.c | 68 > +++++++++++++++++++++++++++++++++++++++++------ kvm/libkvm/libkvm.h | > 10 +++++++ > 4 files changed, 74 insertions(+), 11 deletions(-) > > 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..94f86e5 100644 > --- a/kvm/libkvm/kvm-common.h > +++ b/kvm/libkvm/kvm-common.h > @@ -66,8 +66,9 @@ 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; > #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..43abc7d 100644 > --- a/kvm/libkvm/libkvm.c > +++ b/kvm/libkvm/libkvm.c > @@ -1298,8 +1298,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 +1402,72 @@ 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; > + 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); 3 or 5? I am a little confused by these magic numbers, including 0x1f... I think there are something can indicate the length of unsigned long in QEmu(sorry, can't find it now...), so how about using ffsl() and get other constants based on it?
On Mon, 2009-05-11 at 12:00 +0000, Yang, Sheng wrote: > On Friday 08 May 2009 06:22:20 Alex Williamson wrote: > > + /* 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); > > 3 or 5? 3, ie. /8 (bits to bytes) > I am a little confused by these magic numbers, including 0x1f... The 5 shift gives us the index into the array of ints, the 0x1f gives us the bit index into a specific int. This is very similar to the code in hw/acpi.c. > I think there are something can indicate the length of unsigned long in > QEmu(sorry, can't find it now...), so how about using ffsl() and get other > constants based on it? We'd probably want to use ffsll() so we can ignore 32b vs 64b longs. There's HOST_LONG_BITS, but that doesn't actually help defining a shift value. Thanks, 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
On Tue, 2009-05-12 at 12:45 -0600, Alex Williamson wrote: > On Mon, 2009-05-11 at 12:00 +0000, Yang, Sheng wrote: > > On Friday 08 May 2009 06:22:20 Alex Williamson wrote: > > > + /* 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); > > > > 3 or 5? > > 3, ie. /8 (bits to bytes) > > > I am a little confused by these magic numbers, including 0x1f... > > The 5 shift gives us the index into the array of ints, the 0x1f gives us > the bit index into a specific int. This is very similar to the code in > hw/acpi.c. > > > I think there are something can indicate the length of unsigned long in > > QEmu(sorry, can't find it now...), so how about using ffsl() and get other > > constants based on it? > > We'd probably want to use ffsll() so we can ignore 32b vs 64b longs. > There's HOST_LONG_BITS, but that doesn't actually help defining a shift > value. Thanks, Hmm, neither ffsl() or ffsll() are standard. I'm inclined to stick with 32bits. We'll likely only be using the first index on x86_64 anyway (2nd on ia64). Thanks, 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
On Tue, May 12, 2009 at 01:06:54PM -0600, Alex Williamson wrote: > On Tue, 2009-05-12 at 12:45 -0600, Alex Williamson wrote: > > On Mon, 2009-05-11 at 12:00 +0000, Yang, Sheng wrote: > > > On Friday 08 May 2009 06:22:20 Alex Williamson wrote: > > > > + /* 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); > > > > > > 3 or 5? > > > > 3, ie. /8 (bits to bytes) > > > > > I am a little confused by these magic numbers, including 0x1f... > > > > The 5 shift gives us the index into the array of ints, the 0x1f gives us > > the bit index into a specific int. This is very similar to the code in > > hw/acpi.c. > > > > > I think there are something can indicate the length of unsigned long in > > > QEmu(sorry, can't find it now...), so how about using ffsl() and get other > > > constants based on it? > > > > We'd probably want to use ffsll() so we can ignore 32b vs 64b longs. > > There's HOST_LONG_BITS, but that doesn't actually help defining a shift > > value. Thanks, > > Hmm, neither ffsl() or ffsll() are standard. I'm inclined to stick with > 32bits. We'll likely only be using the first index on x86_64 anyway > (2nd on ia64). Thanks, > > Alex With MSI, we start with GSI 24, so we'll be using more than just the first index.
On Tue, 2009-05-12 at 22:10 +0300, Michael S. Tsirkin wrote: > On Tue, May 12, 2009 at 01:06:54PM -0600, Alex Williamson wrote: > > On Tue, 2009-05-12 at 12:45 -0600, Alex Williamson wrote: > > > On Mon, 2009-05-11 at 12:00 +0000, Yang, Sheng wrote: > > > > On Friday 08 May 2009 06:22:20 Alex Williamson wrote: > > > > > + /* 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); > > > > > > > > 3 or 5? > > > > > > 3, ie. /8 (bits to bytes) > > > > > > > I am a little confused by these magic numbers, including 0x1f... > > > > > > The 5 shift gives us the index into the array of ints, the 0x1f gives us > > > the bit index into a specific int. This is very similar to the code in > > > hw/acpi.c. > > > > > > > I think there are something can indicate the length of unsigned long in > > > > QEmu(sorry, can't find it now...), so how about using ffsl() and get other > > > > constants based on it? > > > > > > We'd probably want to use ffsll() so we can ignore 32b vs 64b longs. > > > There's HOST_LONG_BITS, but that doesn't actually help defining a shift > > > value. Thanks, > > > > Hmm, neither ffsl() or ffsll() are standard. I'm inclined to stick with > > 32bits. We'll likely only be using the first index on x86_64 anyway > > (2nd on ia64). Thanks, > > > > Alex > > With MSI, we start with GSI 24, so we'll be using more than just the > first index. Right, that leaves 7 GSIs free in the first 32 bit index for device assignment, plus 999 GSIs free in the entire table. If we're worried about the overhead of using a 4 or 8 byte index into the bitmap, maybe we should be caching GSIs for specific devices. This feels like a bit of an overkill for now though. Thanks, 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 --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..94f86e5 100644 --- a/kvm/libkvm/kvm-common.h +++ b/kvm/libkvm/kvm-common.h @@ -66,8 +66,9 @@ 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; #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..43abc7d 100644 --- a/kvm/libkvm/libkvm.c +++ b/kvm/libkvm/libkvm.c @@ -1298,8 +1298,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 +1402,72 @@ 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; + 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) + 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); + } + + return kvm_find_free_gsi(kvm); #else return -ENOSYS; #endif } + +void kvm_free_irq_route_gsi(kvm_context_t kvm, int gsi) +{ +#ifdef KVM_CAP_IRQ_ROUTING + clear_bit(kvm->used_gsi_bitmap, gsi); +#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);
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> --- Applies on top of "kvm: device-assignment: Catch GSI overflow" hw/device-assignment.c | 4 ++- kvm/libkvm/kvm-common.h | 3 +- kvm/libkvm/libkvm.c | 68 +++++++++++++++++++++++++++++++++++++++++------ kvm/libkvm/libkvm.h | 10 +++++++ 4 files changed, 74 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