qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM
Date: Thu, 19 Mar 2015 09:52:47 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0


On 18/03/2015 16:04, Jan Kiszka wrote:
> On 2015-03-18 15:52, Paolo Bonzini wrote:
>>
>>
>> On 18/03/2015 15:33, Jan Kiszka wrote:
>>> Just in time: I'm planning to rebase our queue soon, specifically to
>>> benefit from RCU support. Will let you know if it works on top of this
>>> series.
>>
>> Great.  FWIW, this is the most similar version to the one we played with
>> in 2013.
>>
>> I have an alternative one that keeps the single address_space_rw API,
>> and checks if you have the iothread mutex using thread-local storage.
> 
> qemu_mutex_iothread_is_locked() with a qemu_global_mutex-specific flag
> or qemu_mutex_is_locked(&qemu_global_mutex)?

qemu_mutex_iothread_is_locked().

>> The reason for that is that I would have to introduce
>> address_space_map/unmap_unlocked too, which I didn't really like.  Also,
>> in the not-so-long term we want the same code (e.g. SCSI layer) to run
>> in both locked and unlocked mode.
>>
>> What do you think?
> 
> I cannot envision the code differences yet as we didn't have the need to
> play with lockless MMIO or even DMA so far (will probably happen soon,
> though - for networking). For me it boils down to the code impact as
> well, how much we can reuse by hiding locked vs. unlocked mode, and how
> much we may make more fragile and harder to design correctly by doing so.

The main reason, for me, is that I would have to either cut-and-paste 
code between address_space_map and address_space_map_unlocked, or I 
would have to use address_space_map_internal.  And so on through the 
whole stack, and at some point the _internal calls are not so _internal 
anymore.

So at least _internal could be renamed, but I am not sure it's the best 
choice.

The locking policy is easy: either call it with the BQL held (and any 
other locks you care about), or call it with no locks held.

The code impact is small.  Actually it's simplest to just paste the 
diff here:

diff --git a/cpus.c b/cpus.c
index 1ce90a1..58bcc4f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1116,6 +1116,13 @@ bool qemu_in_vcpu_thread(void)
     return current_cpu && qemu_cpu_is_self(current_cpu);
 }
 
+static __thread bool iothread_locked = false;
+
+bool qemu_mutex_iothread_locked(void)
+{
+    return iothread_locked;
+}
+
 void qemu_mutex_lock_iothread(void)
 {
     atomic_inc(&iothread_requesting_mutex);
@@ -1130,10 +1137,12 @@ void qemu_mutex_lock_iothread(void)
         atomic_dec(&iothread_requesting_mutex);
         qemu_cond_broadcast(&qemu_io_proceeded_cond);
     }
+    iothread_locked = true;
 }
 
 void qemu_mutex_unlock_iothread(void)
 {
+    iothread_locked = false;
     qemu_mutex_unlock(&qemu_global_mutex);
 }
 
diff --git a/exec.c b/exec.c
index c1a2e9e..6692f45 100644
--- a/exec.c
+++ b/exec.c
@@ -2297,9 +2297,9 @@ static int memory_access_size(MemoryRegion *mr, unsigned 
l, hwaddr addr)
     return l;
 }
 
-static bool address_space_rw_internal(AddressSpace *as, hwaddr addr,
-                                      uint8_t *buf, int len, bool is_write,
-                                      bool unlocked)
+static bool address_space_rw(AddressSpace *as, hwaddr addr,
+                             uint8_t *buf, int len, bool is_write,
+                             bool unlocked)
 {
     hwaddr l;
     uint8_t *ptr;
@@ -2307,6 +2307,7 @@ static bool address_space_rw_internal(AddressSpace *as, 
hwaddr addr,
     hwaddr addr1;
     MemoryRegion *mr;
     bool error = false;
+    bool unlocked = !qemu_mutex_iothread_locked();
     bool release_lock;
 
     rcu_read_lock();
@@ -2415,18 +2416,6 @@ static bool address_space_rw_internal(AddressSpace *as, 
hwaddr addr,
     return error;
 }
 
-bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
-                      int len, bool is_write)
-{
-    return address_space_rw_internal(as, addr, buf, len, is_write, false);
-}
-
-bool address_space_rw_unlocked(AddressSpace *as, hwaddr addr, uint8_t *buf,
-                               int len, bool is_write)
-{
-    return address_space_rw_internal(as, addr, buf, len, is_write, true);
-}
-
 bool address_space_write(AddressSpace *as, hwaddr addr,
                          const uint8_t *buf, int len)
 {
diff --git a/include/exec/memory.h b/include/exec/memory.h
index cf39a40..0644dc6 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1084,7 +1084,9 @@ void address_space_destroy(AddressSpace *as);
  * Return true if the operation hit any unassigned memory or encountered an
  * IOMMU fault.
  *
- * NOTE: The iothread lock must be held when calling this function.
+ * NOTE: If the iothread lock is not held when calling this function,
+ * _no_ other lock must be held.  Otherwise you risk lock inversion
+ * deadlocks.
  *
  * @as: #AddressSpace to be accessed
  * @addr: address within that address space
@@ -1095,23 +1093,6 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, 
uint8_t *buf,
                       int len, bool is_write);
 
 /**
- * address_space_rw_unlocked: read from or write to an address space outside
- * of the iothread lock.
- *
- * Return true if the operation hit any unassigned memory or encountered an
- * IOMMU fault.
- *
- * NOTE: The iothread lock *must not* be held when calling this function.
- *
- * @as: #AddressSpace to be accessed
- * @addr: address within that address space
- * @buf: buffer with the data transferred
- * @is_write: indicates the transfer direction
- */
-bool address_space_rw_unlocked(AddressSpace *as, hwaddr addr, uint8_t *buf,
-                               int len, bool is_write);
-
-/**
  * address_space_write: write to address space.
  *
  * Return true if the operation hit any unassigned memory or encountered an
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 62c68c0..6b74eb9 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -270,6 +270,16 @@ int qemu_add_child_watch(pid_t pid);
 #endif
 
 /**
+ * qemu_mutex_iothread_locked: Return lock status of the main loop mutex.
+ *
+ * The main loop mutex is the coarsest lock in QEMU, and as such it
+ * must always be taken outside other locks.  This function helps
+ * functions take different paths depending on whether the current
+ * thread is running within the main loop mutex.
+ */
+bool qemu_mutex_iothread_locked(void);
+
+/**
  * qemu_mutex_lock_iothread: Lock the main loop mutex.
  *
  * This function locks the main loop mutex.  The mutex is taken by
diff --git a/kvm-all.c b/kvm-all.c
index fa0cfed..5cac7e7 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1647,8 +1647,8 @@ static void kvm_handle_io(uint16_t port, void *data, int 
direction, int size,
     uint8_t *ptr = data;
 
     for (i = 0; i < count; i++) {
-        address_space_rw_unlocked(&address_space_io, port, ptr, size,
-                                  direction == KVM_EXIT_IO_OUT);
+        address_space_rw(&address_space_io, port, ptr, size,
+                         direction == KVM_EXIT_IO_OUT);
         ptr += size;
     }
 }
@@ -1813,11 +1813,11 @@ int kvm_cpu_exec(CPUState *cpu)
             break;
         case KVM_EXIT_MMIO:
             DPRINTF("handle_mmio\n");
-            address_space_rw_unlocked(&address_space_memory,
-                                      run->mmio.phys_addr,
-                                      run->mmio.data,
-                                      run->mmio.len,
-                                      run->mmio.is_write);
+            address_space_rw(&address_space_memory,
+                             run->mmio.phys_addr,
+                             run->mmio.data,
+                             run->mmio.len,
+                             run->mmio.is_write);
             ret = 0;
             break;
         case KVM_EXIT_IRQ_WINDOW_OPEN:
diff --git a/stubs/iothread-lock.c b/stubs/iothread-lock.c
index 5d8aca1..dda6f6b 100644
--- a/stubs/iothread-lock.c
+++ b/stubs/iothread-lock.c
@@ -1,6 +1,11 @@
 #include "qemu-common.h"
 #include "qemu/main-loop.h"
 
+bool qemu_mutex_iothread_locked(void)
+{
+    return true;
+}
+
 void qemu_mutex_lock_iothread(void)
 {
 }

Paolo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]