Re: [Qemu-devel] [PATCH] kvm: First step to push iothread lock out of inner run loop

[Anthony Liguori <anthony@codemonkey.ws>]

On 06/26/2012 02:34 PM, Marcelo Tosatti wrote:
> On Sat, Jun 23, 2012 at 12:55:49AM +0200, Jan Kiszka wrote:
>> Should have declared this [RFC] in the subject and CC'ed kvm...
>>
>> On 2012-06-23 00:45, Jan Kiszka wrote:
>>> This sketches a possible path to get rid of the iothread lock on vmexits
>>> in KVM mode. On x86, the the in-kernel irqchips has to be used because
>>> we otherwise need to synchronize APIC and other per-cpu state accesses
>>> that could be changed concurrently. Not yet fully analyzed is the NMI
>>> injection path in the absence of an APIC.
>>>
>>> s390x should be fine without specific locking as their pre/post-run
>>> callbacks are empty. Power requires locking for the pre-run callback.
>>>
>>> This patch is untested, but a similar version was successfully used in
>>> a x86 setup with a network I/O path that needed no central iothread
>>> locking anymore (required special MMIO exit handling).
>>> ---
>>>   kvm-all.c         |   18 ++++++++++++++++--
>>>   target-i386/kvm.c |    7 +++++++
>>>   target-ppc/kvm.c  |    4 ++++
>>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index f8e4328..9c3e26f 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -1460,6 +1460,8 @@ int kvm_cpu_exec(CPUArchState *env)
>>>           return EXCP_HLT;
>>>       }
>>>
>>> +    qemu_mutex_unlock_iothread();
>>> +
>>>       do {
>>>           if (env->kvm_vcpu_dirty) {
>>>               kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
>>> @@ -1476,14 +1478,16 @@ int kvm_cpu_exec(CPUArchState *env)
>>>                */
>>>               qemu_cpu_kick_self();
>>>           }
>>> -        qemu_mutex_unlock_iothread();
>>>
>>>           run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
>>>
>>> -        qemu_mutex_lock_iothread();
>>>           kvm_arch_post_run(env, run);
>>>
>>> +        /* TODO: push coalesced mmio flushing to the point where we access
>>> +         * devices that are using it (currently VGA and E1000). */
>>> +        qemu_mutex_lock_iothread();
>>>           kvm_flush_coalesced_mmio_buffer();
>>> +        qemu_mutex_unlock_iothread();
>>>
>>>           if (run_ret<  0) {
>>>               if (run_ret == -EINTR || run_ret == -EAGAIN) {
>>> @@ -1499,19 +1503,23 @@ int kvm_cpu_exec(CPUArchState *env)
>>>           switch (run->exit_reason) {
>>>           case KVM_EXIT_IO:
>>>               DPRINTF("handle_io\n");
>>> +            qemu_mutex_lock_iothread();
>>>               kvm_handle_io(run->io.port,
>>>                             (uint8_t *)run + run->io.data_offset,
>>>                             run->io.direction,
>>>                             run->io.size,
>>>                             run->io.count);
>>> +            qemu_mutex_unlock_iothread();
>>>               ret = 0;
>>>               break;
>>>           case KVM_EXIT_MMIO:
>>>               DPRINTF("handle_mmio\n");
>>> +            qemu_mutex_lock_iothread();
>>>               cpu_physical_memory_rw(run->mmio.phys_addr,
>>>                                      run->mmio.data,
>>>                                      run->mmio.len,
>>>                                      run->mmio.is_write);
>>> +            qemu_mutex_unlock_iothread();
>>>               ret = 0;
>>>               break;
>>>           case KVM_EXIT_IRQ_WINDOW_OPEN:
>>> @@ -1520,7 +1528,9 @@ int kvm_cpu_exec(CPUArchState *env)
>>>               break;
>>>           case KVM_EXIT_SHUTDOWN:
>>>               DPRINTF("shutdown\n");
>>> +            qemu_mutex_lock_iothread();
>>>               qemu_system_reset_request();
>>> +            qemu_mutex_unlock_iothread();
>>>               ret = EXCP_INTERRUPT;
>>>               break;
>>>           case KVM_EXIT_UNKNOWN:
>>> @@ -1533,11 +1543,15 @@ int kvm_cpu_exec(CPUArchState *env)
>>>               break;
>>>           default:
>>>               DPRINTF("kvm_arch_handle_exit\n");
>>> +            qemu_mutex_lock_iothread();
>>>               ret = kvm_arch_handle_exit(env, run);
>>> +            qemu_mutex_unlock_iothread();
>>>               break;
>>>           }
>>>       } while (ret == 0);
>>>
>>> +    qemu_mutex_lock_iothread();
>>> +
>>>       if (ret<  0) {
>>>           cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
>>>           vm_stop(RUN_STATE_INTERNAL_ERROR);
>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>> index 0d0d8f6..0ad64d1 100644
>>> --- a/target-i386/kvm.c
>>> +++ b/target-i386/kvm.c
>>> @@ -1631,7 +1631,10 @@ void kvm_arch_pre_run(CPUX86State *env, struct kvm_run *run)
>>>
>>>       /* Inject NMI */
>>>       if (env->interrupt_request&  CPU_INTERRUPT_NMI) {
>>> +        qemu_mutex_lock_iothread();
>>>           env->interrupt_request&= ~CPU_INTERRUPT_NMI;
>>> +        qemu_mutex_unlock_iothread();
>>> +
>>>           DPRINTF("injected NMI\n");
>>>           ret = kvm_vcpu_ioctl(env, KVM_NMI);
>>>           if (ret<  0) {
>>> @@ -1641,6 +1644,8 @@ void kvm_arch_pre_run(CPUX86State *env, struct kvm_run *run)
>>>       }
>>>
>>>       if (!kvm_irqchip_in_kernel()) {
>>> +        qemu_mutex_lock_iothread();
>>> +
>>>           /* Force the VCPU out of its inner loop to process any INIT requests
>>>            * or pending TPR access reports. */
>>>           if (env->interrupt_request&
>>> @@ -1682,6 +1687,8 @@ void kvm_arch_pre_run(CPUX86State *env, struct kvm_run *run)
>>>
>>>           DPRINTF("setting tpr\n");
>>>           run->cr8 = cpu_get_apic_tpr(env->apic_state);
>>> +
>>> +        qemu_mutex_unlock_iothread();
>>>       }
>>>   }
>>>
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index c09cc39..60d91a5 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -471,6 +471,8 @@ void kvm_arch_pre_run(CPUPPCState *env, struct kvm_run *run)
>>>       int r;
>>>       unsigned irq;
>>>
>>> +    qemu_mutex_lock_iothread();
>>> +
>>>       /* PowerPC QEMU tracks the various core input pins (interrupt, critical
>>>        * interrupt, reset, etc) in PPC-specific env->irq_input_state. */
>>>       if (!cap_interrupt_level&&
>>> @@ -497,6 +499,8 @@ void kvm_arch_pre_run(CPUPPCState *env, struct kvm_run *run)
>>>       /* We don't know if there are more interrupts pending after this. However,
>>>        * the guest will return to userspace in the course of handling this one
>>>        * anyways, so we will get a chance to deliver the rest. */
>>> +
>>> +    qemu_mutex_unlock_iothread();
>>>   }
>>>
>>>   void kvm_arch_post_run(CPUPPCState *env, struct kvm_run *run)
>>>
>
> The following plan would allow progressive convertion to parallel
> operation.
>
> Jan mentioned the MMIO handler->MMIO handler deadlock in a private message.
>
> Jan: if there is recursive MMIO accesses, you can detect that and skip
> such MMIO handlers in dev_can_use_lock() ? Or blacklist.
>
> What i mentioned earlier about "unsolved issues" are the possibilities
> in "iothread flow" (belief is that it would be better determined at
> with execution experience / tuning).
>
> OK, so, each document starts with xyz.txt. This is compatible with TCG,
> the critical part is not dropping/acquire locks for decent performance
> (that is, only drop memmap_lock from TCG VCPUS thread if IOTHREAD
> requests so).
>
> This came out from discussions with Avi.
>
> immediate-plan.txt
> ------------------
>
> 1.  read_lock(memmap_lock)
> 2.  MemoryRegionSection mrs = lookup(addr)
> 3.  qom_ref(mrs.mr->dev)
> 4.  read_unlock(memmap_lock)
>
> 5.  mutex_lock(dev->lock)
> 6.  dispatch(&mrs, addr, data, size)
> 7.  mutex_unlock(dev->lock)

Just a detail, I don't think we should acquire a device specific lock in global 
code.  Rather, I think we should acquire the global lock before dispatch unless 
a MemoryRegion is marked as being unlocked.

Regards,

Anthony Liguori

>
> 8.  qom_unref(mrs.mr->object)
>
>
> A) Add Device object to MemoryRegions.
>
> memory_region_add_subregion
> memory_region_add_eventfd
> memory_region_add_subregion_overlap
>
>
> B) qom_ref details
>
> Z) see device-hotplug.txt items
>
>
>
> lock-model.txt
> --------------
>
>
> lock_device(dev) {
>      if (dev_can_use_lock(dev))
>          mutex_lock(dev->lock)
>      else
>          mutex_lock(qemu_global_mutex)
> }
>
> dev_can_use_lock(dev)
> {
>      if (all services used by dev mmio handler have)
>          - qemu_global_mutex
>          - trylock(dev->lock) and fail
>
>          then yes
>      else
>          then no
> }
>
>
> That is, from the moment in which the device uses the order
> (dev->lock ->  qemu_global_mutex), the qemu_global_mutex ->  dev->lock
> is forbidden by the IOTHREAD or anyone else.
>
>
> convert-to-unlocked.txt
> -----------------------
>
>
> 1.  read_lock(memmap_lock)
> 2.  MemoryRegionSection mrs = lookup(addr)
> 3.  qom_ref(mrs.mr->dev)
> 4.  read_unlock(memmap_lock)
>
> 5.  mutex_lock(dev->lock)
> 6.  dispatch(&mrs, addr, data, size)
> 7.  mutex_unlock(dev->lock)
>
> 8.  qom_unref(mrs.mr->object)
>
> THE ITEMS BELOW SHOULD BE DOCUMENTATION TO CONVERT DEVICE
> TO UNLOCKED OPERATION.
>
> 1) qom_ref guarantees that the Device object does not disappear. However,
> these operations are allowed in parallel:
>
>      a) after 4. device memory and ioports might change or be unregistered.
>
>         Concurrent accesses of a device that is being hotunplugged or
>         whose mappings change are responsability of the OS, so
>         incorrect emulation is not QEMU's problem. However, device
>         emulation is now subject to:
>              - cpu_physical_memory_map failing.
>              - stl_phys* failing.
>              - cpu_physical_memory_rw failing/reading from unassigned mem.
>
>      b) what should happen with a pending cpu_physical_memory_map pointer,
>         when deleting the corresponding memory region?
>
>
> net.txt
> --------
>
>
> iothread flow
> =============
>
> 1) Skip-work-if-device-locked
>
> select(tap fd ready)
> tap_send
>      if (trylock(TAPState->NetClientState->dev))
>          proceed;
>      else
>          continue; (data remains in queue)
>      tap_read_packet
>      qemu_send_packet_async
>
> DRAWBACK: lock intensive.
> DRAWBACK: VCPU threads can starve IOTHREAD (can be fixed with
> a scheme such as trylock() marks a flag saying "iothread wants lock".
>
> Checking each subsystem for possibility:
>
> - tap_send: OK
>      - user,slirp: if not possible, use device->lock.
> - qemu-char: OK
> - interrupts: lock with qemu_global_mutex.
> - qemutimer:
> - read time: qemu_get_clock_ns (see later).
>
>
> 2) Queue-work-to-vcpu-context
>
> select(tap fd ready)
> tap_send
>      if (trylock(TAPState->NetClientState->dev)) {
>          proceed;
>      } else {
>          AddToDeviceWorkQueue(work);
>      }
>      tap_read_packet
>      qemu_send_packet_async
>
> DRAWBACK: lock intensive
> DRAWBACK: cache effects
>
> 3) VCPU-thread-notify-device-unlocked
>
> select(tap fd ready)
> tap_send
>      if (trylock(TAPState->NetClientState->dev)) {
>          proceed;
>      } else {
>          signal VCPU thread to notify IOTHREAD when unlocked.
>      }
>      tap_read_packet
>
>
> GENERAL OBJECTIVE: to avoid lock inversion. IOTHREAD should follow
> same order.
>
>
> PCI HOTPLUG EJECT FLOW
> ----------------------
>
> 1) Monitor initiated.
>
> monitor ->  qmp_device_del ->  qdev_unplug ->  pci_unplug_device ->
> piix4_device_hotplug ->  SCI interrupt ->  OS removes
> application users from device ->  OS runs _EJ0 ->
>
> For hot removal, the device must be immediately ejected when OSPM calls
> the _EJ0 control method. The _EJ0 control method does not return until
> ejection is complete. After calling _EJ0, OSPM verifies the device no
> longer exists to determine if the eject succeeded. For _HID devices,
> OSPM evaluates the _STA method. For _ADR devices, OSPM checks with the
> bus driver for that device.
>
>
> Avi:  A pci eject guarantees that all accesses
> started after the eject completes do not see the device.
>
> Can you do:
>
> - qobject_remove()
>
> and have the device dispatch running safely? NO, therefore pci_unplug_device
> must hold dev->lock.
>
>
>
>