From mboxrd@z Thu Jan  1 00:00:00 1970
From: Alex Williamson <alex.williamson@hp.com>
Subject: Re: [PATCH v2] kvm: Use a bitmap for tracking used GSIs
Date: Tue, 12 May 2009 15:56:22 -0600
Message-ID: <1242165382.4788.12.camel@2710p.home>
References: <20090507222015.5216.18027.stgit@dl380g6-3.ned.telco.ned.telco>
	 <20090508222925.5119.94814.stgit@dl380g6-3.ned.telco.ned.telco>
	 <20090512193914.GB28935@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org, sheng@linux.intel.com
To: "Michael S. Tsirkin" <mst@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from g5t0006.atlanta.hp.com ([15.192.0.43]:2702 "EHLO
	g5t0006.atlanta.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752284AbZELV42 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 12 May 2009 17:56:28 -0400
In-Reply-To: <20090512193914.GB28935@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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