From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52905) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bG9ay-0001Ys-4p for qemu-devel@nongnu.org; Thu, 23 Jun 2016 14:43:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bG9au-0002jX-6k for qemu-devel@nongnu.org; Thu, 23 Jun 2016 14:43:20 -0400 Received: from mail-lf0-x244.google.com ([2a00:1450:4010:c07::244]:35898) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bG9at-0002jC-Us for qemu-devel@nongnu.org; Thu, 23 Jun 2016 14:43:16 -0400 Received: by mail-lf0-x244.google.com with SMTP id a2so19228070lfe.3 for ; Thu, 23 Jun 2016 11:43:15 -0700 (PDT) References: <87fus3rfnw.fsf@linaro.org> From: Sergey Fedorov Message-ID: <576C2DC0.1040403@gmail.com> Date: Thu, 23 Jun 2016 21:43:12 +0300 MIME-Version: 1.0 In-Reply-To: <87fus3rfnw.fsf@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC v3 03/19] translate-all: add DEBUG_LOCKING asserts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= Cc: mttcg@listserver.greensocs.com, qemu-devel@nongnu.org, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org, bobby.prani@gmail.com, mark.burton@greensocs.com, pbonzini@redhat.com, jan.kiszka@siemens.com, rth@twiddle.net, peter.maydell@linaro.org, claudio.fontana@huawei.com, Blue Swirl , Peter Crosthwaite , Riku Voipio On 23/06/16 20:14, Alex Bennée wrote: > Sergey Fedorov writes: > >> On 03/06/16 23:40, Alex Bennée wrote: >>> diff --git a/translate-all.c b/translate-all.c >>> index ec6fdbb..e3f44d9 100644 >>> --- a/translate-all.c >>> +++ b/translate-all.c >> (snip) >>> @@ -60,6 +61,7 @@ >>> >>> /* #define DEBUG_TB_INVALIDATE */ >>> /* #define DEBUG_TB_FLUSH */ >>> +/* #define DEBUG_LOCKING */ >> A bit of bikeshedding: have you considered naming it 'DEBUG_LOCKS'. How >> does it sound for a native English speaker? :) >> >>> /* make various TB consistency checks */ >>> /* #define DEBUG_TB_CHECK */ >>> >>> @@ -68,6 +70,28 @@ >>> #undef DEBUG_TB_CHECK >>> #endif >>> >>> +/* Access to the various translations structures need to be serialised via locks >>> + * for consistency. This is automatic for SoftMMU based system >>> + * emulation due to its single threaded nature. In user-mode emulation >>> + * access to the memory related structures are protected with the >>> + * mmap_lock. >>> + */ >>> +#ifdef DEBUG_LOCKING >>> +#define DEBUG_MEM_LOCKS 1 >>> +#else >>> +#define DEBUG_MEM_LOCKS 0 >>> +#endif >>> + >>> +#ifdef CONFIG_SOFTMMU >>> +#define assert_memory_lock() do { /* nothing */ } while (0) >>> +#else >>> +#define assert_memory_lock() do { \ >>> + if (DEBUG_MEM_LOCKS) { \ >>> + g_assert(have_mmap_lock()); \ >>> + } \ >>> + } while (0) >>> +#endif >>> + >> Why not simply: >> >> #if !defined(DEBUG_LOCKING) || defined(CONFIG_SOFTMMU) >> # define assert_memory_lock() do { /* nothing */ } while (0) >> #else >> # define assert_memory_lock() g_assert(have_mmap_lock()) >> #endif >> >> One more nit: maybe it would be a bit more clear to name it after the >> lock name, i.e. assert_mmap_lock(), or check_mmap_lock(), or >> debug_mmap_lock() etc? > Yes I can do it that way around. The if (FOO) form makes more sense for > debug output to ensure the compiler checks format strings etc. The > resulting code should be the same either way. You are right, this is a good thing, I just missed it. Thanks, Sergey