From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49967) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z81sm-0004Lu-MY for qemu-devel@nongnu.org; Thu, 25 Jun 2015 03:47:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z81sj-0006iD-1x for qemu-devel@nongnu.org; Thu, 25 Jun 2015 03:47:36 -0400 Received: from mail-wi0-x232.google.com ([2a00:1450:400c:c05::232]:36959) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z81si-0006hy-Ny for qemu-devel@nongnu.org; Thu, 25 Jun 2015 03:47:32 -0400 Received: by wicgi11 with SMTP id gi11so67343929wic.0 for ; Thu, 25 Jun 2015 00:47:32 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <558BB211.1000604@redhat.com> Date: Thu, 25 Jun 2015 09:47:29 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1435163110-2724-1-git-send-email-pbonzini@redhat.com> <1435163110-2724-6-git-send-email-pbonzini@redhat.com> <20150625051136.GJ17695@ad.nay.redhat.com> In-Reply-To: <20150625051136.GJ17695@ad.nay.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/9] memory: let address_space_rw/ld*/st* run outside the BQL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: borntraeger@de.ibm.com, Jan Kiszka , qemu-devel@nongnu.org, Frederic Konrad On 25/06/2015 07:11, Fam Zheng wrote: > On Wed, 06/24 18:25, Paolo Bonzini wrote: >> From: Jan Kiszka >> >> The MMIO case is further broken up in two cases: if the caller does not >> hold the BQL on invocation, the unlocked one takes or avoids BQL depending >> on the locking strategy of the target memory region and its coalesced >> MMIO handling. In this case, the caller should not hold _any_ lock >> (a friendly suggestion which is disregarded by virtio-scsi-dataplane). >> >> Signed-off-by: Jan Kiszka >> Cc: Frederic Konrad >> Message-Id: <1434646046-27150-6-git-send-email-pbonzini@redhat.com> >> Signed-off-by: Paolo Bonzini >> --- >> exec.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 60 insertions(+), 9 deletions(-) >> >> diff --git a/exec.c b/exec.c >> index f2e6603..fd0401e 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -48,6 +48,7 @@ >> #endif >> #include "exec/cpu-all.h" >> #include "qemu/rcu_queue.h" >> +#include "qemu/main-loop.h" >> #include "exec/cputlb.h" >> #include "translate-all.h" >> >> @@ -2316,11 +2317,27 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) >> return l; >> } >> >> -static void prepare_mmio_access(MemoryRegion *mr) >> +static bool prepare_mmio_access(MemoryRegion *mr) >> { >> + bool unlocked = !qemu_mutex_iothread_locked(); >> + bool release_lock = false; >> + >> + if (unlocked && mr->global_locking) { >> + qemu_mutex_lock_iothread(); >> + unlocked = false; >> + release_lock = true; >> + } >> if (mr->flush_coalesced_mmio) { >> + if (unlocked) { >> + qemu_mutex_lock_iothread(); >> + } >> qemu_flush_coalesced_mmio_buffer(); >> + if (unlocked) { >> + qemu_mutex_unlock_iothread(); >> + } >> } >> + >> + return release_lock; >> } >> >> MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, >> @@ -2332,6 +2349,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, >> hwaddr addr1; >> MemoryRegion *mr; >> MemTxResult result = MEMTX_OK; >> + bool release_lock = false; >> >> rcu_read_lock(); >> while (len > 0) { >> @@ -2340,7 +2358,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, >> >> if (is_write) { >> if (!memory_access_is_direct(mr, is_write)) { >> - prepare_mmio_access(mr); >> + release_lock |= prepare_mmio_access(mr); >> l = memory_access_size(mr, l, addr1); >> /* XXX: could force current_cpu to NULL to avoid >> potential bugs */ >> @@ -2382,7 +2400,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, >> } else { >> if (!memory_access_is_direct(mr, is_write)) { >> /* I/O case */ >> - prepare_mmio_access(mr); >> + release_lock |= prepare_mmio_access(mr); >> l = memory_access_size(mr, l, addr1); >> switch (l) { >> case 8: >> @@ -2418,6 +2436,12 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, >> memcpy(buf, ptr, l); >> } >> } >> + >> + if (release_lock) { >> + qemu_mutex_unlock_iothread(); >> + release_lock = false; >> + } >> + >> len -= l; >> buf += l; >> addr += l; >> @@ -2744,11 +2768,12 @@ static inline uint32_t address_space_ldl_internal(AddressSpace *as, hwaddr addr, >> hwaddr l = 4; >> hwaddr addr1; >> MemTxResult r; >> + bool release_lock = false; >> >> rcu_read_lock(); >> mr = address_space_translate(as, addr, &addr1, &l, false); >> if (l < 4 || !memory_access_is_direct(mr, false)) { >> - prepare_mmio_access(mr); >> + release_lock |= prepare_mmio_access(mr); >> >> /* I/O case */ >> r = memory_region_dispatch_read(mr, addr1, &val, 4, attrs); >> @@ -2782,6 +2807,9 @@ static inline uint32_t address_space_ldl_internal(AddressSpace *as, hwaddr addr, >> if (result) { >> *result = r; >> } >> + if (release_lock) { >> + qemu_mutex_unlock_iothread(); >> + } >> rcu_read_unlock(); >> return val; >> } >> @@ -2834,12 +2862,13 @@ static inline uint64_t address_space_ldq_internal(AddressSpace *as, hwaddr addr, >> hwaddr l = 8; >> hwaddr addr1; >> MemTxResult r; >> + bool release_lock = false; >> >> rcu_read_lock(); >> mr = address_space_translate(as, addr, &addr1, &l, >> false); >> if (l < 8 || !memory_access_is_direct(mr, false)) { >> - prepare_mmio_access(mr); >> + release_lock |= prepare_mmio_access(mr); >> >> /* I/O case */ >> r = memory_region_dispatch_read(mr, addr1, &val, 8, attrs); >> @@ -2873,6 +2902,9 @@ static inline uint64_t address_space_ldq_internal(AddressSpace *as, hwaddr addr, >> if (result) { >> *result = r; >> } >> + if (release_lock) { >> + qemu_mutex_unlock_iothread(); >> + } >> rcu_read_unlock(); >> return val; >> } >> @@ -2945,12 +2977,13 @@ static inline uint32_t address_space_lduw_internal(AddressSpace *as, >> hwaddr l = 2; >> hwaddr addr1; >> MemTxResult r; >> + bool release_lock = false; >> >> rcu_read_lock(); >> mr = address_space_translate(as, addr, &addr1, &l, >> false); >> if (l < 2 || !memory_access_is_direct(mr, false)) { >> - prepare_mmio_access(mr); >> + release_lock |= prepare_mmio_access(mr); >> >> /* I/O case */ >> r = memory_region_dispatch_read(mr, addr1, &val, 2, attrs); >> @@ -2984,6 +3017,9 @@ static inline uint32_t address_space_lduw_internal(AddressSpace *as, >> if (result) { >> *result = r; >> } >> + if (release_lock) { >> + qemu_mutex_unlock_iothread(); >> + } >> rcu_read_unlock(); >> return val; >> } >> @@ -3036,12 +3072,13 @@ void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val, >> hwaddr addr1; >> MemTxResult r; >> uint8_t dirty_log_mask; >> + bool release_lock = false; >> >> rcu_read_lock(); >> mr = address_space_translate(as, addr, &addr1, &l, >> true); >> if (l < 4 || !memory_access_is_direct(mr, true)) { >> - prepare_mmio_access(mr); >> + release_lock |= prepare_mmio_access(mr); >> >> r = memory_region_dispatch_write(mr, addr1, val, 4, attrs); >> } else { >> @@ -3057,6 +3094,9 @@ void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val, >> if (result) { >> *result = r; >> } >> + if (release_lock) { >> + qemu_mutex_unlock_iothread(); >> + } >> rcu_read_unlock(); >> } >> >> @@ -3077,12 +3117,13 @@ static inline void address_space_stl_internal(AddressSpace *as, >> hwaddr l = 4; >> hwaddr addr1; >> MemTxResult r; >> + bool release_lock = false; >> >> rcu_read_lock(); >> mr = address_space_translate(as, addr, &addr1, &l, >> true); >> if (l < 4 || !memory_access_is_direct(mr, true)) { >> - prepare_mmio_access(mr); >> + release_lock |= prepare_mmio_access(mr); >> >> #if defined(TARGET_WORDS_BIGENDIAN) >> if (endian == DEVICE_LITTLE_ENDIAN) { >> @@ -3115,6 +3156,9 @@ static inline void address_space_stl_internal(AddressSpace *as, >> if (result) { >> *result = r; >> } >> + if (release_lock) { >> + qemu_mutex_unlock_iothread(); >> + } >> rcu_read_unlock(); >> } >> >> @@ -3184,11 +3228,12 @@ static inline void address_space_stw_internal(AddressSpace *as, >> hwaddr l = 2; >> hwaddr addr1; >> MemTxResult r; >> + bool release_lock = false; >> >> rcu_read_lock(); >> mr = address_space_translate(as, addr, &addr1, &l, true); >> if (l < 2 || !memory_access_is_direct(mr, true)) { >> - prepare_mmio_access(mr); >> + release_lock |= prepare_mmio_access(mr); >> >> #if defined(TARGET_WORDS_BIGENDIAN) >> if (endian == DEVICE_LITTLE_ENDIAN) { >> @@ -3221,6 +3266,12 @@ static inline void address_space_stw_internal(AddressSpace *as, >> if (result) { >> *result = r; >> } >> + if (release_lock) { >> + qemu_mutex_unlock_iothread(); >> + } >> + if (release_lock) { >> + qemu_mutex_unlock_iothread(); >> + } > > Bad rebase? Yes. Paolo