From: Eric Auger <eric.auger@linaro.org> To: Christoffer Dall <christoffer.dall@linaro.org> Cc: eric.auger@st.com, marc.zyngier@arm.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, alex.williamson@redhat.com, joel.schopp@amd.com, kim.phillips@freescale.com, paulus@samba.org, gleb@kernel.org, pbonzini@redhat.com, linux-kernel@vger.kernel.org, patches@linaro.org, will.deacon@arm.com, a.motakis@virtualopensystems.com, a.rigo@virtualopensystems.com, john.liuli@huawei.com Subject: Re: [RFC v2 2/9] KVM: ARM: VGIC: add forwarded irq rbtree lock Date: Thu, 11 Sep 2014 19:31:28 +0200 [thread overview] Message-ID: <5411DC70.5060104@linaro.org> (raw) In-Reply-To: <20140911030953.GD2784@lvm> On 09/11/2014 05:09 AM, Christoffer Dall wrote: > 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? :) Hi Christoffer, yep that's understood ;-) Definitively we do. Just that I am discovering it is common too in commits and comments ;-) > >> searched in one thread (irqfd handler for instance) and map/unmap >> happen in another. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> --- >> 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? sure > >> } >> >> 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; OK > >> + } >> >> 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? oups. Thanks for noticing. I Will move the lock. > >> 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. ok > >> + >> + 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. no it can't but I will move anyway. > >> 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. ok Thansk Eric > >> } >> >> @@ -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 >>
WARNING: multiple messages have this Message-ID (diff)
From: eric.auger@linaro.org (Eric Auger) To: linux-arm-kernel@lists.infradead.org Subject: [RFC v2 2/9] KVM: ARM: VGIC: add forwarded irq rbtree lock Date: Thu, 11 Sep 2014 19:31:28 +0200 [thread overview] Message-ID: <5411DC70.5060104@linaro.org> (raw) In-Reply-To: <20140911030953.GD2784@lvm> On 09/11/2014 05:09 AM, Christoffer Dall wrote: > 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? :) Hi Christoffer, yep that's understood ;-) Definitively we do. Just that I am discovering it is common too in commits and comments ;-) > >> searched in one thread (irqfd handler for instance) and map/unmap >> happen in another. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> --- >> 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? sure > >> } >> >> 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; OK > >> + } >> >> 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? oups. Thanks for noticing. I Will move the lock. > >> 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. ok > >> + >> + 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. no it can't but I will move anyway. > >> 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. ok Thansk Eric > >> } >> >> @@ -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 >>
next prev parent reply other threads:[~2014-09-11 17:31 UTC|newest] Thread overview: 101+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-09-01 12:52 [RFC v2 0/9] KVM-VFIO IRQ forward control Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-01 12:52 ` [RFC v2 1/9] KVM: ARM: VGIC: fix multiple injection of level sensitive forwarded IRQ Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-11 3:09 ` Christoffer Dall 2014-09-11 3:09 ` Christoffer Dall 2014-09-11 18:17 ` Eric Auger 2014-09-11 18:17 ` Eric Auger 2014-09-11 22:14 ` Christoffer Dall 2014-09-11 22:14 ` Christoffer Dall 2014-09-01 12:52 ` [RFC v2 2/9] KVM: ARM: VGIC: add forwarded irq rbtree lock Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-11 3:09 ` Christoffer Dall 2014-09-11 3:09 ` Christoffer Dall 2014-09-11 17:31 ` Eric Auger [this message] 2014-09-11 17:31 ` Eric Auger 2014-09-01 12:52 ` [RFC v2 3/9] ARM: KVM: Enable the KVM-VFIO device Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-01 12:52 ` [RFC v2 4/9] VFIO: platform: handler tests whether the IRQ is forwarded Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 8:44 ` Eric Auger 2014-09-11 8:44 ` Eric Auger 2014-09-11 17:05 ` Christoffer Dall 2014-09-11 17:05 ` Christoffer Dall 2014-09-11 18:07 ` Alex Williamson 2014-09-11 18:07 ` Alex Williamson 2014-09-11 17:08 ` Antonios Motakis 2014-09-11 17:08 ` Antonios Motakis 2014-09-01 12:52 ` [RFC v2 5/9] KVM: KVM-VFIO: update user API to program forwarded IRQ Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 8:49 ` Eric Auger 2014-09-11 8:49 ` Eric Auger 2014-09-11 17:08 ` Christoffer Dall 2014-09-11 17:08 ` Christoffer Dall 2014-09-01 12:52 ` [RFC v2 6/9] VFIO: Extend external user API Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 8:50 ` Eric Auger 2014-09-11 8:50 ` Eric Auger 2014-09-01 12:52 ` [RFC v2 7/9] KVM: KVM-VFIO: add new VFIO external API hooks Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 8:51 ` Eric Auger 2014-09-11 8:51 ` Eric Auger 2014-09-01 12:52 ` [RFC v2 8/9] KVM: KVM-VFIO: generic KVM_DEV_VFIO_DEVICE command and IRQ forwarding control Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 5:05 ` Alex Williamson 2014-09-11 5:05 ` Alex Williamson 2014-09-11 5:05 ` Alex Williamson 2014-09-11 12:04 ` Eric Auger 2014-09-11 12:04 ` Eric Auger 2014-09-11 15:59 ` Alex Williamson 2014-09-11 15:59 ` Alex Williamson 2014-09-11 17:24 ` Christoffer Dall 2014-09-11 17:24 ` Christoffer Dall 2014-09-11 17:22 ` Christoffer Dall 2014-09-11 17:22 ` Christoffer Dall 2014-09-11 17:10 ` Christoffer Dall 2014-09-11 17:10 ` Christoffer Dall 2014-09-11 18:14 ` Alex Williamson 2014-09-11 18:14 ` Alex Williamson 2014-09-11 21:59 ` Christoffer Dall 2014-09-11 21:59 ` Christoffer Dall 2014-09-11 9:35 ` Eric Auger 2014-09-11 9:35 ` Eric Auger 2014-09-11 15:47 ` Alex Williamson 2014-09-11 15:47 ` Alex Williamson 2014-09-11 15:47 ` Alex Williamson 2014-09-11 17:32 ` Christoffer Dall 2014-09-11 17:32 ` Christoffer Dall 2014-09-01 12:52 ` [RFC v2 9/9] KVM: KVM-VFIO: ARM " Eric Auger 2014-09-01 12:52 ` Eric Auger 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 3:10 ` Christoffer Dall 2014-09-02 21:05 ` [RFC v2 0/9] KVM-VFIO IRQ forward control Alex Williamson 2014-09-02 21:05 ` Alex Williamson 2014-09-05 12:52 ` Eric Auger 2014-09-05 12:52 ` Eric Auger 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 3:10 ` Christoffer Dall 2014-09-11 5:09 ` Alex Williamson 2014-09-11 5:09 ` Alex Williamson 2014-11-17 11:25 ` Wu, Feng 2014-11-17 11:25 ` Wu, Feng 2014-11-17 11:25 ` Wu, Feng 2014-11-17 13:41 ` Eric Auger 2014-11-17 13:41 ` Eric Auger 2014-11-17 13:41 ` Eric Auger 2014-11-17 13:45 ` Wu, Feng 2014-11-17 13:45 ` Wu, Feng 2014-11-17 13:45 ` Wu, Feng
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5411DC70.5060104@linaro.org \ --to=eric.auger@linaro.org \ --cc=a.motakis@virtualopensystems.com \ --cc=a.rigo@virtualopensystems.com \ --cc=alex.williamson@redhat.com \ --cc=christoffer.dall@linaro.org \ --cc=eric.auger@st.com \ --cc=gleb@kernel.org \ --cc=joel.schopp@amd.com \ --cc=john.liuli@huawei.com \ --cc=kim.phillips@freescale.com \ --cc=kvm@vger.kernel.org \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=marc.zyngier@arm.com \ --cc=patches@linaro.org \ --cc=paulus@samba.org \ --cc=pbonzini@redhat.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.