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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux