Message ID | 545CFDAA.7080502@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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. > > -- > > * 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. > [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. > + 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. */ > + 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; > + } > + } > + > + 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 > 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 > { > 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; > } > > #ifdef SHARED > Ping.
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. >
Hi Will, Thanks for the review, I will send a patchset update soon. Below some comments about your points. On 10-12-2014 17:40, Will Schmidt wrote: > >> --- >> >> 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/ Fixed > >>> + 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] ) Fixed. > > >>> + 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? I will check, but I think it using tab correctly here.
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. + 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. */ + 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; + } + } + + 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 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 { 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; } #ifdef SHARED