Re: [PATCH] S/390: Port of lock elision to System/z

[Stefan Liebler <stli at linux dot vnet dot ibm dot com>]

Hi,

thanks for the review.
IÂve changed the mentioned points.
The Changelog remains the same.

Bye

On 05/08/2014 11:44 AM, Andreas Krebbel wrote:
> On 04/30/2014 04:28 PM, Stefan Liebler wrote:
> > Hi,
> >
> > like mentioned in the discussion before, the clobbering of
> > FPRs/cc/memory is not needed anymore. In an older patch-version, the
> > transaction was either started by the builtin_tbegin or an asm("tbegin").
> > In the latter case, the clobbering of FPRs was needed.
> > There was also a non-transactional store, which is not needed anymore.
> > The objdumps of this patch-version with and without the clobbering does
> > not differ.
> >
> > The file nptl/sysdeps/unix/sysv/linux/s390/pthread_mutex_unlock.c is not
> > added anymore, because it only includes the nptl/pthread_mutex_unlock.c.
> >
> > Ok?
> >
> > Bye
> >
> > ---
> > 2014-04-30  Dominik Vogt  <vogt@linux.vnet.ibm.com>
> >               Stefan Liebler  <stli@linux.vnet.ibm.com>
> >
> > 	* config.make.in (enable-lock-elision): New Makefile variable.
> > 	* configure.ac: Likewise.
> > 	* configure: Regenerate.
> > 	* sysdeps/s390/configure.ac:
> > 	Add check for gcc transactions support.
> > 	* sysdeps/s390/configure: Regenerate.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/Makefile: New file.
> > 	Build elision files if enabled.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/elision-conf.c: New file.
> > 	Add lock elision support for s390.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/elision-conf.h: Likewise.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/elision-lock.c: Likewise.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/elision-timed.c: Likewise.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/elision-trylock.c: Likewise.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/elision-unlock.c: Likewise.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/force-elision.h: Likewise.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/pthread_mutex_cond_lock.c:
> > 	Likewise.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/pthread_mutex_lock.c:
> > 	Likewise.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/pthread_mutex_timedlock.c:
> > 	Likewise.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/pthread_mutex_trylock.c:
> > 	Likewise.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h:
> > 	(__lll_timedlock_elision, __lll_lock_elision)
> > 	(__lll_unlock_elision, __lll_trylock_elision)
> > 	(lll_timedlock_elision, lll_lock_elision)
> > 	(lll_unlock_elision, lll_trylock_elision): Add.
> > 	* nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h
> > 	(pthread_mutex_t): Add lock elision support for s390.
> > ---
>
> Just some (very) minor nitpicking:
>
> nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h:
>
> +#ifdef ENABLE_LOCK_ELISION
> +extern int __lll_lock_elision (int *futex, short *adapt_count, int private)
> +  attribute_hidden;
> +
> +extern int __lll_unlock_elision(int *lock, int private)
> +  attribute_hidden;
> +
> +extern int __lll_trylock_elision(int *lock, short *adapt_count)
> +  attribute_hidden;
>
> What's the reason for calling the first argument futex once and lock
> in the other cases?
No. Now the argument is called futex in all cases.
Same for nptl/sysdeps/unix/sysv/linux/s390/elision-unlock.c.

> nptl/sysdeps/unix/sysv/linux/s390/elision-conf.c:
>
> +{
> +  /* Set when the CPU supports elision. When false elision is never
>    attempted.  */
> ... when the CPU and the kernel support transactional execution. ...
>
> +  int elision_available = (GLRO (dl_hwcap) & HWCAP_S390_TE) ? 1 : 0;
ok

> nptl/sysdeps/unix/sysv/linux/s390/elision-lock.c:
>
> +int
> +__lll_lock_elision (int *futex, short *adapt_count, EXTRAARG int private)
> +{
> +  if (*adapt_count <= 0)
> +    {
> ...
> +    {
> +  else
> +    {
> +      /* Lost updates are possible, but harmless.  Due to races this might lead
> +	 to *adapt_count becoming less than zero.  */
> +      (*adapt_count)--;
> +    }
> +
> +  use_lock:
> +  return LLL_LOCK ((*futex), private);
> +}
>
> Perhaps something like this to reduce indentation of the whole function?!
>
> +int
> +__lll_lock_elision (int *futex, short *adapt_count, EXTRAARG int private)
> +{
> +  if (*adapt_count > 0)
> +    {
> +      /* Lost updates are possible, but harmless.  Due to races this might lead
> +	 to *adapt_count becoming less than zero.  */
> +      (*adapt_count)--;
> +      goto use_lock;
> +    }
> ...
> +
> +  use_lock:
> +  return LLL_LOCK ((*futex), private);
> +}
ok

> +      /* Same logic as above, but for for a number of tepmorary failures in a
> +	 row.  */
>
> ... temporary ...
ok

> nptl/sysdeps/unix/sysv/linux/s390/elision-trylock.c:
>
> +  /* Implement POSIX semantics by forbiding nesting elided trylocks.
> ... forbidding ...
>
> +      __builtin_tabort (_HTM_FIRST_USER_ABORT_CODE + 1);
>
> Here you want to set the lowest order bit in order to enforce a
> persistent abort.  While the code works fine since
> _HTM_FIRST_USER_ABORT_CODE is even (_HTM_FIRST_USER_ABORT_CODE | 1)
> would appear more natural to me.  The same appears in elision-lock.c.
ok

> Bye,
>
> -Andreas-

Attachment: patchglibc_tx_20140509
Description: Text document