| Message ID | 20090520185134.748295218@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Marcelo Tosatti wrote: > Subject says it all. > I hate those changelogs. I guess Subject never reviews code. You might put some evidence that we're suffering from contention here (I'm very willing to believe it, but hard evidence is better). > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > Index: kvm-irqlock/include/linux/kvm_host.h > =================================================================== > --- kvm-irqlock.orig/include/linux/kvm_host.h > +++ kvm-irqlock/include/linux/kvm_host.h > @@ -123,7 +123,6 @@ struct kvm_kernel_irq_routing_entry { > }; > > struct kvm { > - struct mutex lock; /* protects the vcpus array and APIC accesses */ > spinlock_t mmu_lock; > struct rw_semaphore slots_lock; > struct mm_struct *mm; /* userspace tied to this vm */ > @@ -132,6 +131,12 @@ struct kvm { > KVM_PRIVATE_MEM_SLOTS]; > struct kvm_vcpu *vcpus[KVM_MAX_VCPUS]; > struct list_head vm_list; > + struct mutex lock; /* > + * - protects mmio_bus, pio_bus. > + * - protects a few concurrent ioctl's (FIXME). > + * - protects concurrent create_vcpu, but > List data structures, not operations. > + * kvm->vcpus walkers do it locklessly (FIXME). > I think we're fine? maybe add an smp_mb() between cpu creation and assignment into the vcpu array? > + */ > struct kvm_io_bus mmio_bus; > struct kvm_io_bus pio_bus; > struct list_head irqfds; > @@ -143,6 +148,7 @@ struct kvm { > struct kvm_coalesced_mmio_ring *coalesced_mmio_ring; > #endif > > + struct mutex irq_lock; /* protects high level irq logic, ioapic */ > List the data structures protected please.
On Sun, May 24, 2009 at 05:10:07PM +0300, Avi Kivity wrote: > Marcelo Tosatti wrote: >> Subject says it all. >> > > I hate those changelogs. I guess Subject never reviews code. > > You might put some evidence that we're suffering from contention here > (I'm very willing to believe it, but hard evidence is better). Don't have any evidence yet, the main purpose of the split is to fix the deadlock. But, with the data we have so far, slots_lock and kvm->lock will cause significant cache bouncing on EPT hardware (with measurements on shadow mmu_lock is #1 offender). -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcelo Tosatti wrote: > On Sun, May 24, 2009 at 05:10:07PM +0300, Avi Kivity wrote: > >> Marcelo Tosatti wrote: >> >>> Subject says it all. >>> >>> >> I hate those changelogs. I guess Subject never reviews code. >> >> You might put some evidence that we're suffering from contention here >> (I'm very willing to believe it, but hard evidence is better). >> > > Don't have any evidence yet, the main purpose of the split is to fix the > deadlock. > You might have said that. > But, with the data we have so far, slots_lock and kvm->lock will cause > significant cache bouncing on EPT hardware (with measurements on shadow > mmu_lock is #1 offender). > Well, mmu_lock is probably contended, which is much worse than just cache bouncing.
This is to fix a deadlock reported by Alex Williamson, while at the same time makes it easier to allow PIO/MMIO regions to be registered/unregistered while a guest is alive.
Index: kvm-irqlock/include/linux/kvm_host.h =================================================================== --- kvm-irqlock.orig/include/linux/kvm_host.h +++ kvm-irqlock/include/linux/kvm_host.h @@ -123,7 +123,6 @@ struct kvm_kernel_irq_routing_entry { }; struct kvm { - struct mutex lock; /* protects the vcpus array and APIC accesses */ spinlock_t mmu_lock; struct rw_semaphore slots_lock; struct mm_struct *mm; /* userspace tied to this vm */ @@ -132,6 +131,12 @@ struct kvm { KVM_PRIVATE_MEM_SLOTS]; struct kvm_vcpu *vcpus[KVM_MAX_VCPUS]; struct list_head vm_list; + struct mutex lock; /* + * - protects mmio_bus, pio_bus. + * - protects a few concurrent ioctl's (FIXME). + * - protects concurrent create_vcpu, but + * kvm->vcpus walkers do it locklessly (FIXME). + */ struct kvm_io_bus mmio_bus; struct kvm_io_bus pio_bus; struct list_head irqfds; @@ -143,6 +148,7 @@ struct kvm { struct kvm_coalesced_mmio_ring *coalesced_mmio_ring; #endif + struct mutex irq_lock; /* protects high level irq logic, ioapic */ #ifdef CONFIG_HAVE_KVM_IRQCHIP struct list_head irq_routing; /* of kvm_kernel_irq_routing_entry */ struct hlist_head mask_notifier_list; Index: kvm-irqlock/virt/kvm/ioapic.c =================================================================== --- kvm-irqlock.orig/virt/kvm/ioapic.c +++ kvm-irqlock/virt/kvm/ioapic.c @@ -225,6 +225,10 @@ static int ioapic_in_range(struct kvm_io { struct kvm_ioapic *ioapic = (struct kvm_ioapic *)this->private; + /* + * Lockless check for ioapic->base_address on the hazy assumption + * it does not change during the lifetime of a VM. + */ return ((addr >= ioapic->base_address && (addr < ioapic->base_address + IOAPIC_MEM_LENGTH))); } @@ -238,6 +242,7 @@ static void ioapic_mmio_read(struct kvm_ ioapic_debug("addr %lx\n", (unsigned long)addr); ASSERT(!(addr & 0xf)); /* check alignment */ + mutex_lock(&ioapic->kvm->irq_lock); addr &= 0xff; switch (addr) { case IOAPIC_REG_SELECT: @@ -264,6 +269,7 @@ static void ioapic_mmio_read(struct kvm_ default: printk(KERN_WARNING "ioapic: wrong length %d\n", len); } + mutex_unlock(&ioapic->kvm->irq_lock); } static void ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len, @@ -275,6 +281,8 @@ static void ioapic_mmio_write(struct kvm ioapic_debug("ioapic_mmio_write addr=%p len=%d val=%p\n", (void*)addr, len, val); ASSERT(!(addr & 0xf)); /* check alignment */ + + mutex_lock(&ioapic->kvm->irq_lock); if (len == 4 || len == 8) data = *(u32 *) val; else { @@ -300,6 +308,7 @@ static void ioapic_mmio_write(struct kvm default: break; } + mutex_unlock(&ioapic->kvm->irq_lock); } void kvm_ioapic_reset(struct kvm_ioapic *ioapic) Index: kvm-irqlock/virt/kvm/kvm_main.c =================================================================== --- kvm-irqlock.orig/virt/kvm/kvm_main.c +++ kvm-irqlock/virt/kvm/kvm_main.c @@ -985,6 +985,7 @@ static struct kvm *kvm_create_vm(void) kvm_io_bus_init(&kvm->pio_bus); INIT_LIST_HEAD(&kvm->irqfds); mutex_init(&kvm->lock); + mutex_init(&kvm->irq_lock); kvm_io_bus_init(&kvm->mmio_bus); init_rwsem(&kvm->slots_lock); atomic_set(&kvm->users_count, 1);
Subject says it all. Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>