From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH v2] kvm: Use a bitmap for tracking used GSIs Date: Tue, 12 May 2009 22:39:14 +0300 Message-ID: <20090512193914.GB28935@redhat.com> References: <20090507222015.5216.18027.stgit@dl380g6-3.ned.telco.ned.telco> <20090508222925.5119.94814.stgit@dl380g6-3.ned.telco.ned.telco> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, sheng@linux.intel.com To: Alex Williamson Return-path: Received: from mx2.redhat.com ([66.187.237.31]:54883 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751923AbZELTkT (ORCPT ); Tue, 12 May 2009 15:40:19 -0400 Content-Disposition: inline In-Reply-To: <20090508222925.5119.94814.stgit@dl380g6-3.ned.telco.ned.telco> Sender: kvm-owner@vger.kernel.org List-ID: 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); > -- MST