From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54508) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bG8Cy-0007PT-1r for qemu-devel@nongnu.org; Thu, 23 Jun 2016 13:14:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bG8Cu-00046v-K2 for qemu-devel@nongnu.org; Thu, 23 Jun 2016 13:14:27 -0400 Received: from mail-wm0-x235.google.com ([2a00:1450:400c:c09::235]:36830) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bG8Cu-00046k-DC for qemu-devel@nongnu.org; Thu, 23 Jun 2016 13:14:24 -0400 Received: by mail-wm0-x235.google.com with SMTP id f126so135309952wma.1 for ; Thu, 23 Jun 2016 10:14:24 -0700 (PDT) From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <576BF38E.2000408@gmail.com> Date: Thu, 23 Jun 2016 18:14:27 +0100 Message-ID: <87fus3rfnw.fsf@linaro.org> MIME-Version: 1.0 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: Sergey Fedorov 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 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. > > Thanks, > Sergey -- Alex Bennée