Re: [RFC v2 2/9] KVM: ARM: VGIC: add forwarded irq rbtree lock

[Christoffer Dall]

On Mon, Sep 01, 2014 at 02:52:41PM +0200, Eric Auger wrote:
> add a lock related to the rb tree manipulation. The rb tree can be

Ok, I can't hold myself back any longer.  Please begin sentences with a
capital letter. You don't do this in French? :)

> searched in one thread (irqfd handler for instance) and map/unmap
> happen in another.
> 
> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> ---
>  include/kvm/arm_vgic.h |  1 +
>  virt/kvm/arm/vgic.c    | 46 +++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 743020f..3da244f 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -177,6 +177,7 @@ struct vgic_dist {
>  	unsigned long		irq_pending_on_cpu;
>  
>  	struct rb_root		irq_phys_map;
> +	spinlock_t			rb_tree_lock;
>  #endif
>  };
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 8ef495b..dbc2a5a 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1630,9 +1630,15 @@ static struct rb_root *vgic_get_irq_phys_map(struct kvm_vcpu *vcpu,
>  
>  int vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
>  {
> -	struct rb_root *root = vgic_get_irq_phys_map(vcpu, virt_irq);
> -	struct rb_node **new = &root->rb_node, *parent = NULL;
> +	struct rb_root *root;
> +	struct rb_node **new, *parent = NULL;
>  	struct irq_phys_map *new_map;
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> +	spin_lock(&dist->rb_tree_lock);
> +
> +	root = vgic_get_irq_phys_map(vcpu, virt_irq);
> +	new = &root->rb_node;
>  
>  	/* Boilerplate rb_tree code */
>  	while (*new) {
> @@ -1644,13 +1650,17 @@ int vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
>  			new = &(*new)->rb_left;
>  		else if (this->virt_irq > virt_irq)
>  			new = &(*new)->rb_right;
> -		else
> +		else {
> +			spin_unlock(&dist->rb_tree_lock);
>  			return -EEXIST;
> +		}

can you initialize a ret variable to -EEXIST in the beginning of this
function, and add an out label above the unlock below, replace this
multi-line statement with a goto out, and set ret = 0 after the while
loop?

>  	}
>  
>  	new_map = kzalloc(sizeof(*new_map), GFP_KERNEL);
> -	if (!new_map)
> +	if (!new_map) {
> +		spin_unlock(&dist->rb_tree_lock);
>  		return -ENOMEM;

then this becomes ret = -ENOMEM; goto out;

> +	}
>  
>  	new_map->virt_irq = virt_irq;
>  	new_map->phys_irq = phys_irq;
> @@ -1658,6 +1668,8 @@ int vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
>  	rb_link_node(&new_map->node, parent, new);
>  	rb_insert_color(&new_map->node, root);
>  
> +	spin_unlock(&dist->rb_tree_lock);
> +

aren't you allocating memory with GFP_KERNEL while holding a spinlock
here?

>  	return 0;
>  }
>  
> @@ -1685,24 +1697,39 @@ static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
>  
>  int vgic_get_phys_irq(struct kvm_vcpu *vcpu, int virt_irq)
>  {
> -	struct irq_phys_map *map = vgic_irq_map_search(vcpu, virt_irq);
> +	struct irq_phys_map *map;
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +	int ret;
> +
> +	spin_lock(&dist->rb_tree_lock);
> +	map = vgic_irq_map_search(vcpu, virt_irq);
>  
>  	if (map)
> -		return map->phys_irq;
> +		ret = map->phys_irq;
> +	else
> +		ret =  -ENOENT;

initialize ret to -ENOENT and avoid the else statement.

> +
> +	spin_unlock(&dist->rb_tree_lock);
> +	return ret;
>  
> -	return -ENOENT;
>  }
>  
>  int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
>  {
> -	struct irq_phys_map *map = vgic_irq_map_search(vcpu, virt_irq);
> +	struct irq_phys_map *map;
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> +	spin_lock(&dist->rb_tree_lock);
> +
> +	map = vgic_irq_map_search(vcpu, virt_irq);
>  
>  	if (map && map->phys_irq == phys_irq) {
>  		rb_erase(&map->node, vgic_get_irq_phys_map(vcpu, virt_irq));
>  		kfree(map);
> +		spin_unlock(&dist->rb_tree_lock);

can kfree sleep?  I don't remember.  In any case, you can unlock before
calling kfree.

>  		return 0;
>  	}
> -
> +	spin_unlock(&dist->rb_tree_lock);
>  	return -ENOENT;

an out label and single unlock location would be preferred here as well
I think.

>  }
>  
> @@ -1898,6 +1925,7 @@ int kvm_vgic_create(struct kvm *kvm)
>  	}
>  
>  	spin_lock_init(&kvm->arch.vgic.lock);
> +	spin_lock_init(&kvm->arch.vgic.rb_tree_lock);
>  	kvm->arch.vgic.in_kernel = true;
>  	kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
>  	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/