Re: [Qemu-devel] [PATCH v2] QemuMutex: support --enable-debug-mutex

[Peter Xu <peterx@redhat.com>]

On Thu, Apr 19, 2018 at 03:43:01PM -0400, Emilio G. Cota wrote:
> On Thu, Apr 19, 2018 at 11:13:35 +0800, Peter Xu wrote:
> > On Thu, Apr 19, 2018 at 09:56:31AM +0800, Fam Zheng wrote:
> (snip)
> > > > @@ -12,6 +12,10 @@ typedef QemuMutex QemuRecMutex;
> > > >  
> > > >  struct QemuMutex {
> > > >      pthread_mutex_t lock;
> > > > +#ifdef CONFIG_DEBUG_MUTEX
> > > > +    const char *file;
> > > > +    int line;
> > > > +#endif
> > > 
> > > These look quite cheap, why we need a configure option to enable it?
> > 
> > Yeah; I am not 100% sure about whether it's cheap or not, hence with
> > that.  I can remove that part if we are sure we want it always.
> 
> I can think of a few good reasons not to enable these by default.
> 
> * Adds 12 bytes to struct QemuMutex on 64-bit hosts.

Yes, I worried about this too.  Mutex is still widely used, and there
can be a large amount of locks even not used quite often but will
still consume the space.

> * Increases slightly the critical region (the assignment happens
>   once the lock has been acquired)
>   + This is measurable for a single-thread with a microbenchmark:
> Before (no --enable-debug-mutex):
> $ taskset -c 0 tests/atomic_add-bench -n 1 -m -d 10
> Parameters:
>  # of threads:      1
>  duration:          10
>  ops' range:        1024
> Results:
> Duration:            10 s
>  Throughput:         57.59 Mops/s
>  Throughput/thread:  57.59 Mops/s/thread
> 
> After (with --enable-debug-mutex):
> $ taskset -c 0 tests/atomic_add-bench -n 1 -m -d 10
> Parameters:
>  # of threads:      1
>  duration:          10
>  ops' range:        1024
> Results:
> Duration:            10 s
>  Throughput:         56.25 Mops/s
>  Throughput/thread:  56.25 Mops/s/thread
> 
> NB. The -m option for atomic_add-bench is not upstream yet, but
> feel free to cherry-pick this commit: 
>   https://github.com/cota/qemu/commit/f04f34df
> 
>   + A longer critical section can impact scalability, especially
>     for large core counts.

Thanks for the testing report.  Then I think I'll keep the configure
part, and keep it off by default.

> 
> Also note that there are some alternatives to this.
> On POSIX systems when I need to debug mutexes I just revert
> 24fa904 ("qemu-thread: do not use PTHREAD_MUTEX_ERRORCHECK",
> 2015-03-10). Note that this doesn't work well with fork() in
> linux-user, but I rarely need that.

Note that this patch can even be helpful to debug unlock missing paths
(when owner of a lock forgot to release after use).  AFAIU
PTHREAD_MUTEX_ERRORCHECK won't be able to, since it only provides the
thread ID that has taken the lock (and the lock type) then we can't
tell where the lock is explicitly taken.  Or is there a way that I
missed?

> 
> Another alternative is to trace mutex_lock, that will give
> you the same info although at higher overhead (in both runtime
> and disk usage).
> 
> So I'm not against this, but please keep it configured out.
> BTW you might also want to add the file/line pair to
> qemu-thread-win32.c, or hide the configure option to Windows
> users.

Yes, I missed some other paths that will take the lock, and it won't
be hard to support Windows too.  Thanks for your input.

-- 
Peter Xu