From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35441) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqjK6-00014S-PG for qemu-devel@nongnu.org; Thu, 14 Apr 2016 11:36:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aqjK2-0005hw-JN for qemu-devel@nongnu.org; Thu, 14 Apr 2016 11:36:50 -0400 Received: from mail-lf0-x241.google.com ([2a00:1450:4010:c07::241]:34513) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqjK2-0005hs-BA for qemu-devel@nongnu.org; Thu, 14 Apr 2016 11:36:46 -0400 Received: by mail-lf0-x241.google.com with SMTP id e190so12747378lfe.1 for ; Thu, 14 Apr 2016 08:36:46 -0700 (PDT) References: <1458222382-6498-1-git-send-email-sergey.fedorov@linaro.org> <1458222382-6498-5-git-send-email-sergey.fedorov@linaro.org> <56EAC8A2.7060700@redhat.com> <56EAC9E3.60000@gmail.com> <56F94B59.80905@gmail.com> <56F9A051.9090907@redhat.com> <56FA52E3.3000900@gmail.com> <56FA5ADB.7030103@redhat.com> <570FACF1.6020009@gmail.com> <570FB396.6040703@redhat.com> From: Sergey Fedorov Message-ID: <570FB90B.1010209@gmail.com> Date: Thu, 14 Apr 2016 18:36:43 +0300 MIME-Version: 1.0 In-Reply-To: <570FB396.6040703@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/5] tcg: reorder removal from lists in tb_phys_invalidate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , sergey.fedorov@linaro.org, qemu-devel@nongnu.org Cc: Richard Henderson , Peter Crosthwaite , =?UTF-8?Q?Alex_Benn=c3=a9e?= On 14/04/16 18:13, Paolo Bonzini wrote: > This is very similar to the current code. From 10,000 feet, because > tb_find_fast calls tb_find_slow, this could indeed work, but I'm a bit > concerned about how to order the removal of the jump lists. The usage > of "tcg_ctx.tb_ctx.tb_invalidated_flag = 1" in the existing code was > what worries me. Indeed the motivation of this patch was removing that > single line of code to prepare for the move of tb_invalidated_flag to > CPUState. I'm just preparing a patch with a long commit message which describes and removes this setting of tb_invalidated_flag :) I think we can do this safely and even benefit in performance. > > Also, this loop will not be thread-safe anymore as soon as Fred's > "tb_jmp_cache lookup outside tb_lock" goes in: > > CPU_FOREACH(cpu) { > if (cpu->tb_jmp_cache[h] == tb) { > cpu->tb_jmp_cache[h] = NULL; > } > } > > It should use atomic_cmpxchg (slow!) or to unconditionally NULL out > cpu->tb_jmp_cache (a bit hacky). Preparing for that change is an added > bonus of the tb-hacking approach. IIUC we always modify tb_jmp_cache/tb_phys_hash under tb_lock. We're just gonna do tb_jmp_cache lookup outside of tb_lock. I think write memory barrier in tb_phys_invalidate() paired with read memory memory barrier in tb_find_fast()/tb_find_slow() would be enough to make it safe. Also we need to use atomic read/set to tb_jmp_cache but cmpxchg is not necessary. So I'm going to give it a thought then. Kind regards, Sergey