Re: [PATCH RFC V11 15/18] kvm : Paravirtual ticketlocks support for linux guests running on KVM hypervisor
On Tue, Jul 30, 2013 at 10:13:12PM +0530, Raghavendra K T wrote: > On 07/25/2013 03:08 PM, Raghavendra K T wrote: > >On 07/25/2013 02:45 PM, Gleb Natapov wrote: > >>On Thu, Jul 25, 2013 at 02:47:37PM +0530, Raghavendra K T wrote: > >>>On 07/24/2013 06:06 PM, Raghavendra K T wrote: > On 07/24/2013 05:36 PM, Gleb Natapov wrote: > >On Wed, Jul 24, 2013 at 05:30:20PM +0530, Raghavendra K T wrote: > >>On 07/24/2013 04:09 PM, Gleb Natapov wrote: > >>>On Wed, Jul 24, 2013 at 03:15:50PM +0530, Raghavendra K T wrote: > On 07/23/2013 08:37 PM, Gleb Natapov wrote: > >On Mon, Jul 22, 2013 at 11:50:16AM +0530, Raghavendra K T wrote: > >>+static void kvm_lock_spinning(struct arch_spinlock *lock, > >>__ticket_t want) > [...] > >>+ > >>+/* > >>+ * halt until it's our turn and kicked. Note that we do safe > >>halt > >>+ * for irq enabled case to avoid hang when lock info is > >>overwritten > >>+ * in irq spinlock slowpath and no spurious interrupt occur > >>to save us. > >>+ */ > >>+if (arch_irqs_disabled_flags(flags)) > >>+halt(); > >>+else > >>+safe_halt(); > >>+ > >>+out: > >So here now interrupts can be either disabled or enabled. Previous > >version disabled interrupts here, so are we sure it is safe to > >have them > >enabled at this point? I do not see any problem yet, will keep > >thinking. > > If we enable interrupt here, then > > > >>+cpumask_clear_cpu(cpu, &waiting_cpus); > > and if we start serving lock for an interrupt that came here, > cpumask clear and w->lock=null may not happen atomically. > if irq spinlock does not take slow path we would have non null > value > for lock, but with no information in waitingcpu. > > I am still thinking what would be problem with that. > > >>>Exactly, for kicker waiting_cpus and w->lock updates are > >>>non atomic anyway. > >>> > >>+w->lock = NULL; > >>+local_irq_restore(flags); > >>+spin_time_accum_blocked(start); > >>+} > >>+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning); > >>+ > >>+/* Kick vcpu waiting on @lock->head to reach value @ticket */ > >>+static void kvm_unlock_kick(struct arch_spinlock *lock, > >>__ticket_t ticket) > >>+{ > >>+int cpu; > >>+ > >>+add_stats(RELEASED_SLOW, 1); > >>+for_each_cpu(cpu, &waiting_cpus) { > >>+const struct kvm_lock_waiting *w = > >>&per_cpu(lock_waiting, cpu); > >>+if (ACCESS_ONCE(w->lock) == lock && > >>+ACCESS_ONCE(w->want) == ticket) { > >>+add_stats(RELEASED_SLOW_KICKED, 1); > >>+kvm_kick_cpu(cpu); > >What about using NMI to wake sleepers? I think it was > >discussed, but > >forgot why it was dismissed. > > I think I have missed that discussion. 'll go back and check. so > what is the idea here? we can easily wake up the halted vcpus that > have interrupt disabled? > >>>We can of course. IIRC the objection was that NMI handling path > >>>is very > >>>fragile and handling NMI on each wakeup will be more expensive then > >>>waking up a guest without injecting an event, but it is still > >>>interesting > >>>to see the numbers. > >>> > >> > >>Haam, now I remember, We had tried request based mechanism. (new > >>request like REQ_UNHALT) and process that. It had worked, but had > >>some > >>complex hacks in vcpu_enter_guest to avoid guest hang in case of > >>request cleared. So had left it there.. > >> > >>https://lkml.org/lkml/2012/4/30/67 > >> > >>But I do not remember performance impact though. > >No, this is something different. Wakeup with NMI does not need KVM > >changes at > >all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI. > > > > True. It was not NMI. > just to confirm, are you talking about something like this to be > tried ? > > apic->send_IPI_mask(cpumask_of(cpu), APIC_DM_NMI); > >>> > >>>When I started benchmark, I started seeing > >>>"Dazed and confused, but trying to continue" from unknown nmi error > >>>handling. > >>>Did I miss anything (because we did not register any NMI handler)? or > >>>is it that spurious NMIs are trouble because we could get spurious NMIs > >>>if next waiter already acquired the lock. > >>There is a default NMI handler that tries to detect the reason why NMI > >>happened (which is no so easy on x86) and prints this message if it > >>fails. You need to add logic to detect spinlock slow path there. C
Re: Linux Plumbers ACPI/PM, PCI Microconference
On Wed, 2013-07-31 at 00:02 +, Shuah Khan wrote: > On 07/30/2013 05:38 PM, Rafael J. Wysocki wrote: > > On Wednesday, July 17, 2013 08:31:55 AM Shuah Khan wrote: > >> Myron, > >> > >> On Tue, Jul 16, 2013 at 8:21 PM, Myron Stowe wrote: > >> > >>> > >>> Shuah - You brought up the idea about "Converting drivers from Legacy > >>> PM ops to dev_pm_ops"; would you like to present what you have > >>> done/encountered so far? > >>> > >> > >> Awesome. Yes, I would like to present what I have done so far and I do > >> have a couple of things that could benefit from a face to face > >> discussion which would help me make progress on the rest of the work > >> that needs to get done. > > > > Care to sumbit a formal proposal through the LPC web page? > > > > Rafael > > > > > > Rafael, > > I did submit a formal talk proposal to LinuxCon/LPC and it was rejected. > Submission is closed now as far as I know. Microconference topics should be submitted here: http://www.linuxplumbersconf.org/2013/ocw/events/LPC2013/proposals/new ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Linux Plumbers ACPI/PM, PCI Microconference
On Wednesday, July 17, 2013 08:31:55 AM Shuah Khan wrote: > Myron, > > On Tue, Jul 16, 2013 at 8:21 PM, Myron Stowe wrote: > > > > > Shuah - You brought up the idea about "Converting drivers from Legacy > > PM ops to dev_pm_ops"; would you like to present what you have > > done/encountered so far? > > > > Awesome. Yes, I would like to present what I have done so far and I do > have a couple of things that could benefit from a face to face > discussion which would help me make progress on the rest of the work > that needs to get done. Care to sumbit a formal proposal through the LPC web page? Rafael ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC V11 15/18] kvm : Paravirtual ticketlocks support for linux guests running on KVM hypervisor
On 07/25/2013 03:08 PM, Raghavendra K T wrote: On 07/25/2013 02:45 PM, Gleb Natapov wrote: On Thu, Jul 25, 2013 at 02:47:37PM +0530, Raghavendra K T wrote: On 07/24/2013 06:06 PM, Raghavendra K T wrote: On 07/24/2013 05:36 PM, Gleb Natapov wrote: On Wed, Jul 24, 2013 at 05:30:20PM +0530, Raghavendra K T wrote: On 07/24/2013 04:09 PM, Gleb Natapov wrote: On Wed, Jul 24, 2013 at 03:15:50PM +0530, Raghavendra K T wrote: On 07/23/2013 08:37 PM, Gleb Natapov wrote: On Mon, Jul 22, 2013 at 11:50:16AM +0530, Raghavendra K T wrote: +static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want) [...] + +/* + * halt until it's our turn and kicked. Note that we do safe halt + * for irq enabled case to avoid hang when lock info is overwritten + * in irq spinlock slowpath and no spurious interrupt occur to save us. + */ +if (arch_irqs_disabled_flags(flags)) +halt(); +else +safe_halt(); + +out: So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them enabled at this point? I do not see any problem yet, will keep thinking. If we enable interrupt here, then +cpumask_clear_cpu(cpu, &waiting_cpus); and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value for lock, but with no information in waitingcpu. I am still thinking what would be problem with that. Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway. +w->lock = NULL; +local_irq_restore(flags); +spin_time_accum_blocked(start); +} +PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning); + +/* Kick vcpu waiting on @lock->head to reach value @ticket */ +static void kvm_unlock_kick(struct arch_spinlock *lock, __ticket_t ticket) +{ +int cpu; + +add_stats(RELEASED_SLOW, 1); +for_each_cpu(cpu, &waiting_cpus) { +const struct kvm_lock_waiting *w = &per_cpu(lock_waiting, cpu); +if (ACCESS_ONCE(w->lock) == lock && +ACCESS_ONCE(w->want) == ticket) { +add_stats(RELEASED_SLOW_KICKED, 1); +kvm_kick_cpu(cpu); What about using NMI to wake sleepers? I think it was discussed, but forgot why it was dismissed. I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled? We can of course. IIRC the objection was that NMI handling path is very fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting to see the numbers. Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there.. https://lkml.org/lkml/2012/4/30/67 But I do not remember performance impact though. No, this is something different. Wakeup with NMI does not need KVM changes at all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI. True. It was not NMI. just to confirm, are you talking about something like this to be tried ? apic->send_IPI_mask(cpumask_of(cpu), APIC_DM_NMI); When I started benchmark, I started seeing "Dazed and confused, but trying to continue" from unknown nmi error handling. Did I miss anything (because we did not register any NMI handler)? or is it that spurious NMIs are trouble because we could get spurious NMIs if next waiter already acquired the lock. There is a default NMI handler that tries to detect the reason why NMI happened (which is no so easy on x86) and prints this message if it fails. You need to add logic to detect spinlock slow path there. Check bit in waiting_cpus for instance. aha.. Okay. will check that. yes. Thanks.. that did the trick. I did like below in unknown_nmi_error(): if (cpumask_test_cpu(smp_processor_id(), &waiting_cpus)) return; But I believe you asked NMI method only for experimental purpose to check the upperbound. because as I doubted above, for spurious NMI (i.e. when unlocker kicks when waiter already got the lock), we would still hit unknown NMI error. I had hit spurious NMI over 1656 times over entire benchmark run. along with INFO: NMI handler (arch_trigger_all_cpu_backtrace_handler) took too long to run: 24.886 msecs etc... (and we cannot get away with that too because it means we bypass the unknown NMI error even in genuine cases too) Here was the result for the my dbench test( 32 core machine with 32 vcpu guest HT off) -- % improvement -- pvspinlock pvspin_ipi pvpsin_nmi dbench_1x 0.9016 0.7442 0.7522 dbench_2x 14.7513 18.0164 15.9421 dbench_3x 14.7571 17.0793
Re: [PATCH] kexec/kdump implementation for Xen PV domU
On Mon, Jul 29, 2013 at 02:44:19PM -0700, Matt Wilson wrote: > On Mon, Jul 29, 2013 at 07:15:43PM +0200, Daniel Kiper wrote: > > Hi all, > > > > Here I am sending as attachments patches enabling kexec/kdump > > support in Xen PV domU. Only x84_64 architecture is supported. > > There is no support for i386 but some code could be easily reused. > > Here is a description of patches: > > [...] > > > - kexec-kernel-only_20121203.patch: this patch fixes timer > > issue on Amazon EC2 machines. > > Hi Daniel, > > Do you know the cause of this issue? Does it have something to do with > singleshot timer migration when offlining/onlining SMP CPUs? Sadly, no. I was not able to replicate this on my machines (I did test on Xen 4.1). However, as I saw this issue appears on Xen 3.4 and 4.0 (IIRC version numbers used on your machines). Additionally, it does not depend on CPU models. And it appears quite often but not always. Maybe it is linked with singleshot timer migration. Daniel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 1/9] virtio: console: fix race with port unplug and open/close
On (Mon) 29 Jul 2013 [14:18:52], Rusty Russell wrote: > Amit Shah writes: > > There's a window between find_port_by_devt() returning a port and us > > taking a kref on the port, where the port could get unplugged. Fix it > > by taking the reference in find_port_by_devt() itself. > > > > Problem reported and analyzed by Mateusz Guzik. > > This fix is clearly correct, but what about the other find_port_by_* > functions? They don't need a kref -- the kref is only to be bumped when: 1. Initialising / Plugging in the port (add_port) 2. Opening the port (this fix) Both these cases are now covered. As part of the locking rework, the other find_port_by_* functions may be reworked to set some state, like port_in_use, instead of abusing guest_connected today. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization