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

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Alex Williamson <alex.williamson@hp.com>
Cc: kvm@vger.kernel.org, sheng@linux.intel.com
Subject: Re: [PATCH v2] kvm: Use a bitmap for tracking used GSIs
Date: Tue, 12 May 2009 22:39:14 +0300	[thread overview]
Message-ID: <20090512193914.GB28935@redhat.com> (raw)
In-Reply-To: <20090508222925.5119.94814.stgit@dl380g6-3.ned.telco.ned.telco>

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