Gleb Natapov wrote:
> Signed-off-by: Gleb Natapov <gleb@xxxxxxxxxx>
> ---
> include/linux/kvm_host.h | 2 +-
> virt/kvm/irq_comm.c | 55 +++++++++++++++++++++-------------------------
> virt/kvm/kvm_main.c | 1 -
> 3 files changed, 26 insertions(+), 32 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f54a0d3..6756b3e 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -161,7 +161,7 @@ struct kvm {
>
> struct mutex irq_lock;
> #ifdef CONFIG_HAVE_KVM_IRQCHIP
> - struct list_head irq_routing; /* of kvm_kernel_irq_routing_entry */
> + struct kvm_kernel_irq_routing_entry *irq_routing;
> struct hlist_head mask_notifier_list;
> #endif
>
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index 7af18b8..b2fa3f6 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -148,7 +148,8 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
> * IOAPIC. So set the bit in both. The guest will ignore
> * writes to the unused one.
> */
> - list_for_each_entry(e, &kvm->irq_routing, link)
> + rcu_read_lock();
> + for (e = rcu_dereference(kvm->irq_routing); e && e->set; e++) {
>
Hi Gleb,
I haven't had a chance to fully digest and review these patches, but
one thing I did notice is that you seem to be converting from a list to
an open-coded structure. I am just curious why you made this design
decision instead of using the RCU variant of list?
Regards,
-Greg
> if (e->gsi == irq) {
> int r = e->set(e, kvm, sig_level);
> if (r < 0)
> @@ -156,6 +157,8 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
>
> ret = r + ((ret < 0) ? 0 : ret);
> }
> + }
> + rcu_read_unlock();
> return ret;
> }
>
> @@ -168,12 +171,15 @@ void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin)
>
> trace_kvm_ack_irq(irqchip, pin);
>
> - list_for_each_entry(e, &kvm->irq_routing, link)
> + rcu_read_lock();
> + for (e = rcu_dereference(kvm->irq_routing); e && e->set; e++) {
> if (e->irqchip.irqchip == irqchip &&
> e->irqchip.pin == pin) {
> gsi = e->gsi;
> break;
> }
> + }
> + rcu_read_unlock();
>
> hlist_for_each_entry(kian, n, &kvm->arch.irq_ack_notifier_list, link)
> if (kian->gsi == gsi)
> @@ -264,19 +270,11 @@ void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask)
> kimn->func(kimn, mask);
> }
>
> -static void __kvm_free_irq_routing(struct list_head *irq_routing)
> -{
> - struct kvm_kernel_irq_routing_entry *e, *n;
> -
> - list_for_each_entry_safe(e, n, irq_routing, link)
> - kfree(e);
> -}
> -
> void kvm_free_irq_routing(struct kvm *kvm)
> {
> - mutex_lock(&kvm->irq_lock);
> - __kvm_free_irq_routing(&kvm->irq_routing);
> - mutex_unlock(&kvm->irq_lock);
> + /* Called only during vm destruction. Nobody can use the pointer
> + at this stage */
> + kfree(kvm->irq_routing);
> }
>
> static int setup_routing_entry(struct kvm_kernel_irq_routing_entry *e,
> @@ -326,43 +324,40 @@ int kvm_set_irq_routing(struct kvm *kvm,
> unsigned nr,
> unsigned flags)
> {
> - struct list_head irq_list = LIST_HEAD_INIT(irq_list);
> - struct list_head tmp = LIST_HEAD_INIT(tmp);
> - struct kvm_kernel_irq_routing_entry *e = NULL;
> + struct kvm_kernel_irq_routing_entry *new, *old;
> unsigned i;
> int r;
>
> + /* last element is left zeroed and indicates the end of the array */
> + new = kzalloc(sizeof(*new) * (nr + 1), GFP_KERNEL);
> +
> + if (!new)
> + return -ENOMEM;
> +
> for (i = 0; i < nr; ++i) {
> r = -EINVAL;
> if (ue->gsi >= KVM_MAX_IRQ_ROUTES)
> goto out;
> if (ue->flags)
> goto out;
> - r = -ENOMEM;
> - e = kzalloc(sizeof(*e), GFP_KERNEL);
> - if (!e)
> - goto out;
> - r = setup_routing_entry(e, ue);
> + r = setup_routing_entry(new + i, ue);
> if (r)
> goto out;
> ++ue;
> - list_add(&e->link, &irq_list);
> - e = NULL;
> }
>
> mutex_lock(&kvm->irq_lock);
> - list_splice(&kvm->irq_routing, &tmp);
> - INIT_LIST_HEAD(&kvm->irq_routing);
> - list_splice(&irq_list, &kvm->irq_routing);
> - INIT_LIST_HEAD(&irq_list);
> - list_splice(&tmp, &irq_list);
> + old = kvm->irq_routing;
> + rcu_assign_pointer(kvm->irq_routing, new);
> mutex_unlock(&kvm->irq_lock);
>
> + synchronize_rcu();
> +
> r = 0;
> + new = old;
>
> out:
> - kfree(e);
> - __kvm_free_irq_routing(&irq_list);
> + kfree(new);
> return r;
> }
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index cf20dc1..24013b4 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -945,7 +945,6 @@ static struct kvm *kvm_create_vm(void)
> if (IS_ERR(kvm))
> goto out;
> #ifdef CONFIG_HAVE_KVM_IRQCHIP
> - INIT_LIST_HEAD(&kvm->irq_routing);
> INIT_HLIST_HEAD(&kvm->mask_notifier_list);
> #endif
>
>
Attachment:
signature.asc
Description: OpenPGP digital signature
