This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/3] PowerPC: Add adaptive elision to rwlocks
- From: Will Schmidt <will_schmidt at vnet dot ibm dot com>
- To: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Wed, 10 Dec 2014 13:40:39 -0600
- Subject: Re: [PATCH 2/3] PowerPC: Add adaptive elision to rwlocks
- Authentication-results: sourceware.org; auth=none
- References: <545CFDAA dot 7080502 at linux dot vnet dot ibm dot com> <54776130 dot 9000300 at linux dot vnet dot ibm dot com>
- Reply-to: 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.
>