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

[Peter Xu]

On Thu, Apr 19, 2018 at 09:56:31AM +0800, Fam Zheng wrote:
> On Wed, 04/18 17:06, Peter Xu wrote:
> > We have had some tracing tools for mutex but it's not easy to use them
> > for e.g. dead locks.  Let's provide "--enable-debug-mutex" parameter
> > when configure to allow QemuMutex to store the last owner that took
> > specific lock.  It might be extremely easy to use this tool to debug
> > deadlocks since we can directly know who took the lock then as long as
> > we can have a gdb attached to the process.
> > 
> > CC: Paolo Bonzini <address@hidden>
> > CC: Stefan Hajnoczi <address@hidden>
> > CC: Fam Zheng <address@hidden>
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> > v2:
> > - fix "--disable-debug-mutex" not working..
> > ---
> >  configure                   | 10 ++++++++++
> >  include/qemu/thread-posix.h |  4 ++++
> >  util/qemu-thread-posix.c    |  4 ++++
> >  3 files changed, 18 insertions(+)
> > 
> > diff --git a/configure b/configure
> > index 0a19b033bc..a80af735b2 100755
> > --- a/configure
> > +++ b/configure
> > @@ -451,6 +451,7 @@ jemalloc="no"
> >  replication="yes"
> >  vxhs=""
> >  libxml2=""
> > +debug_mutex="no"
> >  
> >  supported_cpu="no"
> >  supported_os="no"
> > @@ -1374,6 +1375,10 @@ for opt do
> >    ;;
> >    --disable-git-update) git_update=no
> >    ;;
> > +  --enable-debug-mutex) debug_mutex=yes
> > +  ;;
> > +  --disable-debug-mutex) debug_mutex=no
> > +  ;;
> >    *)
> >        echo "ERROR: unknown option $opt"
> >        echo "Try '$0 --help' for more information"
> > @@ -1631,6 +1636,7 @@ disabled with --disable-FEATURE, default is enabled 
> > if available:
> >    crypto-afalg    Linux AF_ALG crypto backend driver
> >    vhost-user      vhost-user support
> >    capstone        capstone disassembler support
> > +  debug-mutex     mutex debugging support
> >  
> >  NOTE: The object files are built at the place where configure is launched
> >  EOF
> > @@ -5874,6 +5880,7 @@ echo "avx2 optimization $avx2_opt"
> >  echo "replication support $replication"
> >  echo "VxHS block device $vxhs"
> >  echo "capstone          $capstone"
> > +echo "mutex debugging   $debug_mutex"
> >  
> >  if test "$sdl_too_old" = "yes"; then
> >  echo "-> Your SDL version is too old - please upgrade to have SDL support"
> > @@ -6602,6 +6609,9 @@ fi
> >  if test "$capstone" != "no" ; then
> >    echo "CONFIG_CAPSTONE=y" >> $config_host_mak
> >  fi
> > +if test "$debug_mutex" = "yes" ; then
> > +  echo "CONFIG_DEBUG_MUTEX=y" >> $config_host_mak
> > +fi
> >  
> >  # Hold two types of flag:
> >  #   CONFIG_THREAD_SETNAME_BYTHREAD  - we've got a way of setting the name 
> > on
> > diff --git a/include/qemu/thread-posix.h b/include/qemu/thread-posix.h
> > index f3f47e426f..fd27b34128 100644
> > --- a/include/qemu/thread-posix.h
> > +++ b/include/qemu/thread-posix.h
> > @@ -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.

-- 
Peter Xu