From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51101) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKSps-0005ty-9R for qemu-devel@nongnu.org; Tue, 05 Jul 2016 12:04:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKSpp-0006I8-1k for qemu-devel@nongnu.org; Tue, 05 Jul 2016 12:04:32 -0400 Received: from mail-lf0-x231.google.com ([2a00:1450:4010:c07::231]:34420) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKSpo-0006Ge-LM for qemu-devel@nongnu.org; Tue, 05 Jul 2016 12:04:28 -0400 Received: by mail-lf0-x231.google.com with SMTP id h129so137947446lfh.1 for ; Tue, 05 Jul 2016 09:04:28 -0700 (PDT) References: <1467389770-9738-1-git-send-email-alex.bennee@linaro.org> <1467389770-9738-3-git-send-email-alex.bennee@linaro.org> <20160702003942.GB2295@flamenco> <87lh1hmxsf.fsf@linaro.org> <20160704223001.GA29624@flamenco> <87inwkmj5d.fsf@linaro.org> <38496c08-3c5a-a8a7-dfb2-a81db29c4592@redhat.com> <87h9c4mdqu.fsf@linaro.org> <577BD383.4070701@gmail.com> <87furom5x4.fsf@linaro.org> From: Sergey Fedorov Message-ID: <577BDA89.6010306@gmail.com> Date: Tue, 5 Jul 2016 19:04:25 +0300 MIME-Version: 1.0 In-Reply-To: <87furom5x4.fsf@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 2/2] cpu-exec: remove tb_lock from the hot-path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= Cc: Paolo Bonzini , mttcg@listserver.greensocs.com, peter.maydell@linaro.org, Peter Crosthwaite , jan.kiszka@siemens.com, claudio.fontana@huawei.com, a.rigo@virtualopensystems.com, qemu-devel@nongnu.org, "Emilio G. Cota" , mark.burton@greensocs.com, bobby.prani@gmail.com, fred.konrad@greensocs.com, rth@twiddle.net On 05/07/16 19:00, Alex Bennée wrote: > Sergey Fedorov writes: > >> On 05/07/16 16:42, Paolo Bonzini wrote: >>> On 05/07/2016 15:11, Alex Bennée wrote: >>>> Paolo Bonzini writes: >>>> >>>>> On 05/07/2016 13:14, Alex Bennée wrote: >>>>>> /* >>>>>> * Patch the last TB with a jump to the current TB. >>>>>> * >>>>>> * Modification of the TB has to be protected with tb_lock which can >>>>>> * either be already held or taken here. >>>>>> */ >>>>>> static inline void maybe_patch_last_tb(CPUState *cpu, >>>>>> TranslationBlock *tb, >>>>>> TranslationBlock **last_tb, >>>>>> int tb_exit, >>>>>> bool locked) >>>>>> { >>>>>> if (cpu->tb_flushed) { >>>>>> /* Ensure that no TB jump will be modified as the >>>>>> * translation buffer has been flushed. >>>>>> */ >>>>>> *last_tb = NULL; >>>>>> cpu->tb_flushed = false; >>>>>> } >>>>>> #ifndef CONFIG_USER_ONLY >>>>>> /* We don't take care of direct jumps when address mapping changes in >>>>>> * system emulation. So it's not safe to make a direct jump to a TB >>>>>> * spanning two pages because the mapping for the second page can change. >>>>>> */ >>>>>> if (tb->page_addr[1] != -1) { >>>>>> *last_tb = NULL; >>>>>> } >>>>>> #endif >>>>>> /* See if we can patch the calling TB. */ >>>>>> if (*last_tb && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) { >>>>>> if (!locked) { >>>>>> tb_lock(); >>>>>> } >>>>>> tb_add_jump(*last_tb, tb_exit, tb); >>>>>> if (!locked) { >>>>>> tb_unlock(); >>>>>> } >>>>>> } >>>>>> } >>>>> Why not add tb_lock_recursive() and tb_lock_reset()? >>>> I thought we didn't like having recursive locking? I agree it would make >>>> things a little neater though. >>> I didn't like having recursive mutexes (because recursive mutexes >>> encourage you to be sloppy). Explicitly tagging some tb_lock()s as >>> recursive is fine, though. The explicit tag tells you to be careful. >> We could also introduce mmap_lock_reset() similar to tb_lock_reset(). >> Then we can remove tb_unlock() and mmap_unlock() from tb_find_slow(), >> call tb_lock() and mmap_lock() right before tb_add_jump(), and then call >> tb_lock_reset() and mmap_lock_reset() at the end of tb_find_fast(). This >> would render tb_lock() pretty useless though, since it would be always >> held under mmap_lock(). > I'm about to post v2. I've put all this aggressive extension of the > critical path as additional patches as I'm not sure it is really worth > it. > > The big win is doing the tb_jmp_cache and first tb_find_physical lookups > out of the lock. Trying to avoid bouncing the tb_lock() when patching > doesn't seem to make any difference in my micro benchmarks. I still have a feeling that we don't almost need both tb_lock() and mmap_lock(). Extending tb_lock() across tb_gen_code() and tb_add_jump() would be required should we decide to merge the locks. :) Thanks, Sergey