From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44159) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZP3Ks-0001LC-6C for qemu-devel@nongnu.org; Tue, 11 Aug 2015 02:46:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZP3Km-00034R-Hr for qemu-devel@nongnu.org; Tue, 11 Aug 2015 02:46:58 -0400 Received: from greensocs.com ([193.104.36.180]:37170) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZP3Km-00033i-5u for qemu-devel@nongnu.org; Tue, 11 Aug 2015 02:46:52 -0400 Message-ID: <55C99A57.2090406@greensocs.com> Date: Tue, 11 Aug 2015 08:46:47 +0200 From: Frederic Konrad MIME-Version: 1.0 References: <1439220437-23957-1-git-send-email-fred.konrad@greensocs.com> <1439220437-23957-8-git-send-email-fred.konrad@greensocs.com> <55C8D31A.8070703@redhat.com> In-Reply-To: <55C8D31A.8070703@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH V7 07/19] protect TBContext with tb_lock. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org, mttcg@listserver.greensocs.com Cc: mark.burton@greensocs.com, alex.bennee@linaro.org, a.rigo@virtualopensystems.com, guillaume.delbergue@greensocs.com On 10/08/2015 18:36, Paolo Bonzini wrote: > > On 10/08/2015 17:27, fred.konrad@greensocs.com wrote: >> diff --git a/cpu-exec.c b/cpu-exec.c >> index f3358a9..a012e9d 100644 >> --- a/cpu-exec.c >> +++ b/cpu-exec.c >> @@ -131,6 +131,8 @@ static void init_delay_params(SyncClocks *sc, const CPUState *cpu) >> void cpu_loop_exit(CPUState *cpu) >> { >> cpu->current_tb = NULL; >> + /* Release those mutex before long jump so other thread can work. */ >> + tb_lock_reset(); >> siglongjmp(cpu->jmp_env, 1); >> } >> >> @@ -143,6 +145,8 @@ void cpu_resume_from_signal(CPUState *cpu, void *puc) >> /* XXX: restore cpu registers saved in host registers */ >> >> cpu->exception_index = -1; >> + /* Release those mutex before long jump so other thread can work. */ >> + tb_lock_reset(); >> siglongjmp(cpu->jmp_env, 1); >> } >> > I think you should start easy and reuse the existing tb_lock code in > cpu-exec.c: I think it's definitely not sufficient. Is user-mode multithread still working today? > > diff --git a/cpu-exec.c b/cpu-exec.c > index 9305f03..2909ec2 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -307,7 +307,6 @@ static TranslationBlock *tb_find_slow(CPUState *cpu, target_ulong pc, > > tb = tb_find_physical(cpu, pc, cs_base, flags); > if (!tb) { > - tb_lock(); > /* > * Retry to get the TB in case a CPU just translate it to avoid having > * duplicated TB in the pool. > @@ -316,7 +315,6 @@ static TranslationBlock *tb_find_slow(CPUState *cpu, target_ulong pc, > if (!tb) { > tb = tb_gen_code(cpu, pc, cs_base, flags, 0); > } > - tb_unlock(); > } > /* we add the TB in the virtual pc hash table */ > cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)] = tb; > @@ -372,11 +372,6 @@ int cpu_exec(CPUState *cpu) > uintptr_t next_tb; > SyncClocks sc; > > - /* This must be volatile so it is not trashed by longjmp() */ > -#if defined(CONFIG_USER_ONLY) > - volatile bool have_tb_lock = false; > -#endif > - > if (cpu->halted) { > if (!cpu_has_work(cpu)) { > return EXCP_HALTED; > @@ -480,10 +475,7 @@ int cpu_exec(CPUState *cpu) > cpu->exception_index = EXCP_INTERRUPT; > cpu_loop_exit(cpu); > } > -#if defined(CONFIG_USER_ONLY) > - qemu_mutex_lock(&tcg_ctx.tb_ctx.tb_lock); > - have_tb_lock = true; > -#endif > + tb_lock(); > tb = tb_find_fast(cpu); > /* Note: we do it here to avoid a gcc bug on Mac OS X when > doing it in tb_find_slow */ > @@ -505,10 +497,7 @@ int cpu_exec(CPUState *cpu) > tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK), > next_tb & TB_EXIT_MASK, tb); > } > -#if defined(CONFIG_USER_ONLY) > - have_tb_lock = false; > - qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock); > -#endif > + tb_unlock(); > /* cpu_interrupt might be called while translating the > TB, but before it is linked into a potentially > infinite loop and becomes env->current_tb. Avoid > @@ -575,12 +564,7 @@ int cpu_exec(CPUState *cpu) > x86_cpu = X86_CPU(cpu); > env = &x86_cpu->env; > #endif > -#if defined(CONFIG_USER_ONLY) > - if (have_tb_lock) { > - qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock); > - have_tb_lock = false; > - } > -#endif > + tb_lock_reset(); > } > } /* for(;;) */ > > > Optimizations should then come on top. > >> diff --git a/target-arm/translate.c b/target-arm/translate.c >> index 69ac18c..960c75e 100644 >> --- a/target-arm/translate.c >> +++ b/target-arm/translate.c >> @@ -11166,6 +11166,8 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu, >> >> dc->tb = tb; >> >> + tb_lock(); > This locks twice, I think? Both cpu_restore_state_from_tb and > tb_gen_code (which calls cpu_gen_code) take the lock. How does it work? > Yes it's recursive we might not need that though. I probably locked too much some function. Thanks, Fred >> + >> dc->is_jmp = DISAS_NEXT; >> dc->pc = pc_start; >> dc->singlestep_enabled = cs->singlestep_enabled; >> @@ -11506,6 +11508,7 @@ done_generating: >> tb->size = dc->pc - pc_start; >> tb->icount = num_insns; >> } >> + tb_unlock(); >> } >> >> +/* tb_lock must be help for tcg_malloc_internal. */ > "Held", not "help". > > Paolo >