From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45222) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiKuV-00063b-SM for qemu-devel@nongnu.org; Tue, 22 Mar 2016 07:55:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiKuR-0007Ya-GW for qemu-devel@nongnu.org; Tue, 22 Mar 2016 07:55:43 -0400 Received: from mail-wm0-x22a.google.com ([2a00:1450:400c:c09::22a]:33796) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiKuR-0007YS-7h for qemu-devel@nongnu.org; Tue, 22 Mar 2016 07:55:39 -0400 Received: by mail-wm0-x22a.google.com with SMTP id p65so188860046wmp.1 for ; Tue, 22 Mar 2016 04:55:38 -0700 (PDT) References: <1458317932-1875-1-git-send-email-alex.bennee@linaro.org> <1458317932-1875-2-git-send-email-alex.bennee@linaro.org> <20160321215039.GA2466@flamenco> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <20160321215039.GA2466@flamenco> Date: Tue, 22 Mar 2016 11:55:36 +0000 Message-ID: <87io0esp53.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC v1 01/11] tcg: move tb_find_fast outside the tb_lock critical section List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Emilio G. Cota" Cc: mttcg@listserver.greensocs.com, Peter Crosthwaite , mark.burton@greensocs.com, qemu-devel@nongnu.org, a.rigo@virtualopensystems.com, pbonzini@redhat.com, serge.fdrv@gmail.com, Richard Henderson , Andreas =?utf-8?Q?F=C3=A4rber?= , fred.konrad@greensocs.com Emilio G. Cota writes: > On Fri, Mar 18, 2016 at 16:18:42 +0000, Alex Bennée wrote: >> From: KONRAD Frederic >> >> Signed-off-by: KONRAD Frederic >> Signed-off-by: Paolo Bonzini >> [AJB: minor checkpatch fixes] >> Signed-off-by: Alex Bennée >> >> --- >> v1(ajb) >> - checkpatch fixes >> --- >> diff --git a/cpu-exec.c b/cpu-exec.c >> index 07545aa..52f25de 100644 >> --- a/cpu-exec.c >> +++ b/cpu-exec.c >> @@ -225,8 +225,9 @@ static TranslationBlock *tb_find_physical(CPUState *cpu, >> phys_page1 = phys_pc & TARGET_PAGE_MASK; >> h = tb_phys_hash_func(phys_pc); >> for (ptb1 = &tcg_ctx.tb_ctx.tb_phys_hash[h]; >> - (tb = *ptb1) != NULL; >> + (tb = atomic_read(ptb1)) != NULL; >> ptb1 = &tb->phys_hash_next) { >> + smp_read_barrier_depends(); >> if (tb->pc != pc || >> tb->page_addr[0] != phys_page1 || >> tb->cs_base != cs_base || >> @@ -254,7 +255,18 @@ static TranslationBlock *tb_find_physical(CPUState *cpu, > [ Adding this missing line to the diff for clarity ] I have to admit that clarity is one thing the code in this area could do with. I find it hard to follow on the best of days. > /* Move the TB to the head of the list */ >> *ptb1 = tb->phys_hash_next; >> tb->phys_hash_next = tcg_ctx.tb_ctx.tb_phys_hash[h]; >> tcg_ctx.tb_ctx.tb_phys_hash[h] = tb; > > This function, as is, doesn't really just "find"; two concurrent "finders" > could race here by *writing* to the head of the list at the same time. > > The fix is to get rid of this write entirely; moving the just-found TB to > the head of the list is not really that necessary thanks to the CPU's > tb_jmp_cache table. This fix would make the function read-only, which > is what the function's name implies. > > Further, I'd like to see tb_phys_hash to use the RCU queue primitives; it > makes everything easier to understand (and we avoid sprinkling the code > base with smp_barrier_depends). > > I have these two changes queued up as part of my upcoming series, which I'm > basing on your patchset. Cool, I look forward to it. > > Thanks for putting these changes together! This was exactly my aim, getting the common base stuff reviewed so the competing plurality of approaches can build on it as we shake out the design ;-) > > Emilio -- Alex Bennée