Re: [PATCH 2/3] PowerPC: Add adaptive elision to rwlocks

[Will Schmidt <will_schmidt at vnet dot ibm dot com>]

On Thu, 2014-11-27 at 15:36 -0200, Adhemerval Zanella wrote:
> On 07-11-2014 15:13, Adhemerval Zanella wrote:
> > This patch adds support for lock elision using ISA 2.07 hardware
> > transactional memory for rwlocks.  The logic is similar to the
> > one presented in pthread_mutex lock elision.


Cosmetic comments below.  no code/functional concerns.


> >
> > --
> >
> > 	* sysdeps/powerpc/nptl/elide.h: New file: generic lock elision support
> > 	for powerpc.
> > 	* sysdeps/unix/sysv/linux/powerpc/bits/pthreadtypes.h
> > 	[pthread_rwlock_t] (__pad1): Change size to 7 bytes in 64 bits case
> > 	and remove it for 32 bits case.

I would first ask why the change, but this is actually answered on the
next line.  Since __pad1 is simply padding, and it's being shrunk or
replaced to make room for the new lock elision field, probably not worth
being called out here.

> > 	[pthread_rwlock_t] (__rwelision): New field for lock elision.

> > 	(__PTHREAD_RWLOCK_ELISION_EXTRA): Adjust for new lock elision field
> > 	initialization.
> > 	* sysdeps/unix/sysv/linux/powerpc/elision-conf.c (elision_init):
> > 	Disable lock elision with rdlocks if elision is not available.
> >
> > ---
> >
> > diff --git a/sysdeps/powerpc/nptl/elide.h b/sysdeps/powerpc/nptl/elide.h
> > new file mode 100644
> > index 0000000..5cc47a3
> > --- /dev/null
> > +++ b/sysdeps/powerpc/nptl/elide.h
> > @@ -0,0 +1,111 @@
> > +/* elide.h: Generic lock elision support for powerpc.
> > +   Copyright (C) 2014 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <http://www.gnu.org/licenses/>.  */
> > +
> > +#ifndef ELIDE_PPC_H
> > +# define ELIDE_PPC_H
> > +
> > +#ifdef ENABLE_LOCK_ELISION
> > +# include <htm.h>
> > +# include <elision-conf.h>
> > +
> > +/* Returns true if lock defined by IS_LOCK_FREE was elided.

s/if lock/if the lock/
s/IS_LOCK_FREE/is_lock_free/

> > +   ADAPT_COUNT is a pointer to per-lock state variable. */


> > +
> > +static inline bool
> > +__elide_lock (uint8_t *adapt_count, int is_lock_free)
> > +{
> > +  if (*adapt_count > 0)
> > +    {
> > +      (*adapt_count)--;
> > +      return false;
> > +    }
> > +
> > +  for (int i = __elision_aconf.try_tbegin; i > 0; i--)
> > +    {
> > +      if (__builtin_tbegin (0))
> > +	{
> > +	  if (is_lock_free)
> > +	    return true;
> > +	  /* Lock was busy.  */
> > +	  __builtin_tabort (_ABORT_LOCK_BUSY);
> > +	}
> > +      else
> > +	{
> > +	  /* A persistent failure indicates that a retry will probably
> > +	     result in another failure.  Use normal locking now and
> > +	     for the next couple of calls.  */
> > +	  if (_TEXASRU_FAILURE_PERSISTENT (__builtin_get_texasru ()))
> > +	    {
> > +	      if (__elision_aconf.skip_lock_internal_abort > 0)
> > +		*adapt_count = __elision_aconf.skip_lock_internal_abort;
> > +	      break;
> > +	    }
> > +	  /* Same logic as above, but for for a number of temporary failures in a
> > +	     a row.  */

s/for for/for/
(looking back, I also see this at least once in [1/3] )


> > +	  else if (__elision_aconf.skip_lock_out_of_tbegin_retries > 0
> > +		   && __elision_aconf.try_tbegin > 0)
> > +	    *adapt_count = __elision_aconf.skip_lock_out_of_tbegin_retries;
> > +	}
> > +     }

tab or whitespace indentation glitch here?

> > +
> > +  return false;
> > +}
> > +
> > +# define ELIDE_LOCK(adapt_count, is_lock_free) \
> > +  __elide_lock (&(adapt_count), is_lock_free)
> > +
> > +
> > +static inline bool
> > +__elide_trylock (uint8_t *adapt_count, int is_lock_free, int write)
> > +{
> > +  if (__elision_aconf.try_tbegin > 0)
> > +    {
> > +      if (write)
> > +	__builtin_tabort (_ABORT_NESTED_TRYLOCK);
> > +      return __elide_lock (adapt_count, is_lock_free);
> > +    }
> > +  return false;
> > +}
> > +
> > +# define ELIDE_TRYLOCK(adapt_count, is_lock_free, write)	\
> > +  __elide_trylock (&(adapt_count), is_lock_free, write)
> > +
> > +
> > +static inline bool
> > +__elide_unlock (int is_lock_free)
> > +{
> > +  if (is_lock_free)
> > +    {
> > +      __builtin_tend (0);
> > +      return true;
> > +    }
> > +  return false;
> > +}
> > +
> > +# define ELIDE_UNLOCK(is_lock_free) \
> > +  __elide_unlock (is_lock_free)
> > +
> > +# else
> > +
> > +# define ELIDE_LOCK(adapt_count, is_lock_free) 0
> > +# define ELIDE_TRYLOCK(adapt_count, is_lock_free, write) 0
> > +# define ELIDE_UNLOCK(is_lock_free) 0
> > +
> > +#endif /* ENABLE_LOCK_ELISION  */
> > +
> > +#endif
Ok.

> > diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/pthreadtypes.h b/sysdeps/unix/sysv/linux/powerpc/bits/pthreadtypes.h
> > index 396a3d9..998f6d4 100644
> > --- a/sysdeps/unix/sysv/linux/powerpc/bits/pthreadtypes.h
> > +++ b/sysdeps/unix/sysv/linux/powerpc/bits/pthreadtypes.h
> > @@ -172,11 +172,13 @@ typedef union
> >      unsigned int __nr_writers_queued;
> >      int __writer;
> >      int __shared;
> > -    unsigned long int __pad1;
> > +    unsigned char __rwelision;
> > +    unsigned char __pad1[7];
> >      unsigned long int __pad2;
> >      /* FLAGS must stay at this position in the structure to maintain
> >         binary compatibility.  */
> >      unsigned int __flags;
> > +# define __PTHREAD_RWLOCK_ELISION_EXTRA 0, {0, 0, 0, 0, 0, 0, 0 }
> >    } __data;
> >  # else
> >    struct
> > @@ -187,20 +189,20 @@ typedef union
> >      unsigned int __writer_wakeup;
> >      unsigned int __nr_readers_queued;
> >      unsigned int __nr_writers_queued;
> > -    unsigned char __pad1;
> > +    unsigned char __rwelision;
> >      unsigned char __pad2;
> >      unsigned char __shared;
> >      /* FLAGS must stay at this position in the structure to maintain
> >         binary compatibility.  */
> >      unsigned char __flags;
> >      int __writer;
> > +#define __PTHREAD_RWLOCK_ELISION_EXTRA 0
> >    } __data;
> >  # endif
> >    char __size[__SIZEOF_PTHREAD_RWLOCK_T];
> >    long int __align;
> >  } pthread_rwlock_t;
> >
> > -#define __PTHREAD_RWLOCK_ELISION_EXTRA 0
> >
> >  typedef union
> >  {
Ok.
> > diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> > index 60a7900..44833f1 100644
> > --- a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> > +++ b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
> > @@ -64,6 +64,9 @@ elision_init (int argc __attribute__ ((unused)),
> >  #ifdef ENABLE_LOCK_ELISION
> >    __pthread_force_elision = __libc_enable_secure ? 0 : elision_available;
> >  #endif
> > +  if (!__pthread_force_elision)
> > +    /* Disable elision on rwlocks.  */
> > +    __elision_aconf.try_tbegin = 0;
> >  }

Ok.

> >
> >  #ifdef SHARED
> >
> Ping.
>