From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53554) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5WeW-0007gc-0M for qemu-devel@nongnu.org; Wed, 25 May 2016 07:07:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5WeR-00043d-NE for qemu-devel@nongnu.org; Wed, 25 May 2016 07:07:02 -0400 Received: from mail-wm0-x235.google.com ([2a00:1450:400c:c09::235]:36738) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5WeR-00043X-9A for qemu-devel@nongnu.org; Wed, 25 May 2016 07:06:59 -0400 Received: by mail-wm0-x235.google.com with SMTP id n129so175619999wmn.1 for ; Wed, 25 May 2016 04:06:59 -0700 (PDT) References: <1459870344-16773-1-git-send-email-alex.bennee@linaro.org> <1459870344-16773-11-git-send-email-alex.bennee@linaro.org> <5744C792.6030908@gmail.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: Date: Wed, 25 May 2016 12:07:35 +0100 Message-ID: <87wpmics5k.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC v2 10/11] tcg: drop global lock during TCG code execution List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Sergey Fedorov , mttcg@greensocs.com, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org, qemu-devel@nongnu.org, mark.burton@greensocs.com, jan.kiszka@siemens.com, rth@twiddle.net, peter.maydell@linaro.org, claudio.fontana@huawei.com, Peter Crosthwaite , Eduardo Habkost , "Michael S. Tsirkin" Paolo Bonzini writes: > On 24/05/2016 23:28, Sergey Fedorov wrote: >> On 05/04/16 18:32, Alex Bennée wrote: >>> diff --git a/cpu-exec.c b/cpu-exec.c >>> index bd50fef..f558508 100644 >>> --- a/cpu-exec.c >>> +++ b/cpu-exec.c >>> @@ -28,6 +28,7 @@ >>> #include "qemu/rcu.h" >>> #include "exec/tb-hash.h" >>> #include "exec/log.h" >>> +#include "qemu/main-loop.h" >>> #if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY) >>> #include "hw/i386/apic.h" >>> #endif >>> @@ -452,6 +453,8 @@ int cpu_exec(CPUState *cpu) >>> for(;;) { >>> interrupt_request = cpu->interrupt_request; >>> if (unlikely(interrupt_request)) { >>> + qemu_mutex_lock_iothread(); >>> + >>> if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) { >>> /* Mask out external interrupts for this step. */ >>> interrupt_request &= ~CPU_INTERRUPT_SSTEP_MASK; >>> @@ -504,6 +507,11 @@ int cpu_exec(CPUState *cpu) >>> the program flow was changed */ >>> next_tb = 0; >>> } >>> + >>> + if (qemu_mutex_iothread_locked()) { >>> + qemu_mutex_unlock_iothread(); >>> + } >>> + >> >> Why do we need to check for "qemu_mutex_iothread_locked()" here? >> iothread lock is always held here, isn't it? > > Correct. I'll fix that for the next iteration. The main else leg still needs to check though as it doesn't know if the mutex was grabbed before a siglongjmp. > >>> } >>> if (unlikely(cpu->exit_request >>> || replay_has_interrupt())) { >> (snip) >>> diff --git a/cputlb.c b/cputlb.c >>> index 466663b..1412049 100644 >>> --- a/cputlb.c >>> +++ b/cputlb.c >>> @@ -18,6 +18,7 @@ >>> */ >>> >>> #include "qemu/osdep.h" >>> +#include "qemu/main-loop.h" >> >> No need in this include. >> >>> #include "cpu.h" >>> #include "exec/exec-all.h" >>> #include "exec/memory.h" >>> diff --git a/exec.c b/exec.c >>> index c46c123..9e83673 100644 >>> --- a/exec.c >>> +++ b/exec.c >> (snip) >>> @@ -2347,8 +2353,14 @@ static void io_mem_init(void) >>> memory_region_init_io(&io_mem_rom, NULL, &unassigned_mem_ops, NULL, NULL, UINT64_MAX); >>> memory_region_init_io(&io_mem_unassigned, NULL, &unassigned_mem_ops, NULL, >>> NULL, UINT64_MAX); >>> + >>> + /* io_mem_notdirty calls tb_invalidate_phys_page_fast, >>> + * which must be called without the iothread mutex. >>> + */ >> >> notdirty_mem_write() operates on page dirty flags. Is it safe to do so >> out of iothread lock? > > Yes, see commit 5f2cb94 ("memory: make > cpu_physical_memory_sync_dirty_bitmap() fully atomic", 2014-12-02) and > those before. > >>> memory_region_init_io(&io_mem_notdirty, NULL, ¬dirty_mem_ops, NULL, >>> NULL, UINT64_MAX); >>> + memory_region_clear_global_locking(&io_mem_notdirty); >>> + >>> memory_region_init_io(&io_mem_watch, NULL, &watch_mem_ops, NULL, >>> NULL, UINT64_MAX); >>> } >> (snip) >>> diff --git a/translate-all.c b/translate-all.c >>> index 935d24c..0c377bb 100644 >>> --- a/translate-all.c >>> +++ b/translate-all.c >>> @@ -1318,6 +1318,7 @@ void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end) >>> * this TB. >>> * >>> * Called with mmap_lock held for user-mode emulation >>> + * If called from generated code, iothread mutex must not be held. >> >> What does that mean? If iothread mutex is not required by the function >> then why mention it here at all? > > If this function can take the iothread mutex itself, this would cause a > deadlock. I'm not sure if it could though. > > Another possibility is that this function can longjmp out into cpu_exec, > and then the iothread mutex would not be released. I think this is more > likely. Any longjmp should: tb_lock_reset(); if (qemu_mutex_iothread_locked()) { qemu_mutex_unlock_iothread(); } To reset the locks. > > Thanks, > > Paolo -- Alex Bennée