Re: [Qemu-devel] [RFC v3 03/19] translate-all: add DEBUG_LOCKING asserts

["Alex Bennée" <alex.bennee@linaro.org>]

Sergey Fedorov <serge.fdrv@gmail.com> 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